Bug 42228 – SVG masks are in the wrong colour space in all non-CG platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Alex Nicolaou]

Created attachment 61455 [details]
example SVG file that renders incorrectly on chromium

The ImageBuffer instantiated for SVG masks is supposed to be in LinearRGB but on non-CG platforms the colour space conversion is never done so the colours do not come out correct. This SVG example shows two grey squares with dark grey circles that should be identical but only render correctly on CG, but not on chromium.


<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<defs>
<mask id="mask">
    <rect x="0" y="0" width="140" height="140" fill="rgb(0%,90%,0%)"/>
    <circle cx="70" cy="70" r="40" fill="rgb(0%, 99%, 0%)"/>
</mask>
</defs>
<rect x="20" y="20" width="100" height="100" style="fill:black;mask:url(#mask)"/>

<!-- reference image - should be identical to above -->
<rect x="150" y="20" width="100" height="100" fill="rgb(111,111,111)"/>
<circle cx="200" cy="70" r="40" fill="rgb(76,76,76)"/>
</svg>

[Alex Nicolaou]

Created attachment 61456 [details]
Incorrect result as rendered by chromium

[Alex Nicolaou]

Created attachment 61458 [details]
Correct rendering [Safari Version 5.0 (6533.16)]

[Alex Nicolaou]

I will soon attach a patch with a layout test corresponding to this example and the fix to SVGResourceMasker.cpp

[Alex Nicolaou]

Created attachment 61460 [details]
Patch

[WebKit Review Bot]

Attachment 61460 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/SkiaUtils.cpp:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Nicolaou]

The flagged style problem is because I followed the style in the existing file. I don't mind changing it to a pure !pm || !a if you prefer.

[Dirk Schulze]

(In reply to comment #6)
> The flagged style problem is because I followed the style in the existing file. I don't mind changing it to a pure !pm || !a if you prefer.

I would like to give r+ on you mask code. I'm just not sure about your Skia changes, what do they cover? Where is the test case for it? The change is not mentioned and discribed in the ChangeLog.

[Dirk Schulze]

Comment on attachment 61460 [details]
Patch

because of the comment above. Move the Skia changes into another Patch or make a discription why they are neccessary with a test where the Skia code crashes/fails.

[Alex Nicolaou]

Created attachment 61469 [details]
Patch

[Alex Nicolaou]

Hmm, at some intermediate stage today that check in the colour conversion was necessary to avoid a divide-by-zero because a packed colour with an alpha channel of 0 was reaching that point and causing problems. But I should have separated it out into another fix in the first place, sorry about that. Most annoyingly, the problem no longer occurs so I have just dropped the skia change from the patch.

On the plus side, this patch is now the straightforward change it was meant to be, please take another look.

[Darin Adler]

Comment on attachment 61469 [details]
Patch

> +#if !PLATFORM(CG)
> +    maskerData->maskImage->transformColorSpace(DeviceRGB, LinearRGB);
> +#endif

It seems mysterious to have a platform ifdef here, even if it's correct. Is there some other way to do this? For example, could there be some kind of feature flag to indicate if this is needed instead of just PLATFORM(CG)? Or perhaps a function that is an inline empty body on CG and implemented on other platforms. The point of the abstractions is to avoid #ifdefs like this in the cross-platform code.

At the very least, there needs to be a comment in the code, not just the change log, explaining why one platform is different from all the others. Even your change log comment states that this is needed "ImageBuffer needs to be explicitly told to convert", without answering the question "why?"

[Alex Nicolaou]

Based on email with senorblanco, looks like http://code.google.com/p/chromium/issues/detail?id=48498 might be fixed by the skia utils change. I will take that part of this up in that bug.

[Alex Nicolaou]

Thanks for your comment Darin. Could we address it in a separate change? This is the pattern of code used in the change that introduced this way of handling colour spaces and it already occurs in RenderSVGResourceFilter.cpp in exactly this style. If we are to fix it, I'd prefer a separate change that hits all places it happens, and also I didn't want to do this differently than how Dirk coded the original change.

Also, aside from changing the specific #if there are more complicated ways of fixing this that might be good, but need discussion, so a separate bug/change gives us a place to have that discussion (or we can take it to email).

[Alex Nicolaou]

Dirk are you willing to r+ the latest version of this patch so that I can move on to the problem Darin raised in the other bug?

[Dirk Schulze]

Comment on attachment 61469 [details]
Patch

We have this #if's on some places now. Removing all of them should be fixed in another patch. r=me for this one.

[WebKit Commit Bot]

Comment on attachment 61469 [details]
Patch

Clearing flags on attachment: 61469

Committed r63450: <http://trac.webkit.org/changeset/63450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.