Bug 22150

Summary: Color.cpp gives wrong colors for alpha < 1
Product: WebKit Reporter: Dirk Schulze <krit>
Component: New BugsAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: ariya.hidayat, cshu, darin, mitz, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Short test case in canvas
none
Screenshot
none
Short test case in CSS
none
potential fix for the problem
none
Result of getImageData with ariya's patch
none
fix patch
eric: review-
fix patch 2
darin: review-
fix patch 3 darin: review+

Description Dirk Schulze 2008-11-09 13:23:01 PST
Color.cpp gives wrong colors, for an alpha lower than 1. This is a problem for canvas in Qt and Cairo. Cg seems _not_ to be affected.
It's difficult to messure it or make a real LayoutTest for it, since Qt and Cairo lacks a fully working getImageData support. But it is difficult to implement getImageData as long as we can't test it because of wrong colors.

I'll upload a simple test. If you make a screenshot and compare the color values (rgb) with the color-values of ff or opera, you can see that they differs from ff and opera. ff and opera give the right values.

I checked if it is a problem with canvas. But canvas has the right colors. The colors in canvas are parsed by CSSParser::parseColor and are given to the GraphicsContext by CanvasStyle.cpp. But the GraphicsContext gets the wrong colors.
I can't say if it is a problem of Color.cpp or CSSParser.cpp.

See also: https://bugs.webkit.org/show_bug.cgi?id=21575#c5

And again: Cg seems not to be affected.
Comment 1 Dirk Schulze 2008-11-09 13:25:28 PST
Created attachment 25005 [details]
Short test case in canvas
Comment 2 Dirk Schulze 2008-11-09 13:30:27 PST
Created attachment 25006 [details]
Screenshot

left ff, right the result of qt/gtkwebkit
Comment 3 Dirk Schulze 2008-11-10 05:08:54 PST
Created attachment 25014 [details]
Short test case in CSS

A div box with the same background color like the canvas example but this time via CSS.
This shows that it is not canvas' fault. The difference is minimal (that means it's not visible to the naked eye), but has a big effect on getImageData for canvas.

(Chrome has the same wrong behavior like Qt and Cairo)
Comment 4 Oliver Hunt 2009-01-05 01:39:32 PST
I'll need to look at Color's implementation, but i would guess it's multiply/dividing at the wrong time
Comment 5 Ariya Hidayat 2009-04-30 02:43:05 PDT
Seems to be precision problem. For example, if alpha is 1.0/255.0, when the last few digits are stripped (i.e. the value is truncated), multiplying back to 255 does not give 1.0, but rather 0.99998. Static casting it back to integer gives 0, instead of 1.
Comment 6 Ariya Hidayat 2009-04-30 02:46:06 PDT
Created attachment 29905 [details]
potential fix for the problem

The problem with this patch is the rounding. Now (0.5/255.0) as the alpha value will be treated as 1 (of max 255). Need to find out what CSS specs say about this, or if this is just our implementation limitation (of using RGBA32).
Comment 7 Dirk Schulze 2009-04-30 03:17:15 PDT
Created attachment 29906 [details]
Result of getImageData with ariya's patch

This is the new result after ariya's patch. Many more tests passes now. There are still some tests, that fail (beside outside canvas bounds). But the alpha channel is correct on all tests now.
Comment 8 Dirk Schulze 2009-05-01 13:33:20 PDT
Without going into to much details, the following tests seem to fail on MacOS X 10.5 with this patch:

fast/block/basic/fieldset-stretch-to-legend.html
fast/block/float/intruding-painted-twice.html
fast/block/float/relative-painted-twice.html
fast/borders/fieldsetBorderRadius.html
fast/canvas/canvas-alphaImageData-behavior.html
fast/css/percentage-non-integer.html
fast/css/shadow-multiple.html
fast/js/date-DST-time-cusps.html
fast/js/date-big-setdate.html
fast/overflow/float-in-relpositioned.html
fast/overflow/overflow-focus-ring.html
fast/repaint/fixed.html
fast/table/border-collapsing/equal-precedence-resolution.html
fast/text/shadow-no-blur.html
svg/custom/pointer-events-path.svg

