Bug 43102 - Set incorrect but close expectations for mask-colorspace.svg on mac
: Set incorrect but close expectations for mask-colorspace.svg on mac
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 43172 42403 42428 43142
  Show dependency treegraph
 
Reported: 2010-07-27 19:37 PDT by Alex Nicolaou
Modified: 2010-07-29 03:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.13 KB, patch)
2010-07-27 19:42 PDT, Alex Nicolaou
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2010-07-28 17:33 PDT, Alex Nicolaou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Nicolaou 2010-07-27 19:37:19 PDT
The correct colours for the mask-colorspace.svg test are (111,111,111) and (78,78,78). On Mac, when CoreGraphics converts the mask image from sRGB to Linear, the values that come back in WebCore/platform/graphics/cg/ImageBufferCG.cpp don't match my expectations. Specifically, around line 171:

    for (int y = 0; y < numRows; ++y) {
        for (int x = 0; x < numColumns; x++) {
            int basex = x * 4;
            unsigned char alpha = srcRows[basex + 3];
            if (multiplied == Unmultiplied && alpha) {
                destRows[basex] = (srcRows[basex] * 255) / alpha;
                destRows[basex + 1] = (srcRows[basex + 1] * 255) / alpha;  // **** Green channel set to 211
                destRows[basex + 2] = (srcRows[basex + 2] * 255) / alpha;
                destRows[basex + 3] = alpha;
            } else
                reinterpret_cast<uint32_t*>(destRows + basex)[0] = reinterpret_cast<uint32_t*>(srcRows + basex)[0];
        }


The green channel is set to 211 instead of 202 (where 202 is expected because =255*(((round(0.9*255)/255 + 0.055)/1.055)^2.4) = 201.78, and this is how the SVG specification indicates RGB should be converted to Linear RGB as per http://www.w3.org/TR/SVG/painting.html#RenderingProperties). This can be easily verified with a comment on the line indicated as the first pixel of the mask should be 0,202,0 and so when the breakpoint is hit the 0,211,0 can be verified while viewing the mask-colorspace.svg file.

The closest documentation I can find on what CoreGraphics does in this case is here: http://developer.apple.com/mac/library/documentation/GraphicsImaging/Reference/CGColorSpace/Reference/reference.html#//apple_ref/doc/uid/TP30000949-CH3g-BBCHAECC where it says that Linear is a matter of converting from sRGB to RGB with a gamma of 1.0. I don't think this is correct/matches the SVG spec, but I am not sure of it, would be happy to be enlightened.

The expectation provided in the patch attached is the current behaviour which is pretty close to the desired pixel output (something like 104-gray on the outside and 74 or 75 on the inside). So I think it's worth recording and will make a new bug to note that the behaviour is not actually correct.
Comment 1 Alex Nicolaou 2010-07-27 19:42:30 PDT
Created attachment 62791 [details]
Patch
Comment 2 Alex Nicolaou 2010-07-27 22:52:08 PDT
Is there a way to get layout test results automatically added to this bug by a bot? I am afraid the code change may affect the pixel results of a huge number of tests.
Comment 3 Dirk Schulze 2010-07-28 01:25:18 PDT
First, I would realy like to know, if CG on Windows can transform RGB to linearRGB and back like leopard, so that we may just need to exlude tiger.

To the code you mentioned. This is getImageData and reads pixel information (here in an unmultiplied way) and stores them in an array. The data is still RGB. It has nothing to do with the the sRGB to linearRGB transformation.
Only Qt, Skia and Cairo have the transformation code in their ImageBuffer's as far as I know.
Comment 4 Alex Nicolaou 2010-07-28 06:53:15 PDT
krit: I think I misunderstood your request on this point on IRC yesterday, because I just ran the nightly build and of course it gave me wrong results because the #if around the colourspace higher up in ImageBufferCG disables setting the colour space to linear.

Secondly, I don't understand why you think the colour data are not transformed. The same #if that we discussed that guards the setting of the colour space for tiger+mac and needs to have chromium added affects whether the pixel values as returned by the quoted code are transformed from sRGB to linear or not. If I put that #if back to the way it is pre-patch, of course I'll see my untransformed colour value (230 in this case); whiile with the transform turned on, I am seeing the wrong value (211 instead of 202). Have I misunderstood soemthing about ImageBufferCG?
Comment 5 Alex Nicolaou 2010-07-28 14:43:39 PDT
krit: Could you take a look @ this one?
Comment 6 Ojan Vafai 2010-07-28 16:07:20 PDT
Comment on attachment 62791 [details]
Patch

(In reply to comment #2)
> Is there a way to get layout test results automatically added to this bug by a bot? I am afraid the code change may affect the pixel results of a huge number of tests.

Sorry, I missed this comment. Setting back to r?.

The way you do this currently is to commit the patch and then grab the results off the bot. But, I'm confused now. Do you expect a lot of tests to change, if so, why did you include only the one set of new results in the patch?

For the chromium bots, you should include the tests that will fail in test_expectations.txt with a comment that they'll be removed once results can be grabbed off the bots.
Comment 7 Alex Nicolaou 2010-07-28 16:33:31 PDT
(In reply to comment #6)
> (From update of attachment 62791 [details])
> (In reply to comment #2)
> > Is there a way to get layout test results automatically added to this bug by a bot? I am afraid the code change may affect the pixel results of a huge number of tests.
> 
> Sorry, I missed this comment. Setting back to r?.
> 
> The way you do this currently is to commit the patch and then grab the results off the bot. But, I'm confused now. Do you expect a lot of tests to change, if so, why did you include only the one set of new results in the patch?

I need to set up a leopard machine to rebaseline the tree because I only discovered today that I'm not allowed to rebaseline on snow leopard due to font rendering differences. I guess the right answer here is I need to set up a leopard mac to complete this patch, but in a perfect world there'd be a layout test bot that would run on the patch prior to submission. I also can't trust the commit queue because it isn't running all the tests.

FWIW this change is certainly more correct for Chromium/CG but the overhead of making it when my primary platform isn't mac is really high. I need to get myself set up better (it seems with four environments: linux, mac snow leopard, mac leopard, and windows).
Comment 8 Alex Nicolaou 2010-07-28 17:33:45 PDT
Created attachment 62899 [details]
Patch
Comment 9 Alex Nicolaou 2010-07-28 17:35:07 PDT
*** Bug 43142 has been marked as a duplicate of this bug. ***
Comment 10 Alex Nicolaou 2010-07-28 17:35:42 PDT
This is still not ready for review as I need my leopard machine to finish building.
Comment 11 Ojan Vafai 2010-07-28 17:37:48 PDT
> I need to set up a leopard machine to rebaseline the tree because I only discovered today that I'm not allowed to rebaseline on snow leopard due to font rendering differences. I guess the right answer here is I need to set up a leopard mac to complete this patch, but in a perfect world there'd be a layout test bot that would run on the patch prior to submission. I also can't trust the commit queue because it isn't running all the tests.
> 
> FWIW this change is certainly more correct for Chromium/CG but the overhead of making it when my primary platform isn't mac is really high. I need to get myself set up better (it seems with four environments: linux, mac snow leopard, mac leopard, and windows).

I understand now. It's fine to commit this as is as long as you coordinate with the Chromium webkit sheriff to get it committed and then shortly thereafter (i.e. after the bots have run the new code) checkin any other necessary rebaselines. I'll r+ it and you can coordinate with the webkit sheriff to get it committed. Sorry for the confusion.
Comment 12 Victor Wang 2010-07-28 18:54:05 PDT
Comment on attachment 62899 [details]
Patch

Clearing flags on attachment: 62899

Committed r64254: <http://trac.webkit.org/changeset/64254>
Comment 13 Victor Wang 2010-07-28 18:54:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Dirk Schulze 2010-07-28 23:08:08 PDT
(In reply to comment #11)
> > I need to set up a leopard machine to rebaseline the tree because I only discovered today that I'm not allowed to rebaseline on snow leopard due to font rendering differences. I guess the right answer here is I need to set up a leopard mac to complete this patch, but in a perfect world there'd be a layout test bot that would run on the patch prior to submission. I also can't trust the commit queue because it isn't running all the tests.
> > 
> > FWIW this change is certainly more correct for Chromium/CG but the overhead of making it when my primary platform isn't mac is really high. I need to get myself set up better (it seems with four environments: linux, mac snow leopard, mac leopard, and windows).
> 
> I understand now. It's fine to commit this as is as long as you coordinate with the Chromium webkit sheriff to get it committed and then shortly thereafter (i.e. after the bots have run the new code) checkin any other necessary rebaselines. I'll r+ it and you can coordinate with the webkit sheriff to get it committed. Sorry for the confusion.

Did you check if the (PLATFORM(MAC) || PLATFORM(CHROMIUM)) check was neccessary before r+ing it, like I wrote some comments above?
Comment 15 Alex Nicolaou 2010-07-28 23:19:30 PDT
(In reply to comment #14)
> Did you check if the (PLATFORM(MAC) || PLATFORM(CHROMIUM)) check was neccessary before r+ing it, like I wrote some comments above?

Sorry as I tried to explain on IM I don't have a windows setup to validate this. I added bug 43172 so that I don't forget to fix this once I have a windows system set up for webkit development.
Comment 16 Dirk Schulze 2010-07-29 00:20:55 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Did you check if the (PLATFORM(MAC) || PLATFORM(CHROMIUM)) check was neccessary before r+ing it, like I wrote some comments above?
> 
> Sorry as I tried to explain on IM I don't have a windows setup to validate this. I added bug 43172 so that I don't forget to fix this once I have a windows system set up for webkit development.

Thank you Alex.
Comment 17 Jeremy Orlow 2010-07-29 03:28:37 PDT
FYI: Added a baseline to chromium for svg/custom/convolution-crash.svg : http://trac.webkit.org/changeset/64268