Last Comment Bug 593733 - Support some form of component alpha with GL layers
: Support some form of component alpha with GL layers
Status: RESOLVED FIXED
[hardblocker]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla2.0b10
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on: 625705 612840 621778 625357 627661 840856
Blocks: 593389 582223 606075 607740 608281 618211 715232
  Show dependency treegraph
 
Reported: 2010-09-05 16:26 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2013-02-12 23:59 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Change TextureImage::BeginPaint to return a surface (17.85 KB, patch)
2010-11-25 16:32 PST, Matt Woodrow (:mattwoodrow)
joe: review+
Details | Diff | Splinter Review
Make ThebesLayerOGL support Component Alpha (21.26 KB, patch)
2010-11-25 16:35 PST, Matt Woodrow (:mattwoodrow)
roc: review+
joe: review+
Details | Diff | Splinter Review
Make ContainerLayerOGL support Component Alpha (7.09 KB, patch)
2010-11-25 16:36 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make ContainerLayerOGL support Component Alpha v2 (7.68 KB, patch)
2010-11-25 17:33 PST, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Make ContainerLayerOGL support Component Alpha v3 (7.70 KB, patch)
2010-11-25 17:46 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Change TextureImage::BeginPaint to return a surface v2 (18.07 KB, patch)
2010-11-26 22:18 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Make ContainerLayerOGL support Component Alpha v4 (8.05 KB, patch)
2010-11-26 22:21 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
part 1, v3, wip (Change TextureImage::BeginUpdate to return a surface) (16.51 KB, patch)
2011-01-12 03:30 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v5, wip (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (31.45 KB, patch)
2011-01-12 03:35 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v4 (Change TextureImage::BeginUpdate to return a surface) (16.18 KB, patch)
2011-01-12 07:01 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (31.02 KB, patch)
2011-01-12 07:14 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (31.02 KB, patch)
2011-01-12 07:26 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v7 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (37.20 KB, patch)
2011-01-12 11:16 PST, Markus Stange [:mstange]
joe: review+
Details | Diff | Splinter Review
part 2, v8, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (37.65 KB, patch)
2011-01-12 23:41 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v9, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (40.06 KB, patch)
2011-01-14 08:54 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v10, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (40.03 KB, patch)
2011-01-17 12:28 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface) (17.72 KB, patch)
2011-01-17 13:37 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 1, v5, for checkin (Change TextureImage::BeginUpdate to return a surface) (17.79 KB, patch)
2011-01-17 18:16 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
part 2, v11, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha) (40.16 KB, patch)
2011-01-17 18:19 PST, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-05 16:26:55 PDT
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.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-05 16:33:18 PDT
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.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-05 16:35:59 PDT
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.
Comment 3 Joe Drew (not getting mail) 2010-11-17 11:38:01 PST
I'm assigning this to myself for now, but I'm open to it being stolen from me!
Comment 4 Matt Woodrow (:mattwoodrow) 2010-11-25 16:32:40 PST
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.
Comment 5 Matt Woodrow (:mattwoodrow) 2010-11-25 16:35:57 PST
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.
Comment 6 Matt Woodrow (:mattwoodrow) 2010-11-25 16:36:27 PST
Created attachment 493311 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha
Comment 7 Matt Woodrow (:mattwoodrow) 2010-11-25 16:39:19 PST
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"
Comment 8 Matt Woodrow (:mattwoodrow) 2010-11-25 16:54:50 PST
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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-25 17:01:35 PST
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 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-25 17:06:27 PST
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.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-25 17:12:49 PST
+      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.
Comment 12 Matt Woodrow (:mattwoodrow) 2010-11-25 17:33:48 PST
Created attachment 493316 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v2
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-25 17:40:28 PST
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!
Comment 14 Matt Woodrow (:mattwoodrow) 2010-11-25 17:46:55 PST
Created attachment 493318 [details] [diff] [review]
Make ContainerLayerOGL support Component Alpha v3

Fixed review comment, carrying forward r=roc
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-25 17:48:12 PST
(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 16 Joe Drew (not getting mail) 2010-11-25 17:59:53 PST
Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

I can review this tomorrow, unlike Vlad. :)
Comment 17 Joe Drew (not getting mail) 2010-11-25 18:00:18 PST
Comment on attachment 493309 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface

And this too!
Comment 18 Joe Drew (not getting mail) 2010-11-26 09:41:08 PST
Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

hooray!
Comment 19 Joe Drew (not getting mail) 2010-11-26 09:48:50 PST
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.
Comment 20 Matt Woodrow (:mattwoodrow) 2010-11-26 22:18:15 PST
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
Comment 21 Matt Woodrow (:mattwoodrow) 2010-11-26 22:21:46 PST
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
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-02 20:22:58 PST
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?
Comment 23 Markus Stange [:mstange] 2011-01-11 22:48:42 PST
The patches here have bitrotted a lot in the meantime.
Comment 24 Markus Stange [:mstange] 2011-01-12 03:30:42 PST
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.
Comment 25 Markus Stange [:mstange] 2011-01-12 03:35:41 PST
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.
Comment 26 Markus Stange [:mstange] 2011-01-12 07:01:50 PST
Created attachment 503145 [details] [diff] [review]
part 1, v4 (Change TextureImage::BeginUpdate to return a surface)

The EGL changes should be correct now.
Comment 27 Markus Stange [:mstange] 2011-01-12 07:14:46 PST
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/
Comment 28 Markus Stange [:mstange] 2011-01-12 07:26:31 PST
Created attachment 503152 [details] [diff] [review]
part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

missed a brace
Comment 29 Markus Stange [:mstange] 2011-01-12 11:16:36 PST
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/
Comment 30 Joe Drew (not getting mail) 2011-01-12 12:48:13 PST
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
Comment 31 Markus Stange [:mstange] 2011-01-12 23:41:24 PST
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
Comment 32 Markus Stange [:mstange] 2011-01-14 08:54:10 PST
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.
Comment 33 Matt Woodrow (:mattwoodrow) 2011-01-15 11:24:58 PST
Thanks for fixing these while I was away Markus!
Comment 34 Markus Stange [:mstange] 2011-01-15 17:44:32 PST
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))
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-16 00:30:19 PST
scrolling.js should already be there on trunk!
Comment 36 Markus Stange [:mstange] 2011-01-16 07:16:32 PST
Oh. Now it makes a lot more sense.
Comment 37 Matt Woodrow (:mattwoodrow) 2011-01-16 18:03:04 PST
Can anyone reproduce these failures locally?

I get a few failures, but both of those two pass.
Comment 38 Markus Stange [:mstange] 2011-01-17 08:26:02 PST
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.
Comment 39 Bas Schouten (:bas.schouten) 2011-01-17 08:30:36 PST
(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?
Comment 40 Matt Woodrow (:mattwoodrow) 2011-01-17 12:28:47 PST
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..
Comment 41 Markus Stange [:mstange] 2011-01-17 13:37:50 PST
Created attachment 504553 [details] [diff] [review]
part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface)

updated to trunk
Comment 42 Matt Woodrow (:mattwoodrow) 2011-01-17 18:16:38 PST
Created attachment 504596 [details] [diff] [review]
part 1, v5, for checkin (Change TextureImage::BeginUpdate to return a surface)

Rebased against tip
Comment 43 Matt Woodrow (:mattwoodrow) 2011-01-17 18:19:35 PST
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

Note You need to log in before you can comment on or make changes to this bug.