593733 - Support some form of component alpha with GL layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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)]

I'm assigning this to myself for now, but I'm open to it being stolen from me!

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: 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)]

Attachment: 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)]

Attachment: Make ContainerLayerOGL support Component Alpha

Comment 7

[Matt Woodrow (:mattwoodrow)]

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)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

+      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)]

Attachment: Make ContainerLayerOGL support Component Alpha v2

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

Attachment: Make ContainerLayerOGL support Component Alpha v3

Fixed review comment, carrying forward r=roc

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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)]

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)]

Comment on attachment 493309 [details] [diff] [review]
Change TextureImage::BeginPaint to return a surface

And this too!

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 493310 [details] [diff] [review]
Make ThebesLayerOGL support Component Alpha

hooray!

Comment 19

[Joe Drew (not getting mail)]

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)]

Attachment: 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)]

Attachment: 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) (email my personal email if necessary)]

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]]

The patches here have bitrotted a lot in the meantime.

Comment 24

[Markus Stange [:mstange]]

Attachment: 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]]

Attachment: 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]]

Attachment: part 1, v4 (Change TextureImage::BeginUpdate to return a surface)

The EGL changes should be correct now.

Comment 27

[Markus Stange [:mstange]]

Attachment: 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/[e-mail]-cd446a0bb852/try-osx64/

Comment 28

[Markus Stange [:mstange]]

Attachment: part 2, v6 (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

missed a brace

Comment 29

[Markus Stange [:mstange]]

Attachment: 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/[e-mail]-946c9d30467e/try-osx64/

Comment 30

[Joe Drew (not getting mail)]

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]]

Attachment: part 2, v8, for checkin (Make ThebesLayerOGL and ContainerLayerOGL support component alpha)

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3f03d57f05c9/try-osx64/firefox-4.0b10pre.en-US.mac64.dmg

Comment 32

[Markus Stange [:mstange]]

Attachment: 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)]

Thanks for fixing these while I was away Markus!

Comment 34

[Markus Stange [:mstange]]

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) (email my personal email if necessary)]

scrolling.js should already be there on trunk!

Comment 36

[Markus Stange [:mstange]]

Oh. Now it makes a lot more sense.

Comment 37

[Matt Woodrow (:mattwoodrow)]

Can anyone reproduce these failures locally?

I get a few failures, but both of those two pass.

Comment 38

[Markus Stange [:mstange]]

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)]

(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)]

Attachment: 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]]

Attachment: part 1, v4, for checkin (Change TextureImage::BeginUpdate to return a surface)

updated to trunk

Comment 42

[Matt Woodrow (:mattwoodrow)]

Attachment: part 1, v5, for checkin (Change TextureImage::BeginUpdate to return a surface)

Rebased against tip

Comment 43

[Matt Woodrow (:mattwoodrow)]

Attachment: 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

Comment 44

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/bcb3065d87d6
http://hg.mozilla.org/mozilla-central/rev/6550455f427e