Bug 42228 - SVG masks are in the wrong colour space in all non-CG platforms
Summary: SVG masks are in the wrong colour space in all non-CG platforms
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P4 Normal
Assignee: Nobody
Depends on:
Blocks: 42261
  Show dependency treegraph
Reported: 2010-07-13 19:11 PDT by Alex Nicolaou
Modified: 2010-07-15 13:02 PDT (History)
5 users (show)

See Also:

example SVG file that renders incorrectly on chromium (529 bytes, image/svg+xml)
2010-07-13 19:11 PDT, Alex Nicolaou
no flags Details
Incorrect result as rendered by chromium (5.44 KB, image/png)
2010-07-13 19:12 PDT, Alex Nicolaou
no flags Details
Correct rendering [Safari Version 5.0 (6533.16)] (5.54 KB, image/png)
2010-07-13 19:14 PDT, Alex Nicolaou
no flags Details
Patch (12.58 KB, patch)
2010-07-13 19:58 PDT, Alex Nicolaou
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2010-07-13 22:09 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-13 19:11:50 PDT
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">
<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%)"/>
<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)"/>
Comment 1 Alex Nicolaou 2010-07-13 19:12:51 PDT
Created attachment 61456 [details]
Incorrect result as rendered by chromium
Comment 2 Alex Nicolaou 2010-07-13 19:14:08 PDT
Created attachment 61458 [details]
Correct rendering [Safari Version 5.0 (6533.16)]
Comment 3 Alex Nicolaou 2010-07-13 19:15:27 PDT
I will soon attach a patch with a layout test corresponding to this example and the fix to SVGResourceMasker.cpp
Comment 4 Alex Nicolaou 2010-07-13 19:58:18 PDT
Created attachment 61460 [details]
Comment 5 WebKit Review Bot 2010-07-13 20:01:15 PDT
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.
Comment 6 Alex Nicolaou 2010-07-13 20:15:39 PDT
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.
Comment 7 Dirk Schulze 2010-07-13 21:23:00 PDT
(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.
Comment 8 Dirk Schulze 2010-07-13 21:24:36 PDT
Comment on attachment 61460 [details]

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.
Comment 9 Alex Nicolaou 2010-07-13 22:09:21 PDT
Created attachment 61469 [details]
Comment 10 Alex Nicolaou 2010-07-13 22:12:36 PDT
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.
Comment 11 Darin Adler 2010-07-13 22:15:05 PDT
Comment on attachment 61469 [details]

> +#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?"
Comment 12 Alex Nicolaou 2010-07-13 22:18:49 PDT
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.
Comment 13 Alex Nicolaou 2010-07-13 22:26:04 PDT
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).
Comment 14 Alex Nicolaou 2010-07-15 00:27:38 PDT
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?
Comment 15 Dirk Schulze 2010-07-15 03:23:32 PDT
Comment on attachment 61469 [details]

We have this #if's on some places now. Removing all of them should be fixed in another patch. r=me for this one.
Comment 16 WebKit Commit Bot 2010-07-15 13:02:19 PDT
Comment on attachment 61469 [details]

Clearing flags on attachment: 61469

Committed r63450: <http://trac.webkit.org/changeset/63450>
Comment 17 WebKit Commit Bot 2010-07-15 13:02:28 PDT
All reviewed patches have been landed.  Closing bug.