Bug 156129 - toDataURL image upside down if premultipliedAlpha=false
Summary: toDataURL image upside down if premultipliedAlpha=false
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac OS X 10.11
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: 126404 226254
  Show dependency treegraph
 
Reported: 2016-04-01 22:07 PDT by nkronlage
Modified: 2021-06-30 12:43 PDT (History)
13 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2021-06-29 11:13 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (3.34 KB, patch)
2021-06-29 11:16 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (4.33 KB, patch)
2021-06-30 00:51 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nkronlage 2016-04-01 22:07:18 PDT
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.
Comment 1 Aleksandar Rodic 2019-08-05 07:01:24 PDT
How is it possible that such a simply bug hasn't been fixed in more than 3 years? This is still broken.
Comment 2 Radar WebKit Bug Importer 2019-08-05 08:20:35 PDT
<rdar://problem/53942867>
Comment 3 William Furr 2019-12-05 11:58:42 PST
Is this something that switching to ANGLE will fix?
Comment 4 Kenneth Russell 2019-12-05 13:10:59 PST
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 .
Comment 5 nkronlage 2020-06-02 15:21:11 PDT
I've added a conformance test for this to the WebGL suite: 
https://github.com/KhronosGroup/WebGL/commit/fd4489e57bb17b276dc575d0f8b6681d9c2ebcc8
Comment 6 Kenneth Russell 2020-06-02 17:13:01 PDT
Thanks nkronlage@ for creating the test. Let me block the shipment of WebGL 2.0 in Safari on fixing this as well.
Comment 7 Kenneth Russell 2021-05-17 17:41:54 PDT
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.
Comment 8 Kimmo Kinnunen 2021-06-29 11:13:43 PDT
Created attachment 432502 [details]
Patch
Comment 9 Kimmo Kinnunen 2021-06-29 11:16:04 PDT
Created attachment 432503 [details]
Patch
Comment 10 Said Abou-Hallawa 2021-06-29 12:09:41 PDT
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.
Comment 11 Kenneth Russell 2021-06-29 13:22:43 PDT
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.
Comment 12 Kimmo Kinnunen 2021-06-30 00:51:38 PDT
Created attachment 432572 [details]
Patch for landing
Comment 13 Kenneth Russell 2021-06-30 11:10:36 PDT
Comment on attachment 432572 [details]
Patch for landing

Still looks good!
Comment 14 EWS 2021-06-30 11:29:09 PDT
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].