Bug 156129

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

<rdar://problem/53942867>

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