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.
Created attachment 25005 [details] Short test case in canvas
Created attachment 25006 [details] Screenshot left ff, right the result of qt/gtkwebkit
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)
I'll need to look at Color's implementation, but i would guess it's multiply/dividing at the wrong time
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.
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).
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.
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.
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.
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.
Seems to me that this is wrong. I think the test should just be skipped, not putting a different result into place.
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.
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.
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 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.
(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.
(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 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.
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.
(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.
> > 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.
(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 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.
(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...
Created attachment 50426 [details] fix patch 3
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 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 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.
But the r=me still stands. Those are just a couple of minor criticisms/ways this could be improved.
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.