Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (skia)

[Kenneth Russell]

If an RGBA PNG image is uploaded via texImage2D or texSubImage2D and false is passed for the premultiplyAlpha argument, the result is currently lossy; the alpha channel is multiplied in to the color channels by the browser and then divided out later. The WebGL specification requires that this upload path be lossless so that images which do not contain color and alpha information (for example, using the RGBA channels as arbitrary data inputs to a shader) are not destroyed during the upload process.

[Zhenyao Mo]

break the bug into two: skia and cg

cg is here: https://bugs.webkit.org/show_bug.cgi?id=44566

[Zhenyao Mo]

Created attachment 65347 [details]
patch

Test is in sync with khronos (with certain parts commented out).

[WebKit Review Bot]

Attachment 65347 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:87:  Missing spaces around <  [whitespace/operators] [3]
WebCore/platform/image-decoders/ImageDecoder.h:158:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

Created attachment 65349 [details]
revised patch: fixing style issues

[Zhenyao Mo]

Created attachment 65433 [details]
revised patch: an attempt to fix the Qt port

[Zhenyao Mo]

Eric, is there anyway we can get the output from Qt?  I see it's failing, but have no idea why.  If I can get the output, it will be much easier for me to fix the issue.

[Eric Seidel (no email)]

I'm not sure why Qt failed to post results here.

[Kenneth Russell]

Comment on attachment 65433 [details]
revised patch: an attempt to fix the Qt port

Generally looks good. A few comments; please address before committing.

WebCore/platform/graphics/cg/ImageSourceCG.cpp:68
 +      , m_premultiplyAlpha(premultiplyAlpha)
Should add a FIXME indicating this isn't obeyed yet.

WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp: 
 +      if (skiaConfig != SkBitmap::kARGB_8888_Config)
I think it would be a good idea to retain some sort of this test of the configuration rather than just the assertion about pixels->rowBytes() below.

WebCore/platform/image-decoders/ImageDecoder.h:158
 +              if (!a && m_premultiplyAlpha)
It may be clearer here and below if m_premultiplyAlpha is tested first; i.e., "if (m_premultiplyAlpha && !a)".

WebCore/platform/image-decoders/ImageDecoder.h:197
 +                                // Whether to premultiply alpha to R, G, B channels;
alpha to -> alpha into

LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:1
 +  CONSOLE MESSAGE: line 260: ReferenceError: Can't find variable: notifyFinishedToHarness
This looks bad. Please investigate why this is being reported and fix.


LayoutTests/platform/mac/Skipped:297
 +  fast/canvas/webgl/gl-teximage.html
We should consider either adding this to mac-snowleopard or moving the skipped entries from that directory to this one. Doesn't need to be fixed right now.

[Zhenyao Mo]

Committed r66039: <http://trac.webkit.org/changeset/66039>

[Zhenyao Mo]

The landed patch addressed all the comments here except the last one.

(In reply to comment #8)
> (From update of attachment 65433 [details])
> Generally looks good. A few comments; please address before committing.
> 
> WebCore/platform/graphics/cg/ImageSourceCG.cpp:68
>  +      , m_premultiplyAlpha(premultiplyAlpha)
> Should add a FIXME indicating this isn't obeyed yet.
> 
> WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp: 
>  +      if (skiaConfig != SkBitmap::kARGB_8888_Config)
> I think it would be a good idea to retain some sort of this test of the configuration rather than just the assertion about pixels->rowBytes() below.
> 
> WebCore/platform/image-decoders/ImageDecoder.h:158
>  +              if (!a && m_premultiplyAlpha)
> It may be clearer here and below if m_premultiplyAlpha is tested first; i.e., "if (m_premultiplyAlpha && !a)".
> 
> WebCore/platform/image-decoders/ImageDecoder.h:197
>  +                                // Whether to premultiply alpha to R, G, B channels;
> alpha to -> alpha into
> 
> LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt:1
>  +  CONSOLE MESSAGE: line 260: ReferenceError: Can't find variable: notifyFinishedToHarness
> This looks bad. Please investigate why this is being reported and fix.
> 
> 
> LayoutTests/platform/mac/Skipped:297
>  +  fast/canvas/webgl/gl-teximage.html
> We should consider either adding this to mac-snowleopard or moving the skipped entries from that directory to this one. Doesn't need to be fixed right now.

[Zhenyao Mo]

Need to deal with the case where image->data() == null (in texImage2D with HTMLCanvasElement input)

[Kenneth Russell]

(In reply to comment #11)
> Need to deal with the case where image->data() == null (in texImage2D with HTMLCanvasElement input)

Is it possible to refer to the Canvas's ImageBuffer rather than an Image for this upload case?

[Zhenyao Mo]

(In reply to comment #12)
> (In reply to comment #11)
> > Need to deal with the case where image->data() == null (in texImage2D with HTMLCanvasElement input)
> 
> Is it possible to refer to the Canvas's ImageBuffer rather than an Image for this upload case?

I'll have a look.

[Zhenyao Mo]

Created attachment 65659 [details]
patch: dealing case of texImage2D using HTMLCanvasElement as input

[Zhenyao Mo]

Created attachment 65660 [details]
revised patch: fix a tiny logic bug

[Kenneth Russell]

Comment on attachment 65660 [details]
revised patch: fix a tiny logic bug

Looks good to me.

[Zhenyao Mo]

Committed r66288: <http://trac.webkit.org/changeset/66288>