Closed Bug 389366 Opened 14 years ago Closed 14 years ago

Canvas's .getImageData is returning premultiplied alpha pixels

Categories

(Core :: Canvas: 2D, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: Dolske, Assigned: philip)

Details

(Keywords: testcase)

Attachments

(3 files, 1 obsolete file)

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? :-)
Attached patch patchSplinter Review
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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Attachment #275624 - Flags: review?(vladimir)
Comment on attachment 275624 [details] [diff] [review]
patch

Yeah, this looks fine.
Attachment #275624 - Flags: review?(vladimir)
Attachment #275624 - Flags: review+
Attachment #275624 - Flags: approval1.9+
Comment on attachment 275624 [details] [diff] [review]
patch

Whoops, can't do approval 1.9 now
Attachment #275624 - Flags: approval1.9+ → approval1.9?
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).
Attachment #275624 - Flags: approval1.9? → approval1.9+
Targeting M10.
Target Milestone: --- → mozilla1.9 M10
Keywords: checkin-needed
Assignee: nobody → philip
Target Milestone: mozilla1.9 M10 → mozilla1.9 M9
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
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I backed this out, as it didn't have M9 approval.
/me frowns at stuart for asking him to check this in
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Status: REOPENED → NEW
Flags: in-testsuite?
Keywords: testcase
ugh, sorry -- when i asked it had approval-m9+ on it... probably when connor messed up the flag :/
Priority: -- → P2
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.
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.
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.
Attached file 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.
Attachment #273540 - Attachment is obsolete: true
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.
Attachment #275624 - Flags: approvalM9?
Comment on attachment 275624 [details] [diff] [review]
patch

a=endgame drivers for M9
Attachment #275624 - Flags: approvalM9? → approvalM9+
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
Status: NEW → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 M10 → mozilla1.9 M9
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.
Attachment #286588 - Flags: review?(dolske)
Attachment #286588 - Flags: review?(dolske) → review+
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.
Status: RESOLVED → VERIFIED

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.

You need to log in before you can comment on or make changes to this bug.