Bug 30333 – (Un)premultiplication code is not optimal and broken in alpha==0 case

• Component: SVG
• Status: NEW

[Sebastian Dröge (slomo)]

Hi,
attached patch fixes the (Un)premultiplication code in WebCore/platform/graphics/Color.cpp.

Explanation why it's broken follows

[Sebastian Dröge (slomo)]

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.

[Darin Adler]

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.

[Sebastian Dröge (slomo)]

(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?

[Sebastian Dröge (slomo)]

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

[Dirk Schulze]

(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.

[Sebastian Dröge (slomo)]

Thanks, I'll take those tests as starting point

[Sebastian Dröge (slomo)]

After reading bug #22150 it could be that my patch improves the situation there... we'll see.

[Dirk Schulze]

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.

[Simon Fraser (smfr)]

Had to work around this issue in http://trac.webkit.org/changeset/54106

[Sebastian Dröge (slomo)]

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?

[Ahmad Saleem]

Is this still an issue? I tried to look for the code from the patch but couldn't manage to find "colorFromPremultipliedARGB" and it seems that it was deleted in this commit:

https://github.com/WebKit/WebKit/commit/9b9b872709ed845f54ad8f5aa9257bd0212c455d

If not needed, can we close this bug or if this is still an issue, can we get update explanation etc.? Thanks!