Bug 55411 - [chromium] premultipliedAlpha WebGL context attribute is ignored.
Summary: [chromium] premultipliedAlpha WebGL context attribute is ignored.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: John Bauman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-28 14:18 PST by John Bauman
Modified: 2011-03-07 14:05 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2011-02-28 14:28 PST, John Bauman
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2011-02-28 14:43 PST, John Bauman
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2011-02-28 16:03 PST, John Bauman
no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2011-03-01 14:18 PST, John Bauman
no flags Details | Formatted Diff | Diff
Patch (12.35 KB, patch)
2011-03-02 20:03 PST, John Bauman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bauman 2011-02-28 14:18:11 PST
The premultipliedAlpha context attribute is ignored, and all webgl contexts are essentially treated as having premultiplied alpha. This affects Chromium's layer compositor, drawImage, and toDataURL. I have a patch that should fix this.
Comment 1 John Bauman 2011-02-28 14:28:10 PST
Created attachment 84121 [details]
Patch
Comment 2 John Bauman 2011-02-28 14:43:03 PST
Created attachment 84126 [details]
Patch
Comment 3 Kenneth Russell 2011-02-28 15:47:56 PST
I've added this information to the associated Chromium bug, but please take a look at Gregg's new premultiply-alpha test in the WebGL conformance suite. We should enhance it if necessary to cover the cases you're working on fixing, and pull a copy into the WebKit layout tests. We can comment out sections that aren't working yet and incrementally fix them with multiple bugs if needed.
Comment 4 John Bauman 2011-02-28 16:03:04 PST
Created attachment 84143 [details]
Patch
Comment 5 John Bauman 2011-02-28 16:04:42 PST
That change removes the drawImage part of the LayoutTest, as it could easily be tested in a Khronos test. All of the layer-compositing changes remain.
Comment 6 Kenneth Russell 2011-02-28 16:42:10 PST
Comment on attachment 84143 [details]
Patch

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

The code changes look good. Some more work is needed on the test case.

> LayoutTests/ChangeLog:10
> +        * compositing/webgl/webgl-nonpremultiplied-blend.html: Added.

You'll need to incorporate expected results for this new test. Chromium's results for the existing compositing/webgl/ tests seem to be under LayoutTests in platform/chromium-gpu-{win,mac,linux}/compositing/webgl/ . Despite the fact that the existing tests in compositing/webgl/ have text on the web page, that makes the pixel results non-portable, so I would recommend putting the text in an HTML comment instead. You may then be able to share the pixel results across platforms and put them under platform/chromium-gpu/ .

Because you aren't fixing the Mac code path in this patch, you'll need to add your new test to the Skipped file in LayoutTests/platform/mac/ . I don't think the other ports are running WebGL layout tests by default right now. You'll also need to add it to that in LayoutTests/platform/mac-wk2/ to keep the bots green at http://build.webkit.org/console .

See http://www.chromium.org/developers/testing/webkit-layout-tests for instructions on building DumpRenderTree out of your Chromium tree and running layout tests. You will likely need to pass the arguments "--debug --platform chromium-gpu" to run_webkit_tests.sh.

> LayoutTests/compositing/webgl/webgl-nonpremultiplied-blend.html:20
> +      .compare {
> +        margin: 20px;
> +        width: 200px;
> +        height: 200px;
> +        background-color: rgba(0, 0, 128, 0.5);
> +      }

It looks like the "compare" class is left over and should be taken out.

> LayoutTests/compositing/webgl/webgl-nonpremultiplied-blend.html:62
> +    <div class="compare"></div>

This should be taken out.
Comment 7 Kenneth Russell 2011-02-28 16:44:08 PST
Also, when adding your test to the Skipped list for the mac platform, you should file a new bug to track proper implementation of the premultipliedAlpha flag on that code path.
Comment 8 John Bauman 2011-03-01 14:18:10 PST
Created attachment 84292 [details]
Patch
Comment 9 John Bauman 2011-03-01 14:19:57 PST
Note: This needs the chromium patch in order for the layout test to work properly.
Comment 10 James Robinson 2011-03-01 14:20:58 PST
What does this need on the chromium side?  Can the chromium patch land first?

