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>
Created attachment 61456 [details] Incorrect result as rendered by chromium
Created attachment 61458 [details] Correct rendering [Safari Version 5.0 (6533.16)]
I will soon attach a patch with a layout test corresponding to this example and the fix to SVGResourceMasker.cpp
Created attachment 61460 [details] Patch
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.
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.
(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 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.
Created attachment 61469 [details] Patch
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 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?"
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.
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).
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 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.
Comment on attachment 61469 [details] Patch Clearing flags on attachment: 61469 Committed r63450: <http://trac.webkit.org/changeset/63450>
All reviewed patches have been landed. Closing bug.