Bug 137793 – Calling glReadPixels with BGRA format on an NVIDIA machine with an opaque context returns the wrong alpha values.

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

[Roger Fong]

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;
    }

[Roger Fong]

Created attachment 239980 [details]
patch

[Brent Fulgham]

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.

[Roger Fong]

*** Bug 137752 has been marked as a duplicate of this bug. ***

[Roger Fong]

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.

[Roger Fong]

Created attachment 240001 [details]
patch

[Dean Jackson]

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

[Dean Jackson]

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.

[Dean Jackson]

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);

[Dean Jackson]

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 :)

[Roger Fong]

I'll try running instruments on a test page with
a) no swapping at all
b) the std::swap
c) the vImagePermuteChannels_ARGB8888

[Roger Fong]

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.

[Roger Fong]

Created attachment 240053 [details]
patch

[Roger Fong]

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

[Dean Jackson]

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.

[Tim Horton]

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.

[Roger Fong]

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

[Roger Fong]

http://trac.webkit.org/changeset/174855
http://trac.webkit.org/changeset/174867