Bug 156129 – toDataURL image upside down if premultipliedAlpha=false

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

Calling .toDataURL() on a WebGL canvas that has premultipliedAlpha set to false results in an image that upside down.

See https://jsfiddle.net/oagpgo80/ for an example.  It renders a triangle in a canvas (left) and calls toDataURL and puts the result in an <img> on the right.

Expected: 
Two triangles that look the same

Actual:
The right triangle (the image created from toDataURL) is upside down.

[Aleksandar Rodic]

How is it possible that such a simply bug hasn't been fixed in more than 3 years? This is still broken.

[Radar WebKit Bug Importer]

<rdar://problem/53942867>

[William Furr]

Is this something that switching to ANGLE will fix?

[Kenneth Russell]

It probably won't automatically fix it - I suspect this code path uses glReadPixels to read back the framebuffer, and that the bug is in common CPU-side code. I'm surprised there isn't a WebGL conformance test to catch this. Someone should add one to https://github.com/KhronosGroup/WebGL .

I've added a conformance test for this to the WebGL suite: 
https://github.com/KhronosGroup/WebGL/commit/fd4489e57bb17b276dc575d0f8b6681d9c2ebcc8

[Kenneth Russell]

Thanks nkronlage@ for creating the test. Let me block the shipment of WebGL 2.0 in Safari on fixing this as well.

[Kenneth Russell]

For the record, the modified test:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/canvas/to-data-url-test.html

still fails in current Safari Technology Preview.

[Kimmo Kinnunen]

Created attachment 432502 [details]
Patch

[Kimmo Kinnunen]

Created attachment 432503 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 432503 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        Fixes webgl/1.0.x/conformance/canvas/to-data-url-test.html
> +        (which is not run at the moment).

Should this patch unskip this test from the TextExpectations?

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:239
> +        ASSERT(results->format().pixelFormat == PixelFormat::RGBA8 || results->format().pixelFormat == PixelFormat::BGRA8);

GraphicsContextGLOpenGL::readPixelsForPaintResults() creates the PixelBuffer with PixelFormat::RGBA8 and it calls gl::ReadnPixelsRobustANGLE() with GL_RGBA. So why do we assert "|| pixelFormat == PixelFormat::BGRA8"?

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:243
> +        uint8_t* src = results->data().data();
> +        uint8_t* dst = src + (size.height() - 1) * rowStride;

What do the "src" and "dst" mean here? We are swapping rows of pixels. I would expect names like: "up" and "down", "first" and "last" "begin" and "end" or something similar.

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:248
> +            memcpy(temp.get(), dst, rowStride);
> +            memcpy(dst, src, rowStride);
> +            memcpy(src, temp.get(), rowStride);

Can't we use vImage to accelerate this operation? I read this comment in vImage.h:

 *    Sometimes you may want to flip an image vertically. In vImage, this can be done cheaply by adjusting the pointer
 *    to point to the last scanline of the image and setting rowBytes negative, for either the source or destination
 *    image:

I am not sure if vImage can do this in place as you did or not.

[Kenneth Russell]

Comment on attachment 432503 [details]
Patch

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

Please address Said's remaining code review comments, but this looks fine to me overall. Would be great to get this longstanding bug fixed. r+

>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:239
>> +        ASSERT(results->format().pixelFormat == PixelFormat::RGBA8 || results->format().pixelFormat == PixelFormat::BGRA8);
> 
> GraphicsContextGLOpenGL::readPixelsForPaintResults() creates the PixelBuffer with PixelFormat::RGBA8 and it calls gl::ReadnPixelsRobustANGLE() with GL_RGBA. So why do we assert "|| pixelFormat == PixelFormat::BGRA8"?

I assume in order to better enforce constraints between unrelated code, especially if the called code changes in the future - for example, to support higher bit depth back buffers in WebGL, which is a feature being actively developed in the ColorWeb CG.

>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:248
>> +            memcpy(src, temp.get(), rowStride);
> 
> Can't we use vImage to accelerate this operation? I read this comment in vImage.h:
> 
>  *    Sometimes you may want to flip an image vertically. In vImage, this can be done cheaply by adjusting the pointer
>  *    to point to the last scanline of the image and setting rowBytes negative, for either the source or destination
>  *    image:
> 
> I am not sure if vImage can do this in place as you did or not.

This code isn't Cocoa-specific so writing the loop manually seems most portable.

[Kimmo Kinnunen]

Created attachment 432572 [details]
Patch for landing

[Kenneth Russell]

Comment on attachment 432572 [details]
Patch for landing

Still looks good!

[EWS]

Committed r279424 (239285@main): <https://commits.webkit.org/239285@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432572 [details].