I did not run the tests without the patch, but the buildbots were green.
Comment 9 Ariya Hidayat 2009-05-04 04:51:41 PDT
Some of the failure make sense, because now the color alpha is rounded. Some of them are certainly invalid (nothing to do with color), e.g. date-big-setdate.html.
Comment 10 Ariya Hidayat 2009-05-12 04:51:46 PDT
I change my position on this. As long as Qt is internally using premultiplied ARGB32 for doing the alpha-blending, there will be discrepancies with respect to the precise ARGB32 color that will be retrieved from the pixmap.

As agreed by Simon Hausmann, for the Qt port we will just create a specialized test result to take this into account.
Comment 11 George Staikos 2009-05-12 06:59:06 PDT
Seems to me that this is wrong.  I think the test should just be skipped, not putting a different result into place.
Comment 12 Dirk Schulze 2009-05-12 09:14:32 PDT
Qt and Cairo pass many tests, that are not related to a change of the alpha channel. This is the only test for getImageData and it's important to know if someone breaks something for getImageData. That means skipping is the wrong solution. 
Comment 13 Ariya Hidayat 2009-05-12 10:15:37 PDT
On IRC, George suggested to split the test into two parts: one which does not use alpha channel and one which uses it. This is good and something we should consider as well.
Comment 14 Chang Shu 2010-03-03 13:40:27 PST
Created attachment 49939 [details]
fix patch

I came up with the same solution as Ariya before I found the existing bug with Ariya's fix. To avoid changing the behavior of rounding up alpha value, I used a small tolerance number. I tested on mac and the patch didn't break anything.
Comment 15 Darin Adler 2010-03-03 14:12:51 PST
Comment on attachment 49939 [details]
fix patch

> -        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255);
> +        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255 + 0.0000000001);

That doesn't look right to me. I believe the correct fix for this is:

    colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * nextafter(256.0, 0.0));

See code using nextafter in Color.cpp. We'd have to do some testing to be sure this does not cause problems.
Comment 16 Chang Shu 2010-03-03 14:55:22 PST
(In reply to comment #15)
> (From update of attachment 49939 [details])
> > -        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255);
> > +        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255 + 0.0000000001);
> 
> That doesn't look right to me. I believe the correct fix for this is:
> 
>     colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) *
> nextafter(256.0, 0.0));
> 
> See code using nextafter in Color.cpp. We'd have to do some testing to be sure
> this does not cause problems.

Thanks for showing me the right way to do this fix! I will submit the new patch after running all layout tests.
Comment 17 Chang Shu 2010-03-04 07:51:58 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 49939 [details] [details])
> > > -        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255);
> > > +        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * 255 + 0.0000000001);
> > 
> > That doesn't look right to me. I believe the correct fix for this is:
> > 
> >     colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) *
> > nextafter(256.0, 0.0));
> > 
> > See code using nextafter in Color.cpp. We'd have to do some testing to be sure
> > this does not cause problems.

Hi, Darin, your algorithm seems problematic. Note that we do NOT want to round up alpha in general but only when the precision plays a role.
For example, let alpha = 0.995, the result is expected to be
floor(0.995 * 255) = floor(253.72) = 253. While in your algorithm, the result is
floor(0.995 * nextafter(256,0)) = floor(~254.72499) = 254.
Another example, let alpha = 0.0078431372549, which is 2.0/255.0, but lost some precision due to float and string representation, the result is expected to be
floor(2.0/255.0*255.0) = 2, but actual result(current algorithm) is
floor(0.0078431372549 * 255.0) = floor(1.9999999999995002) = 1.
Comment 18 Eric Seidel (no email) 2010-03-04 13:17:22 PST
Comment on attachment 49939 [details]
fix patch

This change at minimum needs an explanitory comment in the code.  Otherwise future contributers will be as confused as I am now.
Comment 19 Chang Shu 2010-03-05 09:24:14 PST
Created attachment 50100 [details]
fix patch 2

