Bug 61091 – [chromium] Disable blending in compositor for WebGL layers with alpha=false

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

On Macs with ATI / AMD cards it appears that although we allocate a GL_RGB texture for WebGL contexts with the context creation attribute {alpha:false}, internally it actually allocates a four-channel texture and leaves the alpha channel blank. When we later enable blending in the compositor, the WebGL content becomes completely transparent.

To be more robust, the compositor should disable blending when rendering WebGL layers with {alpha:false} specified.

[John Bauman]

Is it defined what happens if a webgl application does glBlendFunc with GL_DST_ALPHA in that case?

[Kenneth Russell]

Created attachment 94016 [details]
Patch

[Kenneth Russell]

(In reply to comment #1)
> Is it defined what happens if a webgl application does glBlendFunc with GL_DST_ALPHA in that case?

The OpenGL ES 2.0 spec is supposed to define this behavior. I suspect that on these apparently buggy ATI drivers the wrong result will be generated. It will probably be hard to emulate the correct behavior in all cases.

[Vangelis Kokkevis]

Comment on attachment 94016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94016&action=review

Couple of nits, otherwise lgtm

> Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.cpp:64
>      GC3Denum sfactor = m_premultipliedAlpha ? GraphicsContext3D::ONE : GraphicsContext3D::SRC_ALPHA;

The blendFunc() setting can go in an else clause

> LayoutTests/compositing/webgl/webgl-no-alpha.html:26
> +            // layoutTestController.dumpAsText(true);

Does the layer tree provide any useful information here? You probably should be calling dumpAsText(true) and eliminate the -expected.txt file.

> LayoutTests/compositing/webgl/webgl-no-alpha.html:46
> +          var gl = initWebGL(canvasID, "", "", [], [ 0, 1, 0, 0 ], 1);

Not a biggie but initWebGL could be folded into this function to avoid passing all these parameters that go unused.

> LayoutTests/compositing/webgl/webgl-no-alpha.html:54
> +       }

watch indentation

[Kenneth Russell]

(In reply to comment #4)
> (From update of attachment 94016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94016&action=review
> 
> Couple of nits, otherwise lgtm
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.cpp:64
> >      GC3Denum sfactor = m_premultipliedAlpha ? GraphicsContext3D::ONE : GraphicsContext3D::SRC_ALPHA;
> 
> The blendFunc() setting can go in an else clause

I'll make this change.

> > LayoutTests/compositing/webgl/webgl-no-alpha.html:26
> > +            // layoutTestController.dumpAsText(true);
> 
> Does the layer tree provide any useful information here? You probably should be calling dumpAsText(true) and eliminate the -expected.txt file.

I think it provides more useful information than dumpAsText would -- it might catch something in the future.

> > LayoutTests/compositing/webgl/webgl-no-alpha.html:46
> > +          var gl = initWebGL(canvasID, "", "", [], [ 0, 1, 0, 0 ], 1);
> 
> Not a biggie but initWebGL could be folded into this function to avoid passing all these parameters that go unused.

This structure is shared among other tests in this directory so I'd rather leave it.

> > LayoutTests/compositing/webgl/webgl-no-alpha.html:54
> > +       }
> 
> watch indentation

Will fix.

[Kenneth Russell]

Created attachment 94136 [details]
Patch

[James Robinson]

Comment on attachment 94136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94136&action=review

Code looks good but the test shouldn't output a render tree.

> LayoutTests/compositing/webgl/webgl-no-alpha.html:27
> +        function initWebGL(canvasName, vshader, fshader, attribs, clearColor, clearDepth)

if this is common to tests in this directory should it be moved to a helper .js file?

> LayoutTests/platform/chromium-gpu/compositing/webgl/webgl-no-alpha-expected.txt:11
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x472
> +  RenderBlock {HTML} at (0,0) size 800x472
> +    RenderBody {BODY} at (8,20) size 784x444
> +      RenderBlock {DIV} at (40,0) size 200x200 [bgcolor=#00FF00]
> +      RenderBlock (anonymous) at (0,200) size 784x244
> +        RenderText {#text} at (0,0) size 0x0
> +        RenderText {#text} at (0,0) size 0x0
> +layer at (28,240) size 240x200
> +  RenderHTMLCanvas {CANVAS} at (20,20) size 240x200

the layer tree is noise on this test and will just cause maintenance headaches for gardeners.  This should be a dumpAsText(true) test

[Kenneth Russell]

Created attachment 94142 [details]
Patch

Dump text instead of layer tree in layout test

[James Robinson]

Comment on attachment 94142 [details]
Patch

R=me

[Kenneth Russell]

Committed r86905: <http://trac.webkit.org/changeset/86905>