Bug 87310 – WebKit incorrectly clears the alpha channel on readPixels, even for Framebuffers

• Component: WebGL
• Status: RESOLVED FIXED

[Gregg Tavares]

In Source/WebCore/html/canvas/WebGLRenderingContext.cpp there's a check in WebGLRenderingContext::readPixels


#if OS(DARWIN)
    // FIXME: remove this section when GL driver bug on Mac is fixed, i.e.,
    // when alpha is off, readPixels should set alpha to 255 instead of 0.
    if (!m_context->getContextAttributes().alpha) {
        unsigned char* pixels = reinterpret_cast<unsigned char*>(data);
        for (GC3Dsizei iy = 0; iy < height; ++iy) {
            for (GC3Dsizei ix = 0; ix < width; ++ix) {
                pixels[3] = 255;
                pixels += 4;
            }
            pixels += padding;
        }
    }
#endif

This check should not be happening if a framebuffer is currently bound, only if the backbuffer is currently bound. Possibly you'd like to check the format of the current framebuffer.

The linked test above tests this

[Zhenyao Mo]

Created attachment 144935 [details]
Patch

[Zhenyao Mo]

The test is in sync with khronos side.  Please review.

[Kenneth Russell]

Comment on attachment 144935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144935&action=review

Looks good. r=me

> LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html:170
> +      shouldBe("pixel", "[127, 127, 127, 127]");

I could imagine this test being flaky. Should it have a one-pixel value tolerance?

[Zhenyao Mo]

Comment on attachment 144935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144935&action=review

>> LayoutTests/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html:170
>> +      shouldBe("pixel", "[127, 127, 127, 127]");
> 
> I could imagine this test being flaky. Should it have a one-pixel value tolerance?

Agree.  I'll update the khronos side and resync.

[Zhenyao Mo]

Committed r119014: <http://trac.webkit.org/changeset/119014>