Bug 87310

Summary: WebKit incorrectly clears the alpha channel on readPixels, even for Framebuffers
Product: WebKit Reporter: Gregg Tavares <gman>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: kbr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/context-attributes-alpha-depth-stencil-antialias.html
Attachments:
Description Flags
Patch kbr: review+

Description Gregg Tavares 2012-05-23 14:07:33 PDT
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
Comment 1 Zhenyao Mo 2012-05-30 15:13:45 PDT
Created attachment 144935 [details]
Patch
Comment 2 Zhenyao Mo 2012-05-30 15:14:08 PDT
The test is in sync with khronos side.  Please review.
Comment 3 Kenneth Russell 2012-05-30 17:27:54 PDT
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?
Comment 4 Zhenyao Mo 2012-05-30 17:33:52 PDT
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.
Comment 5 Zhenyao Mo 2012-05-30 18:32:05 PDT
Committed r119014: <http://trac.webkit.org/changeset/119014>