650720 - [HTML5] canvas.toDataURL("image/jpeg") should composite onto black

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[noel gordon]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/534.29 (KHTML, like Gecko) Chrome/12.0.733.0 Safari/534.29
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

WhatWG Canvas.toDataURL spec: 
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-canvas-todataurl

Corresponding test in Philip Taylors Canvas test suite:
http://philip.html5.org/tests/canvas/suite/tests/toDataURL.jpeg.alpha.html








Reproducible: Always

Steps to Reproduce:
1. Run the test above.

Actual Results:  
FAIL

Expected Results:  
PASS

For image types that do not support an alpha channel, the image must be
composited onto a solid black background using the source-over operator,
and the resulting image must be the one used to create the data: URL.

See also https://bugs.webkit.org/attachment.cgi?id=76855

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html

Comment 2

[Brian O'Keefe [:bokeefe]]

Attachment: Fix the JPEG and BMP encoders

This patch changes the JPEG and BMP encoders to discard the alpha channel information from the HOSTARGB case, instead of un-premultiplying it, so they match what Webkit does (per the attachment in comment 0).

Comment 3

[noel gordon]

Thank you Brian.  "canvas uses premultiplied alpha".  I agree in general, but one thing that came to mind was canvas-3d (webgl).  There I can set

   <canvas>.getContext('3d').premultipliedAlpha = false;

Does Firefox use premultiplied alpha on the canvas in that case?

Comment 4

[Brian O'Keefe [:bokeefe]]

(In reply to noel gordon from comment #3)
> one thing that came to mind was canvas-3d (webgl).  There I can set
> 
>    <canvas>.getContext('3d').premultipliedAlpha = false;
> 
> Does Firefox use premultiplied alpha on the canvas in that case?

Based on some comparisons of the WebGL premultiplied alpha test [1], and if I'm understanding it right:
with premultipliedAlpha = true, the canvas stores premultiplied alpha.
with premultipliedAlpha = false, the canvas stores unpremultiplied alpha.

It's also worth noting, that calling toDataUrl on the canvas causes it to be premultiplied before it gets to the encoder. The PNG decoder will un-premultiply it again.

With the patch, the test at [1] has slightly different results:
Before the patch, the image/jpeg premultiplied case passes, and the image/jpeg not premultiplied case fails.
After the patch, the image/jpeg premultiplied case fails, but the image/jpeg not premultiplied case passes.
None of the other cases are affected (not that they're passing right now).

[1] https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/premultiplyalpha-test.html

Comment 5

[Masatoshi Kimura [:emk]]

Is it correct to use pre-multiplied alpha for 32-bit BMP? At least our decoder doesn't.

Comment 6

[Masatoshi Kimura [:emk]]

My bad. Our BMP decoder doesn't pre-multiply the alpha. That is, it treat the BMP alpha channel as pre-multiplied.

Comment 7

[noel gordon]

(In reply to Brian O'Keefe from comment #4)
> > 
> >    <canvas>.getContext('3d').premultipliedAlpha = false;
> > 
> > Does Firefox use premultiplied alpha on the canvas in that case?
> 
> Based on some comparisons of the WebGL premultiplied alpha test [1], and if
> I'm understanding it right:
> with premultipliedAlpha = true, the canvas stores premultiplied alpha.
> with premultipliedAlpha = false, the canvas stores unpremultiplied alpha.

Correct.  And if the encoding systems do not correct for that premultipliedAlpha state, the test at [1] complains, as you noted.  Best to file a new bug about that, right?

For the current bug, can I assume that the test
http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html
now passes with your change?  If so, we're good here.

I'm curious about the BMP changes though.  If the format supports alpha, viz.,
toDataURL('image/bmp', '-moz-parse-options:bpp=32'), then I'd consider sending unpremultiplied RGBA to the BMP encoder.  If the format does not support alpha, this being the toDataURL('image/bmp') case, I'd consider sending premultiplied RGB to the BMP encoder to effect a composite source-over black.  I'd also compare with the results produced by IE10 to help decide the matter.

Comment 8

[noel gordon]

Attachment: test-case

<canvas>.toDataURL('image/bmp') cross-browser test-case.

Comment 9

[noel gordon]

My results for the image/bmp test-case are:
 
 IE9 9.0.2 win     : outputs image/png
 Firefox 7.0.1 win : outputs image/png
 Chrome 15 win     : outputs image/png
 Safari 5.0.5 win  : outputs image/png

Seems image/bmp is not exposed to web content, so there is no web compat story here. In that case, disregard my earlier comments about BMP.

Comment 10

[Brian O'Keefe [:bokeefe]]

> Best to file a new bug about that, right?

Filed Bug 696569 for that.

> For the current bug, can I assume that the test
> http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html
> now passes with your change?  If so, we're good here.

Yes, that test is passing now.

> I'm curious about the BMP changes though.

Yeah, my first thought was to store postmultiplied alpha in the 32bpp case, but that wasn't passing the test, so I stored premultiplied alpha and kept the alpha channel around.

> I'd also compare with the results produced by IE10 to help decide the matter.

I went and installed Windows 8 so I could run IE10, so I'll post that output this afternoon, for completeness.

Comment 11

[Brian O'Keefe [:bokeefe]]

> I went and installed Windows 8 so I could run IE10, so I'll post that output
> this afternoon, for completeness.

IE10 10.0.8102.0 Win8 : outputs image/png

So, IE10 doesn't support image/bmp either.

Comment 12

[Joe Drew (not getting mail)]

(In reply to Brian O'Keefe from comment #10)
> > Best to file a new bug about that, right?
> 
> Filed Bug 696569 for that.

Does that bug account for the testcase changes in comment 4?

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 567867 [details] [diff] [review]
Fix the JPEG and BMP encoders

Review of attachment 567867 [details] [diff] [review]:
-----------------------------------------------------------------

This is simply lovely. Sorry it took a couple of weeks to get the review; I was on vacation.

::: content/canvas/test/test_toDataURL_alpha.html
@@ +178,5 @@
> +{
> +    for (var t in finishedTests) {
> +        if (!finishedTests[t]) {
> +            setTimeout(checkFinished, 500);
> +            return

Add a semicolon here.

Comment 14

[Brian O'Keefe [:bokeefe]]

Attachment: Fix the JPEG and BMP encoders (r=roc, joedrew)

(In reply to Joe Drew (:JOEDREW!) (Unburying inbox) from comment #13)
> Sorry it took a couple of weeks to get the review; I
> was on vacation.

No worries. Thanks for the review.


> ::: content/canvas/test/test_toDataURL_alpha.html
> @@ +178,5 @@
> > +{
> > +    for (var t in finishedTests) {
> > +        if (!finishedTests[t]) {
> > +            setTimeout(checkFinished, 500);
> > +            return
> 
> Add a semicolon here.

Ah, javascript. Fixed. Carried forward r+.

Comment 15

[Brian O'Keefe [:bokeefe]]

Comment on attachment 571949 [details] [diff] [review]
Fix the JPEG and BMP encoders (r=roc, joedrew)

As an FYI, I don't have commit access, but I just noticed that I'm also not able to add [checkin-needed] to the whiteboard.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8e79374481ff

Comment 17

[Marco Bonardo [:mak]]

backed out for xpcshell-tests failure

https://tbpl.mozilla.org/php/getParsedLog.php?id=7307646&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_imgtools.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_imgtools.js | 1078 == 1081 - See following stack:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | FAILED in test #2 -- test encoding a scaled JPEG: 2147500036

Comment 18

[Brian O'Keefe [:bokeefe]]

Attachment: Fix the JPEG and BMP encoders (with xpcshell-tests fix)

I overlooked this xpcshell test earlier. I fixed it by regenerating the reference images it was using for the JPEG encoding sections. They differ from the originals only by a few pixels at the edges of the transparent area in the PNG version.

Comment 19

[Joe Drew (not getting mail)]

Comment on attachment 573248 [details] [diff] [review]
Fix the JPEG and BMP encoders (with xpcshell-tests fix)

Review of attachment 573248 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if it passes try

Comment 20

[Brian O'Keefe [:bokeefe]]

(In reply to Joe Drew (:JOEDREW!) from comment #19)
> r=me if it passes try

I'm not able to push to try, but xpcshell-tests passes locally. The other two failures are intermittent, so I think everything is fixed.

Unless someone else wants to push to try for me?

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 22

[noel gordon]

> > I'm curious about the BMP changes though.
> 
> Yeah, my first thought was to store postmultiplied alpha in the 32bpp case,
> but that wasn't passing the test, so I stored premultiplied alpha and kept
> the alpha channel around.

Your first thought seems sensible to me.  Maybe your test is not quite right,
or I don't understand how the BMP decoders work.

So I'd invoke an idempotent argument: fill a canvas with rgba(0,64,0,0.5) and
repeat these steps:

   1. dataURL = canvas.toDataURL('image/bmp', '-moz-parse-options:bpp=32')
   2. create a new Image() and set its img.src = dataURL
   3. load the image into a new canvas, visually inspect
   4. goto 1 with your new canvas

Repeat ~6 times, and the image on the canvas should look the same, neglecting
pixel rounding effects.  If alpha premultiplication is not properly accounted
for (comment #5), the canvas pixels will tend to rgba(0,0,0,0.5) :/

Comment 23

[Brian O'Keefe [:bokeefe]]

Attachment: Test for 32bpp BMPs with alpha channels

(In reply to noel gordon from comment #22)
> So I'd invoke an idempotent argument: fill a canvas with rgba(0,64,0,0.5) and
> repeat these steps:
> 
>    1. dataURL = canvas.toDataURL('image/bmp', '-moz-parse-options:bpp=32')
>    2. create a new Image() and set its img.src = dataURL
>    3. load the image into a new canvas, visually inspect
>    4. goto 1 with your new canvas
> 
> Repeat ~6 times, and the image on the canvas should look the same, neglecting
> pixel rounding effects.  If alpha premultiplication is not properly accounted
> for (comment #5), the canvas pixels will tend to rgba(0,0,0,0.5) :/

I whipped up a quick test for that. Open the test case, and then click the 'Another Row' button a few times. The first additional row differs from the original image, but after that, they're always identical to the previous row.

I got the same results with an without the patch, except that with the patch the canvases were darker than without.

Comment 24

[Brian O'Keefe [:bokeefe]]

(In reply to Ms2ger from comment #21)
> https://tbpl.mozilla.org/?tree=Try&rev=a4e5d582fa22

Thanks for that :)

Except for Linux64 Debug, which is still running, the rest came back green, so I'll re-add [checkin-needed]

Comment 25

[noel gordon]

(In reply to Brian O'Keefe from comment #23)

> > Repeat ~6 times, and the image on the canvas should look the same, neglecting
> > pixel rounding effects.  If alpha premultiplication is not properly accounted
> > for (comment #5), the canvas pixels will tend to rgba(0,0,0,0.5) :/
> 
> I whipped up a quick test for that. 

You are awesome.

> I got the same results with an without the patch, except that with the patch
> the canvases were darker than without.

If you're happy with the results, I am too. Thanks for checking.

Comment 26

[noel gordon]

s/too/to/ :)

Comment 27

[noel gordon]

Oh you said "darker".  Hmmm, I wonder what that happens.

Comment 28

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8abaa54efb2a

Before pushing, I tweaked the multi-line checkin comment to be a single-line comment describing the change.  I also removed some extraneous spaces on 3 blank lines, in the added file test_toDataURL_alpha.html

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8abaa54efb2a