The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

Core
Canvas: 2D
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: noel gordon, Assigned: bokeefe)

Tracking

(Blocks: 2 bugs, {html5})

Trunk
mozilla11
html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk

Updated

6 years ago
Keywords: html5
http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html
(Assignee)

Comment 2

6 years ago
Created attachment 567867 [details] [diff] [review]
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).
Attachment #567867 - Flags: review?(roc)
Attachment #567867 - Flags: review?(joe)
Attachment #567867 - Flags: review?(roc) → review+
(Reporter)

Comment 3

6 years ago
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?
(Assignee)

Comment 4

6 years ago
(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
Is it correct to use pre-multiplied alpha for 32-bit BMP? At least our decoder doesn't.
My bad. Our BMP decoder doesn't pre-multiply the alpha. That is, it treat the BMP alpha channel as pre-multiplied.
(Reporter)

Comment 7

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

Comment 8

6 years ago
Created attachment 568819 [details]
test-case

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

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
> 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.
(Assignee)

Comment 11

6 years ago
> 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.
(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 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.
Attachment #567867 - Flags: review?(joe) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 571949 [details] [diff] [review]
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+.
Attachment #567867 - Attachment is obsolete: true
Attachment #571949 - Flags: review+
Blocks: 696569
(Assignee)

Comment 15

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

Updated

5 years ago
Assignee: nobody → bokeefe
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/8e79374481ff
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla11
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
Target Milestone: mozilla11 → ---
(Assignee)

Comment 18

5 years ago
Created attachment 573248 [details] [diff] [review]
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.
Attachment #571949 - Attachment is obsolete: true
Attachment #573248 - Flags: review?(joe)
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
Attachment #573248 - Flags: review?(joe) → review+
(Assignee)

Comment 20

5 years ago
(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?
https://tbpl.mozilla.org/?tree=Try&rev=a4e5d582fa22
(Reporter)

Comment 22

5 years ago
> > 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) :/
(Assignee)

Comment 23

5 years ago
Created attachment 573632 [details]
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.
(Assignee)

Comment 24

5 years ago
(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]
Keywords: checkin-needed
(Reporter)

Comment 25

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

Comment 26

5 years ago
s/too/to/ :)
(Reporter)

Comment 27

5 years ago
Oh you said "darker".  Hmmm, I wonder what that happens.
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
Keywords: checkin-needed
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/8abaa54efb2a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 713143
Blocks: 723453
You need to log in before you can comment on or make changes to this bug.