I used an "intelligent" number in this patch.
I also put in some comments as suggested by Eric.
Comment 20 Darin Adler 2010-03-10 09:28:03 PST
(In reply to comment #17)
> Hi, Darin, your algorithm seems problematic. Note that we do NOT want to round
> up alpha in general but only when the precision plays a role.
> For example, let alpha = 0.995, the result is expected to be
> floor(0.995 * 255) = floor(253.72) = 253. While in your algorithm, the result
> is
> floor(0.995 * nextafter(256,0)) = floor(~254.72499) = 254.

I don't understand why 0.995 should be 253. Maybe to match other web browsers?

253 / 255 is 0.992156862745098
245 / 255 is 0.996078431372549

The value 0.995 is closer to 0.996 than to 0.992 and so 0.995 should give the result 254, not 253. This is rounding to nearest, not rounding up.

> Another example, let alpha = 0.0078431372549, which is 2.0/255.0, but lost some
> precision due to float and string representation, the result is expected to be
> floor(2.0/255.0*255.0) = 2, but actual result(current algorithm) is
> floor(0.0078431372549 * 255.0) = floor(1.9999999999995002) = 1.

Yes, we agree on the correct result for 0.0078431372549. It must be 2.
Comment 21 Chang Shu 2010-03-10 10:05:58 PST
> > For example, let alpha = 0.995, the result is expected to be
> > floor(0.995 * 255) = floor(253.72) = 253. While in your algorithm, the result
> > is
> > floor(0.995 * nextafter(256,0)) = floor(~254.72499) = 254.
> 
> I don't understand why 0.995 should be 253. Maybe to match other web browsers?

I also think 254 makes more sense (round to nearest). I just didn't want to change the current behavior and break an existing test case: fast/canvas/canvas-alphaImageData-behavior.html. 253 was expected over there.
I also verified it on firefox and it gives 254. Looks like we should update the expected results for all the failed tests.
Comment 22 Oliver Hunt 2010-03-10 10:24:14 PST
(In reply to comment #21)
> > > For example, let alpha = 0.995, the result is expected to be
> > > floor(0.995 * 255) = floor(253.72) = 253. While in your algorithm, the result
> > > is
> > > floor(0.995 * nextafter(256,0)) = floor(~254.72499) = 254.
> > 
> > I don't understand why 0.995 should be 253. Maybe to match other web browsers?
> 
> I also think 254 makes more sense (round to nearest). I just didn't want to
> change the current behavior and break an existing test case:
> fast/canvas/canvas-alphaImageData-behavior.html. 253 was expected over there.
> I also verified it on firefox and it gives 254. Looks like we should update the
> expected results for all the failed tests.

That seems like the correct approach to me, I can't recall what rounding mode i was targeting back when i wrote this but i don't recall deliberately choosing one model over another.
Comment 23 Darin Adler 2010-03-10 11:06:54 PST
Comment on attachment 50100 [details]
fix patch 2

Lets do a fix more like the one I suggested as per our current discussion in the bug.
Comment 24 Chang Shu 2010-03-10 12:07:15 PST
(In reply to comment #23)
> (From update of attachment 50100 [details])
> Lets do a fix more like the one I suggested as per our current discussion in
> the bug.

working on that...
Comment 25 Chang Shu 2010-03-10 12:49:31 PST
Created attachment 50426 [details]
fix patch 3
Comment 26 Chang Shu 2010-03-10 13:00:07 PST
Comment on attachment 50426 [details]
fix patch 3

the styles checks, etc. do not seem to start if I don't put r?. but please do not approve the patch before all lights show green.
Comment 27 Darin Adler 2010-03-10 15:24:52 PST
Comment on attachment 50426 [details]
fix patch 3

Will this change pixel results for any tests? If so, we need those to be updated too.

Assuming it does not, r=me
Comment 28 Darin Adler 2010-03-10 15:29:41 PST
Comment on attachment 50426 [details]
fix patch 3

> +        // Round the float alpha to its nearest integer value.

That comment is not what the code does. After all, the float is between 0 and 1, so the nearest integer is either 0 or 1.

What the code does is convert the floating point number to an integer in the range [0, 256), with an equal distribution across all 256 values.

> +        colorArray[3] = static_cast<int>(max(0.0, min(1.0, v->fValue)) * (nextafter(256.0, 0.0)));

There's an extra set of parentheses around the nextafter function that are not needed.
Comment 29 Darin Adler 2010-03-10 15:30:01 PST
But the r=me still stands. Those are just a couple of minor criticisms/ways this could be improved.
Comment 30 Chang Shu 2010-03-10 19:26:43 PST
I updated the code with Darin's latest comments on patch 3 and manually committed the patch.
Committed revision 55827. Set the bug to closed.