Bug 111661

Summary: [Chromium] Fix byte ordering bugs reading back WebGL canvases' content on Android
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, skyostil, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Kenneth Russell 2013-03-06 18:49:42 PST
Chrome on Android is failing a few WebGL conformance tests (canvas/canvas-test.html, context/context-attributes-alpha-depth-stencil-antialias.html, context/premultiplyalpha-test.html, textures/gl-pixelstorei.html) because of incorrect swizzling of color channels. The basic problem is that the contract of WebGraphicsContext3D::readBackFrameBuffer is not well enough defined, causing problems when drawing a WebGL-rendered canvas into another, and when constructing an ImageData from a WebGL-rendered canvas.

One half of the fix can be done in WebKit, but the other half must be done in Chromium.
Comment 1 Kenneth Russell 2013-03-06 18:56:44 PST
The associated Chromium bug is https://code.google.com/p/chromium/issues/detail?id=176029 and that patch will be landed separately. While the bug won't be completely fixed until both land, the intermediate state is no worse than the current one.
Comment 2 Kenneth Russell 2013-03-06 18:59:33 PST
Created attachment 191888 [details]
Patch
Comment 3 WebKit Review Bot 2013-03-06 19:00:42 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 James Robinson 2013-03-06 19:26:15 PST
Comment on attachment 191888 [details]
Patch

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

> Source/WebCore/platform/chromium/support/GraphicsContext3DChromium.cpp:533
> +#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT

this is the same check as https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=WebViewImpl.cpp&sq=package:chromium&type=cs&l=1885, although it's written the other way 'round.  Any chance of getting something clearer?
Comment 5 Kenneth Russell 2013-03-06 19:40:25 PST
(In reply to comment #4)
> (From update of attachment 191888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191888&action=review
> 
> > Source/WebCore/platform/chromium/support/GraphicsContext3DChromium.cpp:533
> > +#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
> 
> this is the same check as https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=WebViewImpl.cpp&sq=package:chromium&type=cs&l=1885, although it's written the other way 'round.  Any chance of getting something clearer?

Argh, sorry, I would have used the same #if test at least.

I could define a macro like SKIA_USING_RGBA_BYTE_ORDERING in one of the headers in Source/Platform/chromium/public/ which would do this check. It would (currently) be used in WebViewImpl.cpp, webgraphicscontext3d_command_buffer_impl.cc, and GraphicsContext3DChromium.cpp. Where would you say would be a good place to put that macro and what would you suggest it be called? Note that the header file would have to include SkTypes.h.
Comment 6 WebKit Review Bot 2013-03-06 19:48:35 PST
Comment on attachment 191888 [details]
Patch

Clearing flags on attachment: 191888

Committed r145027: <http://trac.webkit.org/changeset/145027>
Comment 7 WebKit Review Bot 2013-03-06 19:48:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 James Robinson 2013-03-06 20:10:26 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 191888 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191888&action=review
> > 
> > > Source/WebCore/platform/chromium/support/GraphicsContext3DChromium.cpp:533
> > > +#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
> > 
> > this is the same check as https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=WebViewImpl.cpp&sq=package:chromium&type=cs&l=1885, although it's written the other way 'round.  Any chance of getting something clearer?
> 
> Argh, sorry, I would have used the same #if test at least.

It's no biggie.

> 
> I could define a macro like SKIA_USING_RGBA_BYTE_ORDERING in one of the headers in Source/Platform/chromium/public/ which would do this check. It would (currently) be used in WebViewImpl.cpp, webgraphicscontext3d_command_buffer_impl.cc, and GraphicsContext3DChromium.cpp. Where would you say would be a good place to put that macro and what would you suggest it be called? Note that the header file would have to include SkTypes.h.

Could we get a nicer-named macro in skia, perhaps?
Comment 9 Kenneth Russell 2013-03-07 14:51:04 PST
(In reply to comment #8)
> Could we get a nicer-named macro in skia, perhaps?

I'll ask the Skia team about this.