Bug 61091 - [chromium] Disable blending in compositor for WebGL layers with alpha=false
Summary: [chromium] Disable blending in compositor for WebGL layers with alpha=false
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-18 14:34 PDT by Kenneth Russell
Modified: 2011-05-19 16:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (31.06 KB, patch)
2011-05-18 17:59 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (31.33 KB, patch)
2011-05-19 16:02 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (30.45 KB, patch)
2011-05-19 16:23 PDT, Kenneth Russell
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-05-18 14:34:34 PDT
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.
Comment 1 John Bauman 2011-05-18 17:14:58 PDT
Is it defined what happens if a webgl application does glBlendFunc with GL_DST_ALPHA in that case?
Comment 2 Kenneth Russell 2011-05-18 17:59:51 PDT
Created attachment 94016 [details]
Patch
Comment 3 Kenneth Russell 2011-05-18 18:07:42 PDT
(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.
Comment 4 Vangelis Kokkevis 2011-05-19 14:08:49 PDT
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
Comment 5 Kenneth Russell 2011-05-19 15:29:39 PDT
(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.
Comment 6 Kenneth Russell 2011-05-19 16:02:58 PDT
Created attachment 94136 [details]
Patch
Comment 7 James Robinson 2011-05-19 16:09:07 PDT
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
Comment 8 Kenneth Russell 2011-05-19 16:23:46 PDT
Created attachment 94142 [details]
Patch

Dump text instead of layer tree in layout test
Comment 9 James Robinson 2011-05-19 16:25:05 PDT
Comment on attachment 94142 [details]
Patch

R=me
Comment 10 Kenneth Russell 2011-05-19 16:43:59 PDT
Committed r86905: <http://trac.webkit.org/changeset/86905>