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

NEW
Unassigned

Status

()

defect
5 years ago
5 years ago

People

(Reporter: jwatt, Unassigned)

Tracking

Trunk
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.)
(Reporter)

Comment 1

5 years ago
(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
(Reporter)

Comment 2

5 years ago
Posted patch patchSplinter Review
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.
(Reporter)

Comment 3

5 years ago
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.
Flags: needinfo?(smichaud)
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Updated

5 years ago
No longer blocks: 950372
Flags: needinfo?(smichaud)
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);
All of these methods go back at least to OS X 10.6.
You need to log in before you can comment on or make changes to this bug.