Summary: | toDataURL image upside down if premultipliedAlpha=false | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | nkronlage | ||||||||
Component: | WebGL | Assignee: | Kimmo Kinnunen <kkinnunen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aleksandar.xyz, dino, ews-watchlist, jdarpinian, justin_fan, kbr, kkinnunen, kondapallykalyan, mail, sabouhallawa, thorton, webkit-bug-importer, wfurr | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.11 | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=227538 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 126404, 226254 | ||||||||||
Attachments: |
|
Description
nkronlage
2016-04-01 22:07:18 PDT
How is it possible that such a simply bug hasn't been fixed in more than 3 years? This is still broken. Is this something that switching to ANGLE will fix? 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 Thanks nkronlage@ for creating the test. Let me block the shipment of WebGL 2.0 in Safari on fixing this as well. 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. Created attachment 432502 [details]
Patch
Created attachment 432503 [details]
Patch
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 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. Created attachment 432572 [details]
Patch for landing
Comment on attachment 432572 [details]
Patch for landing
Still looks good!
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]. |