Support some form of component alpha with GL layers

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: roc, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(2 attachments, 17 obsolete attachments)

17.79 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
40.16 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
GL version of bug 593604.

ARB_blend_func_extended is one approach, but it doesn't seem to be widely supported yet. Need to investigate whether there are other options.
glColorMask is another option, but it would mean having to composite each ThebesLayer in three separate operations (one per color channel, assuming we can write the alpha channel in the same pass as one of the color channels).

On my rather new Linux box at home, with a highish-end Nvidia card, ARB_blend_func_extended is not listed anywhere by glxinfo, and I don't see any other extensions that would be helpful. Nor is ARB_blend_func_extended supported on my (old) Macbook Pro.

The good news for GL is that on both Mac and X, we can draw text to a transparent surface and get subpixel AA as long as the text is over opaque pixels. So we need component alpha less often than we do with D3D+D2D.
And on low-end stuff (mobile), we can just suck it up and disable subpixel AA in the layers that have text over transparent pixels.
Blocks: 582223
Blocks: 593389
Blocks: 602757
Blocks: 606075
Blocks: 607740
Blocks: 608281
blocking2.0: --- → ?
Depends on: 612840
No longer blocks: 602757
I'm assigning this to myself for now, but I'm open to it being stolen from me!
Assignee: nobody → joe
blocking2.0: ? → betaN+
(Assignee)

Updated

7 years ago
Assignee: joe → matt.woodrow+bugzilla
(Assignee)

Comment 4

7 years ago
Created attachment 493309 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface

