Bug 22150 – Color.cpp gives wrong colors for alpha < 1

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 25005 [details]
Short test case in canvas

[Dirk Schulze]

Created attachment 25006 [details]
Screenshot

left ff, right the result of qt/gtkwebkit

[Dirk Schulze]

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)

[Oliver Hunt]

I'll need to look at Color's implementation, but i would guess it's multiply/dividing at the wrong time

[Ariya Hidayat]

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.

[Ariya Hidayat]

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

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Ariya Hidayat]

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.

[Ariya Hidayat]

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.

[George Staikos]

Seems to me that this is wrong.  I think the test should just be skipped, not putting a different result into place.

[Dirk Schulze]

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. 

[Ariya Hidayat]

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.

[Chang Shu]

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.

[Darin Adler]

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.

[Chang Shu]

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

[Chang Shu]

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

[Eric Seidel (no email)]

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.

[Chang Shu]

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.

[Darin Adler]

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

[Chang Shu]

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

[Oliver Hunt]

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

[Darin Adler]

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.

[Chang Shu]

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

[Chang Shu]

Created attachment 50426 [details]
fix patch 3

[Chang Shu]

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.

[Darin Adler]

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

[Darin Adler]

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.

[Darin Adler]

But the r=me still stands. Those are just a couple of minor criticisms/ways this could be improved.

[Chang Shu]

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.