When we call readPixels from the WebGLRenderingContext note that we spit out an error if GL_BGRA format is used. However, we for some reason use this format when reading the rendering the results for the purposes of either reading the rendering results into an ImageData or a canvas. This color format seems to fail specifically fail on NVIDIA cards and in particular the alpha channel can be read incorrectly when the alpha is off for the context. However, I will make a blanket change here for all devices and context attribute settings for alpha to be consistent with WebGLRenderingContext::readPixels which does the following: switch (format) { case GraphicsContext3D::ALPHA: case GraphicsContext3D::RGB: case GraphicsContext3D::RGBA: break; default: synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "invalid format"); return; }
Created attachment 239980 [details] patch
Comment on attachment 239980 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=239980&action=review I think this change will break whatever port uses the OpenGLES backend (EFL maybe?). > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:254 > + ::glReadPixels(0, 0, m_currentWidth, m_currentHeight, GL_RGBA, GL_UNSIGNED_BYTE, pixels); I think this was here to support OpenGLES clients. See GraphicsContext3DOPenGLES.cpp, which looks like GraphicsContext3D::readPixelsAndConvertToBGRAIfNecessary, except that it swaps the R and B pixels.
*** Bug 137752 has been marked as a duplicate of this bug. ***
Chromium queries in RGBA, not BGRA and swaps if necessary, which is essentially what my patch will do, except only for NVIDIA cards where this is actually an issue.
Created attachment 240001 [details] patch
Comment on attachment 240001 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240001&action=review > Source/WebCore/ChangeLog:13 > + On an nvidia machine, when the context has alpha turned off, call glReadPixels with RGBA format and then convert to RGBA. NVIDIA
Comment on attachment 240001 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240001&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:73 > + int totalBytes = width * height * 4; > + for (int i = 0; i < totalBytes; i += 4) > + std::swap(pixels[i], pixels[i + 2]); I wonder if something like vImagePermuteChannels_ARGB8888 would do this faster.
Comment on attachment 240001 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240001&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:72 > + ::glReadPixels(x, y, width, height, GL_RGBA, GL_UNSIGNED_INT_8_8_8_8_REV, pixels); > + int totalBytes = width * height * 4; > + for (int i = 0; i < totalBytes; i += 4) e.g. vImage_Buffer src; src.height = height; src.width = width; src.rowBytes = srcBytesPerRow; src.data = srcRows; vImage_Buffer dest; dest.height = height; dest.width = width; dest.rowBytes = destBytesPerRow; dest.data = destRows; // Swap pixel channels from BGRA to RGBA. const uint8_t map[4] = { 2, 1, 0, 3 }; vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags);
Comment on attachment 240001 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240001&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:72 >> + for (int i = 0; i < totalBytes; i += 4) > > e.g. > vImage_Buffer src; > src.height = height; > src.width = width; > src.rowBytes = srcBytesPerRow; > src.data = srcRows; > > vImage_Buffer dest; > dest.height = height; > dest.width = width; > dest.rowBytes = destBytesPerRow; > dest.data = destRows; > > // Swap pixel channels from BGRA to RGBA. > const uint8_t map[4] = { 2, 1, 0, 3 }; > vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags); Except you want the other way around :)
I'll try running instruments on a test page with a) no swapping at all b) the std::swap c) the vImagePermuteChannels_ARGB8888
a) No reason to run this test really...all readPixelsAndConvertToBGRAIfNecessary does on mac is call glReadPixels, so the times are the same. b) Running Time Self Symbol Name 5012.0ms 3.8% 445.0 readPixelsAndConvertToBGRAIfNecessary 4562.0ms 3.5% 1.0 glReadPixels c) Running Time Self Symbol Name 5006.0ms 4.8% 1.0 readPixelsAndConvertToBGRAIfNecessary 4833.0ms 4.7% 0.0 glReadPixels So it looks like the vaccelerator stuff is definitely faster, though I guess we already knew that to be the case.
Created attachment 240053 [details] patch
(In reply to comment #11) > a) No reason to run this test really...all > readPixelsAndConvertToBGRAIfNecessary does on mac is call glReadPixels, so > the times are the same. > > b) > Running Time Self Symbol Name > 5012.0ms 3.8% 445.0 readPixelsAndConvertToBGRAIfNecessary > > 4562.0ms 3.5% 1.0 glReadPixels > > c) > Running Time Self Symbol Name > 5006.0ms 4.8% 1.0 readPixelsAndConvertToBGRAIfNecessary > > 4833.0ms 4.7% 0.0 glReadPixels > > So it looks like the vaccelerator stuff is definitely faster, though I guess > we already knew that to be the case. What we're comparing is the differences between the two function calls (which is the time it takes to swap the 1st and 3rd channels). Using accelerator stuff is about 30% better.
Comment on attachment 240053 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240053&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:89 > + // NVIDIA drivers have a bug where calling readPixels in BGRA can return the wrong values for the alpha channel when the alpha is off for the context. > + if (!m_attrs.alpha && getExtensions()->isNVIDIA()) { > + ::glReadPixels(x, y, width, height, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > + vImage_Buffer src; > + src.height = height; > + src.width = width; > + src.rowBytes = width*4; > + src.data = pixels; > + > + vImage_Buffer dest; > + dest.height = height; > + dest.width = width; > + dest.rowBytes = width*4; > + dest.data = pixels; > + > + // Swap pixel channels from BGRA to RGBA. > + const uint8_t map[4] = { 2, 1, 0, 3 }; > + vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags); Won't you need the USE(ACCELERATE) around this too? Other ports won't work.
Comment on attachment 240053 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=240053&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:72 > + // NVIDIA drivers have a bug where calling readPixels in BGRA can return the wrong values for the alpha channel when the alpha is off for the context. Is this true for all NVIDIA drivers, historically? Is this likely to change? Should we be detecting the wrongness somehow instead of just flipping things around like this? > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:78 > + src.rowBytes = width*4; Spaces around the star, please. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:84 > + dest.rowBytes = width*4; Ditto.
(In reply to comment #15) > Comment on attachment 240053 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240053&action=review > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:72 > > + // NVIDIA drivers have a bug where calling readPixels in BGRA can return the wrong values for the alpha channel when the alpha is off for the context. > > Is this true for all NVIDIA drivers, historically? Is this likely to change? > Should we be detecting the wrongness somehow instead of just flipping things > around like this? We'd then have to make two queries to glReadPixels then which seems unfortunate. I've tested on 3 devices, dating back to the 2009 macbook pro card. > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:78 > > + src.rowBytes = width*4; > > Spaces around the star, please. > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:84 > > + dest.rowBytes = width*4; > > Ditto. Whoops missed those, I'll add those in.
http://trac.webkit.org/changeset/174855 http://trac.webkit.org/changeset/174867