To use a tee surface we need to get back a surface from BeginPaint. Grabbing the current surface from the returned context works, but discards any state that the context may have been holding (even though the current implementations don't have any other than clipping).

The current docs also say that no clipping will be applied, yet EGL and CGL clip to the surface size. I've left these out of the current patch because clipping to the surface size sounds like a nop. Will need to test this properly to confirm.
Attachment #493309 - Flags: review?(jones.chris.g)
(Assignee)

Comment 5

7 years ago
Created attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

Still needs more tidying and testing, WIP.

This will only work on mac where it uses the BasicBufferOGL path instead of SurfaceBufferOGL. SurfaceBufferOGL logic would attempt to set the surface as a source (for self copying during scrolling) and tee surfaces don't support this. Haven't got a solution in mind for this yet.

Also only works with BGR surfaces, this probably wont always be the case, especially with linux/mobile.
Attachment #493310 - Flags: review?(roc)
(Assignee)

Comment 6

7 years ago
Created attachment 493311 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha
Attachment #493311 - Flags: review?(roc)
(Assignee)

Comment 7

7 years ago
Above patches based on top of http://hg.mozilla.org/mozilla-central/rev/4e66b4cb5e3a + http://hg.mozilla.org/users/rocallahan_mozilla.com/main/rev/d34011c63db3 up to patch "bug602757-opaque-rects"
(Assignee)

Comment 8

7 years ago
Possible idea for SurfaceBufferOGL would be to allow setting a tee surface as a source if the destination is also a tee surface, and has the same number of child surfaces as the source. Then get cairo to copy each of the children separately.

Very open to more elegant ideas.
I don't think mobile wants or needs component alpha. I'm totally OK with a solution that's restricted to Mac and desktop Linux.
Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

Looks right to me, but you should also get review from someone who actually knows GL, like vlad maybe.
Attachment #493310 - Flags: review?(roc) → review+
+      aContainer->mSupportsComponentAlphaChildren = PR_TRUE;

We should be setting this to false somewhere.

+LayerManagerOGL::CreateFBOWithTexture(nsIntRect aRect, bool aCopy,

const nsIntRect&

+  if (!aCopy) {
+      mGLContext->fClearColor(0.0, 0.0, 0.0, 0.0);
+      mGLContext->fClear(LOCAL_GL_COLOR_BUFFER_BIT);
+  }

fix indent

+      // don't need a background, we're going to paint all opaque stuff

We actually do end up clearing in CreateFBOWithTexture anyway. It may be worth avoiding that.

The rest looks great.
(Assignee)

Comment 12

7 years ago
Created attachment 493316 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v2
Attachment #493311 - Attachment is obsolete: true
Attachment #493316 - Flags: review?(roc)
Attachment #493311 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #493310 - Flags: review?(vladimir)
Comment on attachment 493316 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v2

(In reply to comment #11)
> +LayerManagerOGL::CreateFBOWithTexture(nsIntRect aRect, bool aCopy,
> 
> const nsIntRect&

You missed this!
Attachment #493316 - Flags: review?(roc) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 493318 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v3

Fixed review comment, carrying forward r=roc
Attachment #493316 - Attachment is obsolete: true
Attachment #493318 - Flags: review+
(In reply to comment #8)
> Possible idea for SurfaceBufferOGL would be to allow setting a tee surface as a
> source if the destination is also a tee surface, and has the same number of
> child surfaces as the source. Then get cairo to copy each of the children
> separately.

That sounds right.
Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

I can review this tomorrow, unlike Vlad. :)
Attachment #493310 - Flags: review?(vladimir) → review?(joe)
Comment on attachment 493309 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface

And this too!
Attachment #493309 - Flags: review?(jones.chris.g) → review?(joe)
Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

hooray!
Attachment #493310 - Flags: review?(joe) → review+
Comment on attachment 493309 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface

I have never really loved the fact that BeginUpdate returns a bare pointer, rather than an already_AddRefed, but that isn't in the scope of this patch.

One issue is that on CGL, we clipped the context to the area we are going to draw into, because we reuse mUpdateSurface. While we *should* only ever draw into those areas, it's not guaranteed without a clip.
Attachment #493309 - Flags: review?(joe) → review+
(Assignee)

Comment 20

7 years ago
Created attachment 493502 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface v2

Added comment in TextureImageCGL::BeginPaint to clarify the clipping changes.

Carrying forward r=joe
Attachment #493309 - Attachment is obsolete: true
Attachment #493502 - Flags: review+
(Assignee)

Comment 21

7 years ago
Created attachment 493503 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v4

Fixed bug with copying up the existing content. We need to account for the offset, translation and GL's coordinate system.

Carrying forward r=roc
Attachment #493318 - Attachment is obsolete: true
Attachment #493503 - Flags: review+
Blocks: 618211
I've landed component alpha for D3D9, so I think this can land now.

But, is there anything here to disable component alpha for mobile?
Whiteboard: [hard blocker]
Whiteboard: [hard blocker] → [hardblocker]
Whiteboard: [hardblocker] → [hardblocker][needs landing]
The patches here have bitrotted a lot in the meantime.
Whiteboard: [hardblocker][needs landing] → [hardblocker][needs updated patches]
Created attachment 503112 [details] [diff] [review]
part 1, v3, wip (Change TextureImage::BeginUpdate to return a surface)

Updated on top of bug 621778. I need to double-check the changes to GLContextProviderEGL before this is ready.
Attachment #493502 - Attachment is obsolete: true
Created attachment 503113 [details] [diff] [review]
part 2, v5, wip (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

This patch contains both the changes to ThebesLayerOGL and to ContainerLayerOGL. Two places (marked with XXX) still need some clean up.
Attachment #493310 - Attachment is obsolete: true
Attachment #493503 - Attachment is obsolete: true
Created attachment 503145 [details] [diff] [review]
part 1, v4 (Change TextureImage::BeginUpdate to return a surface)

The EGL changes should be correct now.
Attachment #503112 - Attachment is obsolete: true
Created attachment 503147 [details] [diff] [review]
part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

Somebody could have another look at the tee surface device offset issue (marked with XXX) but I think this can land in this state. I'll wait for tryserver results first, though.

I've added some code that turns off component alpha inside an
#ifdef MOZ_GFX_OPTIMIZE_MOBILE.

Tryserver build will appear here: http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-cd446a0bb852/try-osx64/
Created attachment 503152 [details] [diff] [review]
part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

missed a brace
Attachment #503147 - Attachment is obsolete: true
Created attachment 503229 [details] [diff] [review]
part 2, v7 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

I guess this could need another review. I don't want to land it before bug 621778 anyway since these patches are on top of the patch over there.

Binding the textures to GL_TEXTURE0 and GL_TEXTURE1 needs to happen after EndUpdate, since EndUpdate uses UploadSurfaceToTexture which calls fActiveTexture(LOCAL_GL_TEXTURE0) before binding the texture.

The changes in GLContextProviderCGL.mm make sure that the right pixel buffer is bound at the right time. Before that change, EndUpdate didn't re-bind the right buffer. That only worked because there was only one pixel buffer per texture image, and that was still bound from BeginUpdate. But now that there can be two pixel buffers with interleaving BeginUpdate/EndUpdate calls, EndUpdate needs to re-bind the right pixel buffer, and now it does so by calling FinishedSurfaceUpdate.

I've also tweaked the shaders a bit.

The new tryserver build will appear in http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-946c9d30467e/try-osx64/
Attachment #503113 - Attachment is obsolete: true
Attachment #503152 - Attachment is obsolete: true
Attachment #503229 - Flags: review?(joe)
Comment on attachment 503229 [details] [diff] [review]
part 2, v7 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

>diff --git a/gfx/layers/opengl/LayerManagerOGL.h b/gfx/layers/opengl/LayerManagerOGL.h
>@@ -261,23 +269,29 @@ public:

>+  enum InitMode {
>+    InitModeNone,
>+    InitModeClear,
>+    InitModeCopy
>+  };

These should be documented. In particular, InitModeCopy should be specified as copying from the current glReadBuffer. (Incidentally, I thought it was weird that CreateFBOWithTexture doesn't call ReadBuffer; seems like a bug waiting to happen.)

>diff --git a/gfx/layers/opengl/LayerManagerOGLProgram.h b/gfx/layers/opengl/LayerManagerOGLProgram.h

>@@ -637,16 +637,81 @@ public:

>+ * A ComponentAlphaTextureLayerProgram is a LayerProgram that renders
>+ * a single texture.  It adds the following attributes and uniforms:

It does more than that, doesn't it?

>+ *
>+ * Attribute inputs:
>+ *   aTexCoord     - texture coordinate
>+ *
>+ * Uniforms:
>+ *   uTexture         - 2D texture unit which to sample

also uBlackTexture, uWhiteTexture
Attachment #503229 - Flags: review?(joe) → review+
Created attachment 503422 [details] [diff] [review]
part 2, v8, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-3f03d57f05c9/try-osx64/firefox-4.0b10pre.en-US.mac64.dmg
Attachment #503229 - Attachment is obsolete: true
Whiteboard: [hardblocker][needs updated patches] → [hardblocker][needs landing after bug 621778]
Depends on: 625357
Depends on: 625705
Created attachment 503862 [details] [diff] [review]
part 2, v9, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

There was another bug in here: mTexImageOnWhite wasn't cleared when the layer transitioned from being component-alpha to being not-component-alpha. Then only mTexImage was updated, but due to mTexImageOnWhite being non-null, the painting part still thought we were in component alpha mode, which resulted in garbage.
I've added a reftest.
Attachment #503422 - Attachment is obsolete: true
(Assignee)

Comment 33

7 years ago
Thanks for fixing these while I was away Markus!
Whiteboard: [hardblocker][needs landing after bug 621778] → [hardblocker][needs landing after bug 621778][january 16]
No problem. Unfortunately it looks like we're still not done...
Somebody needs to look at the reftests failures in http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1295104836.1295105692.8649.gz which I had on my push to try http://hg.mozilla.org/try/pushloghtml?changeset=56ed4ca51a14
(though I'm just noticing that the opacity-mixed-scrolling-2.html failure is very strange indeed because I pushed an incomplete reftest (scrolling.js is missing))
scrolling.js should already be there on trunk!
Oh. Now it makes a lot more sense.
(Assignee)

Comment 37

7 years ago
Can anyone reproduce these failures locally?

I get a few failures, but both of those two pass.
I can. The 602200-4.html failure is intermittent, but it goes away if I make this change:

diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp
--- a/gfx/layers/opengl/LayerManagerOGL.cpp
+++ b/gfx/layers/opengl/LayerManagerOGL.cpp
@@ -908,17 +908,17 @@ LayerManagerOGL::CreateFBOWithTexture(co
 
   mGLContext->fActiveTexture(LOCAL_GL_TEXTURE0);
   mGLContext->fGenTextures(1, &tex);
   mGLContext->fBindTexture(mFBOTextureTarget, tex);
   if (aInit == InitModeCopy) {
     mGLContext->fCopyTexImage2D(mFBOTextureTarget,
                                 0,
                                 LOCAL_GL_RGBA,
-                                aRect.x, aRect.y,
+                                0, 0,
                                 aRect.width, aRect.height,
                                 0);
   } else {
     mGLContext->fTexImage2D(mFBOTextureTarget,
                             0,
                             LOCAL_GL_RGBA,
                             aRect.width, aRect.height,
                             0,

I haven't thought this through completely yet. This change might not be correct.

The opacity-mixed-scrolling-2.html failure was from bug 625357 and the test has been disabled.
(In reply to comment #38)
> I can. The 602200-4.html failure is intermittent, but it goes away if I make
> this change:
> 
> diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp
> b/gfx/layers/opengl/LayerManagerOGL.cpp
> --- a/gfx/layers/opengl/LayerManagerOGL.cpp
> +++ b/gfx/layers/opengl/LayerManagerOGL.cpp
> @@ -908,17 +908,17 @@ LayerManagerOGL::CreateFBOWithTexture(co
> 
>    mGLContext->fActiveTexture(LOCAL_GL_TEXTURE0);
>    mGLContext->fGenTextures(1, &tex);
>    mGLContext->fBindTexture(mFBOTextureTarget, tex);
>    if (aInit == InitModeCopy) {
>      mGLContext->fCopyTexImage2D(mFBOTextureTarget,
>                                  0,
>                                  LOCAL_GL_RGBA,
> -                                aRect.x, aRect.y,
> +                                0, 0,
>                                  aRect.width, aRect.height,
>                                  0);
>    } else {
>      mGLContext->fTexImage2D(mFBOTextureTarget,
>                              0,
>                              LOCAL_GL_RGBA,
>                              aRect.width, aRect.height,
>                              0,
> 
> I haven't thought this through completely yet. This change might not be
> correct.
> 
> The opacity-mixed-scrolling-2.html failure was from bug 625357 and the test has
> been disabled.

Matt, could this be related to the 602200 failure for D3D10 component alpha?
(Assignee)

Comment 40

7 years ago
Created attachment 504538 [details] [diff] [review]
part 2, v10, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

I have no idea how you found that, but great catch!

It does fail for me about 9 times out of 10, must have got unlucky (or is it lucky?) on my reftest run.

We were adding the child offset to the visible rectangle, instead of subtracting it. This patch fixes it for me.

Bas: The same bug is in the D3D10 patches, so I'd say fairly likely. D3D9 doesn't take the child offset into account at all, not sure how that works..
Attachment #503862 - Attachment is obsolete: true
Created attachment 504553 [details] [diff] [review]
part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface)

updated to trunk
Attachment #503145 - Attachment is obsolete: true
Attachment #504553 - Attachment description: 503145: part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface) → part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface)
(Assignee)

Updated

7 years ago
Depends on: 621778
(Assignee)

Comment 42

7 years ago
Created attachment 504596 [details] [diff] [review]
part 1, v5, for checkin (Change TextureImage::BeginUpdate to return a surface)

Rebased against tip
Attachment #504553 - Attachment is obsolete: true
Attachment #504596 - Flags: review+
(Assignee)

Comment 43

7 years ago
Created attachment 504598 [details] [diff] [review]
part 2, v11, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

Rebased against tip.

Pushed to try one final time as http://hg.mozilla.org/try/pushloghtml?changeset=e67c26c64dd5
Attachment #504538 - Attachment is obsolete: true
Attachment #504598 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/bcb3065d87d6
http://hg.mozilla.org/mozilla-central/rev/6550455f427e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs landing after bug 621778][january 16] → [hardblocker]
Target Milestone: --- → mozilla2.0b10
Version: unspecified → Trunk
Depends on: 627661
Blocks: 715232

Updated

4 years ago
Blocks: 840856

Updated

4 years ago
No longer blocks: 840856
Depends on: 840856
No longer blocks: 607740
You need to log in before you can comment on or make changes to this bug.