Bug 30333 - (Un)premultiplication code is not optimal and broken in alpha==0 case
Summary: (Un)premultiplication code is not optimal and broken in alpha==0 case
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-13 06:43 PDT by Sebastian Dröge (slomo)
Modified: 2014-12-07 10:48 PST (History)
5 users (show)

See Also:


Attachments
0002-Fix-Color-alpha-un-premultiplication-code.patch (3.54 KB, patch)
2009-10-13 06:50 PDT, Sebastian Dröge (slomo)
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Dröge (slomo) 2009-10-13 06:43:04 PDT
Hi,
attached patch fixes the (Un)premultiplication code in WebCore/platform/graphics/Color.cpp.

Explanation why it's broken follows
Comment 1 Sebastian Dröge (slomo) 2009-10-13 06:50:58 PDT
Created attachment 41101 [details]
0002-Fix-Color-alpha-un-premultiplication-code.patch

Now to the explanation. First of all, for consistency and correctness the resulting value of (un-)premultiplication with a 0-alpha should result in (0, 0, 0, 0) instead of the input value without changes. This is because:
a) unpremultiplication: if input alpha is 0 all other components *must* be 0 too, otherwise the premultiplication was done incorrectly
b) premultiplication: for non-0 alpha we multiply with alpha, if we do the same for 0 alpha the output will be (0, 0, 0, 0) as it should

For the non-0 alpha cases it makes more sense to round to the nearest integer instead of always rounding upwards. Rounding upwards results in an uneven distribution of color values, i.e. a 0 component is much less likely than every other component value.
Comment 2 Darin Adler 2009-10-13 08:46:43 PDT
Comment on attachment 41101 [details]
0002-Fix-Color-alpha-un-premultiplication-code.patch

Thanks for this contribution!

The change seems OK, but we need a test case that demonstrates the effects this has. We don't take bug fixes without test cases. So please add a test case demonstrating the effect this has. One way to create a test case may be to use the <canvas> element, because that supports reading back pixel values from the canvas buffer.

LayoutTests/fast/canvas/script-tests/set-colors.js is an example of a test case that might be adaptable to test this behavior.

An alternative would be to explain why no test case is possible. That seems unlikely, though.

Also, you need to run all existing tests and generate new results for any tests that now will have different results.

review- because there is no test case.
Comment 3 Sebastian Dröge (slomo) 2009-10-13 21:47:49 PDT
(In reply to comment #2)
> (From update of attachment 41101 [details])
> Thanks for this contribution!
> 
> The change seems OK, but we need a test case that demonstrates the effects this
> has. We don't take bug fixes without test cases. So please add a test case
> demonstrating the effect this has. One way to create a test case may be to use
> the <canvas> element, because that supports reading back pixel values from the
> canvas buffer.
> 
> LayoutTests/fast/canvas/script-tests/set-colors.js is an example of a test case
> that might be adaptable to test this behavior.

I just noticed this while working on other code that needs alpha premultiplication. I have absolutely *no* idea where this code here is used and how.

I could provide a small test case that produces different results with/without the patch by just calling those functions but no idea how to do it from a canvas element.

> An alternative would be to explain why no test case is possible. That seems
> unlikely, though.
> 
> Also, you need to run all existing tests and generate new results for any tests
> that now will have different results.

How can I do that?
Comment 4 Sebastian Dröge (slomo) 2009-10-14 01:23:57 PDT
Ok, I know how to run the layout tests now but this is really taking far too long for such a simply patch. I'll try to write a small testcase for this later though.

Also this code is only used by the cairo backend in a few places, here's the relevant code from cairo itself and as you can see it does the same.

http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n53
http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n408
Comment 5 Dirk Schulze 2009-10-15 09:42:37 PDT
(In reply to comment #4)
> Ok, I know how to run the layout tests now but this is really taking far too
> long for such a simply patch. I'll try to write a small testcase for this later
> though.
> 
> Also this code is only used by the cairo backend in a few places, here's the
> relevant code from cairo itself and as you can see it does the same.
> 
> http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n53
> http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n408

There are already some tests for getImageData and putImageData in LayoutTests/fast/canvas/
This both calls are mainly using this color-code.

Tests that can help you to figure out if your code works are:
   * canvas-putImageData.html
   * canvas-getImageData.html

But I should warn you. canvas-getImageData might still fail because of bug 22150
It would be a great improvement if some more steps pass on canvas-getImageData.html.
Comment 6 Sebastian Dröge (slomo) 2009-10-15 09:51:06 PDT
Thanks, I'll take those tests as starting point
Comment 7 Sebastian Dröge (slomo) 2009-10-15 09:52:28 PDT
After reading bug #22150 it could be that my patch improves the situation there... we'll see.
Comment 8 Dirk Schulze 2010-03-24 00:03:31 PDT
Simon added a test case for SVG and premultiplied/not-premultiplied Colors. Maybe this patch should get rid of the hack in bug 34383 and use this SVG test as test case.
Comment 9 Simon Fraser (smfr) 2010-03-24 09:48:50 PDT
Had to work around this issue in http://trac.webkit.org/changeset/54106
Comment 10 Sebastian Dröge (slomo) 2014-12-07 10:48:37 PST
What should we do with this? Note that if you consider merging this patch, that the mail address used there is not valid anymore.

Simon?