Preferably we wouldn't land a WebKit patch until it would pass.
Comment 11 Kenneth Russell 2011-03-01 14:22:15 PST
Comment on attachment 84292 [details]
Patch

Please file a bug about the fact that premultipliedAlpha=false isn't implemented on Mac WebKit yet, and reference it in LayoutTests/platform/mac/Skipped. Looks perfect otherwise.
Comment 12 John Bauman 2011-03-01 14:23:26 PST
It needs a patch to make sure that DumpRenderTree's GL implementation doesn't disable the premultipliedAlpha flag. The patch to do that has been reviewed, but not committed yet.
Comment 13 John Bauman 2011-03-02 20:03:50 PST
Created attachment 84514 [details]
Patch
Comment 14 John Bauman 2011-03-03 13:42:31 PST
Ok, the fix for DumpRenderTree is in chromium, so this should be ready.
Comment 15 Kenneth Russell 2011-03-04 16:52:22 PST
Comment on attachment 84514 [details]
Patch

Looks good. r=me
Comment 16 WebKit Commit Bot 2011-03-04 20:27:29 PST
Comment on attachment 84514 [details]
Patch

Clearing flags on attachment: 84514

Committed r80404: <http://trac.webkit.org/changeset/80404>
Comment 17 WebKit Commit Bot 2011-03-04 20:27:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Mihai Parparita 2011-03-07 13:25:02 PST
(In reply to comment #11)
> (From update of attachment 84292 [details])
> Please file a bug about the fact that premultipliedAlpha=false isn't implemented on Mac WebKit yet, and reference it in LayoutTests/platform/mac/Skipped. Looks perfect otherwise.

Does this apply to chromium-mac too? The test is failing there:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=compositing%2Fwebgl%2Fwebgl-nonpremultiplied-blend.html&group=%40ToT%20GPU%20Mesa%20-%20chromium.org

(chromium-mac does not use the mac Skipped file)

Also, there's a failure on on the Windows debug bot only that you may want to look into.
Comment 19 John Bauman 2011-03-07 13:32:44 PST
I'm not sure why the render-tree is different; some pixel-precision issues, I guess. Also, I'm curious why a win debug CPU machine is running this test. 

As for the pixel results do you know whether they're using accelerated compositing on those macs? I'm pretty sure that non-accelerated compositing is still broken for the moment.
Comment 20 Mihai Parparita 2011-03-07 13:34:34 PST
(In reply to comment #19)
> I'm not sure why the render-tree is different; some pixel-precision issues, I guess. Also, I'm curious why a win debug CPU machine is running this test. 

That's a Windows GPU bot (running with Mesa).
 
> As for the pixel results do you know whether they're using accelerated compositing on those macs? I'm pretty sure that non-accelerated compositing is still broken for the moment.

It should be accelerated compositing (also with Mesa).
Comment 21 Kenneth Russell 2011-03-07 14:05:11 PST
(In reply to comment #18)
> (In reply to comment #11)
> > (From update of attachment 84292 [details] [details])
> > Please file a bug about the fact that premultipliedAlpha=false isn't implemented on Mac WebKit yet, and reference it in LayoutTests/platform/mac/Skipped. Looks perfect otherwise.
> 
> Does this apply to chromium-mac too? The test is failing there:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=compositing%2Fwebgl%2Fwebgl-nonpremultiplied-blend.html&group=%40ToT%20GPU%20Mesa%20-%20chromium.org
> 
> (chromium-mac does not use the mac Skipped file)

The exclusion doesn't apply to Chromium's Mac port; it's Core Animation-specific and so only applies to Safari.

The various failures need to be investigated. John, could you please file a new bug tracking them?