Bug 43102 – Set incorrect but close expectations for mask-colorspace.svg on mac

• Component: SVG
• Status: RESOLVED FIXED

[Alex Nicolaou]

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.

[Alex Nicolaou]

Created attachment 62791 [details]
Patch

[Alex Nicolaou]

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.

[Dirk Schulze]

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.

[Alex Nicolaou]

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?

[Alex Nicolaou]

krit: Could you take a look @ this one?

[Ojan Vafai]

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.

[Alex Nicolaou]

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

[Alex Nicolaou]

Created attachment 62899 [details]
Patch

[Alex Nicolaou]

*** Bug 43142 has been marked as a duplicate of this bug. ***

[Alex Nicolaou]

This is still not ready for review as I need my leopard machine to finish building.

[Ojan Vafai]

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

[Victor Wang]

Comment on attachment 62899 [details]
Patch

Clearing flags on attachment: 62899

Committed r64254: <http://trac.webkit.org/changeset/64254>

[Victor Wang]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

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

[Alex Nicolaou]

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

[Dirk Schulze]

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

[Jeremy Orlow]

FYI: Added a baseline to chromium for svg/custom/convolution-crash.svg : http://trac.webkit.org/changeset/64268