Bug 111661 – [Chromium] Fix byte ordering bugs reading back WebGL canvases' content on Android

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 191888 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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?

[Kenneth Russell]

(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.

[WebKit Review Bot]

Comment on attachment 191888 [details]
Patch

Clearing flags on attachment: 191888

Committed r145027: <http://trac.webkit.org/changeset/145027>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

(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?

[Kenneth Russell]

(In reply to comment #8)
> Could we get a nicer-named macro in skia, perhaps?

I'll ask the Skia team about this.