389366 - Canvas's .getImageData is returning premultiplied alpha pixels

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect, P2)

Description

[Justin Dolske [:Dolske]]

Attachment: Testcase, reports R/G/B/A values for a 50% transparent red pixel.

From WHATWG spec:

---
3.14.11.1.10. Pixel manipulation

The getImageData(sx, sy, sw, sh) method must return an ImageData object representing the underlying pixel data for the area of the canvas denoted by the rectangle which has one corner at the (sx, sy) coordinate, and that has width sw and height sh. Pixels outside the canvas must be returned as transparent black. Pixels must be returned as non-premultiplied alpha values.
---

Note the last sentence. Unfortunately, Mozilla seems to be returning pre-multiplied alpha values. I first noticed this with my APNG editor (the encoder wants non-premultiplied pixels).

Note that .putImageData will also need to accept non-premultiplied pixels, as ctx.putImageData(ctx.getImageData()) should be a nop.

The attached testcase shows that a #ff0000 pixel drawn at 50% opacity comes out as 128/0/0/128, tested on Mac, Windows, and Linux. One Linux system was 127/0/0/127. Yay, rounding? :-)

Comment 1

[Philip Taylor]

Attachment: patch

Not sure if I'm doing this properly, but here's a patch. The rounding is done so that transparent white is always (255, 255, 255, a) and putImageData(getImageData()) has no effect.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 275624 [details] [diff] [review]
patch

Yeah, this looks fine.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 275624 [details] [diff] [review]
patch

Whoops, can't do approval 1.9 now

Comment 4

[Damon Sicore (:damons)]

Comment on attachment 275624 [details] [diff] [review]
patch

Setting approval1.9+, targeting for M10, and you are clear to land once the tree is open for M10 (i.e., after tree opens, after beta).

Comment 5

[Damon Sicore (:damons)]

Targeting M10.

Comment 6

[Reed Loden [:reed]]

Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp;
/cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v  <--  nsCanvasRenderingContext2D.cpp
new revision: 1.104; previous revision: 1.103
done

Comment 7

[Reed Loden [:reed]]

I backed this out, as it didn't have M9 approval.
/me frowns at stuart for asking him to check this in

Comment 8

[Stuart Parmenter]

ugh, sorry -- when i asked it had approval-m9+ on it... probably when connor messed up the flag :/

Comment 9

[Jeff Walden [:Waldo]]

Philip: could you take the time to write a testcase for this?  The testcase would be a Mochitest:

http://developer.mozilla.org/en/docs/Mochitest

The "Writing new Mochitest tests" section in that document describes how to generate a skeleton.  From there, just call |SimpleTest.waitForExplicitFinish()| and run the test in an onload handler, calling the following functions to do the actual tests:

  ok(a == b, "a should be equal to b, i.e. a==b");
  is(a, b, "a should be equal to b, i.e. a==b");

Then call |SimpleTest.finish()| when all checks complete.  The test should fail when run in a build without your patch, and it should succeed in one with your patch.

If you have any questions at all about this, either ask them here or jump on irc.mozilla.org and #developers (<http://ircatwork.com/> is probably the least hassle) and ask questions there, ideally at a time that overlaps with 12:00-20:00 PDT on a weekday.  The former's perfectly fine, but the latter will likely work much faster.

Comment 10

[pete]

This patch looks wrong to me:

+            // Convert to non-premultiplied color
+            if (a != 0) {
+                r = (r * 255) / a;
+                g = (g * 255) / a;
+                b = (b * 255) / a;
+            }
+

I think you meant something more like foo / a.

Comment 11

[pete]

Err wait, ignore that. I'm the one that's wrong.

What I got confused was that this these functions aren't exact inverses of each other 50% of the time and I vaguely remember that there was some fancy-pants way to reduce that even further by doing some manipulation of the low order bits. I'll see if I can remember where I found that.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Testcase, v.2

I modified the original test a bit to get putImageData too. Not a mochitest, though, sorry. :)

With this patch on OS X, the test case correctly reports "R = 255, G = 0, B = 0, A = 128" for both canvasses.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 275624 [details] [diff] [review]
patch

Renoming for beta (nom nom nom)...

Has patch, and is extremely low risk. This is needed for compliance with the WHATWG spec, and also helps with the image reftests / APNG encoder I'm working on.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 275624 [details] [diff] [review]
patch

a=endgame drivers for M9

Comment 15

[Reed Loden [:reed]]

Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp;
/cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v  <--  nsCanvasRenderingContext2D.cpp
new revision: 1.106; previous revision: 1.105
done

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Following up on dolske's slackerliness :-P

This really didn't take *that* long to make.  It passes on trunk, but I don't have the time/desire to double-check it fails pre-patch.

Comment 17

[Jeff Walden [:Waldo]]

http://mxr.mozilla.org/mozilla/source/dom/tests/mochitest/bugs/test_bug389366.html

Comment 18

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110104 Minefield/3.0a9pre. I verified using the testcase and the expected results in Comment 12.

Comment 19

[ashley_c_mort]

Any chance this will ever get merged? It would be nice to have it kinda like it's nice to have the option for {premultipliedAlpha: false} on the 'webgl' context.