995409 - Stop SourceSurfaceCG::GetDataSurface() from dropping the color component values of transparent pixels

Status: NEW

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

SourceSurfaceCG::GetDataSurface() drops the color component values of transparent pixels. With the patch for bug 950372 applied we trip over this bug and fail in:

content/canvas/test/webgl-conformance/test_webgl_conformance_test_suite.html

See:

https://tbpl.mozilla.org/?tree=Try&rev=cc042860ee89

The sub-test that is actually failing is the "check pixels are NOT pre-multiplied" section of:

https://mxr.mozilla.org/mozilla-central/source/content/canvas/test/webgl-conformance/conformance/textures/gl-teximage.html

This section of the sub-test checks texImage2D for a 3x3 fully transparent PNG with the following (rgb) color component values:

   #ff0000  #ffff00  #00ff00
   #808080  #ffffff  #00ffff
   #000000  #ff00ff  #0000ff

The test fails because when it gets the pixel values they are all black. This is because the WebGLContext::TexImage2D<mozilla::dom::HTMLImageElement>() call ends up with a transparent black DataSourceSurface because that DataSourceSurface comes from SourceSurfaceCG::GetDataSurface(), which contains this bug. More specifically a SourceSurfaceCG is returned from the imgContainer->GetFrame() call in nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, ...). On returning to WebGLContext::TexImage2D<mozilla::dom::HTMLImageElement> |res| is passed to SurfaceFromElementResultToImageSurface and GetDataSurface is called on it there.

We are currently (without the GetFrame patch) protected from this because nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, ...) currently passes the DrawTarget created by Factory::CreateDrawTargetForData to GetSourceSurfaceForSurface, and GetSourceSurfaceForSurface just wraps the gfxQuartzImageSurface's buffer in a gfxImageSurface (the aSurface->GetAsImageSurface() call), and then wraps the gfxImageSurface's buffer in a DataSourceSurface (the CreateSourceSurfaceFromData call), before returning that. (So there's currently no pixel copying there, and therefore no loss of pixel data.)

Comment 1

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #0)
> See:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=cc042860ee89

The mochitest-1 failures on Mac.

For my own future reference, it's a bit of a pain to run the test. I did it by running:

./mach mochitest-plain --no-autorun 
content/canvas/test/webgl-conformance/test_webgl_conformance_test_suite.html

then opening a new Tab and loading the test by loading:

http://mochi.test:8888/tests/content/canvas/test/webgl-conformance/conformance/textures/gl-teximage.html

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

mstange suggested using CFDataGetBytes to copy the pixel data from the CGImageRef to the DataSourceSurfaceCG's mCg (instead of drawing it using CGContextDrawImage which loses the color component data for transparent pixels). The one down side of that is that it involves a double copy. I'm not sure if that's an issue or not.

Comment 3

[Jonathan Watt [:jwatt]]

smichaud is going to take a look at my "direct access" in the patch to see if he thinks it's safe to use that to avoid the double copy.

Comment 4

[Jonathan Watt [:jwatt]]

Hmm, of course this is only an issue because SourceSurface is being abused to hold unpremultiplied color. SourceSurface is supposed to always contain premultiplied pixel data, so it isn't actually supposed to be capable of preserving the color components of transparent pixels.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Your patch doesn't stray into any of the danger areas I discovered working on bug 925448 ... as far as I can tell.

And yes, I suspect you'll be just fine using CGAccessSessionGetBytePointer() and company.

Here are declarations of some undocumented methods you may find useful:

typedef struct CGAccessSession *CGAccessSessionRef;

CGAccessSessionRef CGAccessSessionCreate(CGDataProviderRef provider);

const char *CGAccessSessionGetBytePointer(CGAccessSessionRef session);

off_t CGDataProviderGetSize(CGDataProviderRef provider);

void CGAccessSessionRelease(CGAccessSessionRef session);

Comment 6

[Steven Michaud [:smichaud] (Retired)]

All of these methods go back at least to OS X 10.6.