Bug 87310 - WebKit incorrectly clears the alpha channel on readPixels, even for Framebuffers
Summary: WebKit incorrectly clears the alpha channel on readPixels, even for Framebuffers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL: https://cvs.khronos.org/svn/repos/reg...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-23 14:07 PDT by Gregg Tavares
Modified: 2012-05-30 18:32 PDT (History)
1 user (show)

See Also:


Attachments
Patch (23.22 KB, patch)
2012-05-30 15:13 PDT, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>