Bug 105437 – Incorrect color space conversion for FEImage

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

[Florin Malita]

Chromium issue: http://code.google.com/p/chromium/issues/detail?id=166154

The default color space for SVG filters is linearRGB (http://www.w3.org/TR/SVG/filters.html#FilterPrimitivesOverviewIntro), but FEImage always produces deviceRGB results without updating its result color space field to reflect that. Hence, the filter chain performs an incorrect linearRGB->deviceRGB conversion for FEImage colors.

Patch to follow.

[Florin Malita]

Created attachment 180224 [details]
Patch

[Florin Malita]

(In reply to comment #1)
> Created an attachment (id=180224) [details]
> Patch

I've verified that the new pixel results are consistent with FF.

[Stephen Chenney]

Comment on attachment 180224 [details]
Patch

Wow, that explains why the CG results were different for so many images. I would R+ this with the nit about the goto, but of course I can't.

[Stephen Chenney]

Comment on attachment 180224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180224&action=review

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:460
>  #endif

Dammit, this was lost the first time through. Why oh why?

You can avoid the goto by putting "else {" inside the if/end and then putting "}" inside the if/end below (there the goto target is now). We use this pattern elsewhere in the code and it is nicer.

[Eric Seidel (no email)]

The review tool badness continues.

[Build Bot]

Comment on attachment 180224 [details]
Patch

Attachment 180224 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15405851

New failing tests:
http/tests/security/data-url-inline.css.html

[Dirk Schulze]

Comment on attachment 180224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180224&action=review

r=me. But agree with schenney.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:460
>>  #endif
> 
> Dammit, this was lost the first time through. Why oh why?
> 
> You can avoid the goto by putting "else {" inside the if/end and then putting "}" inside the if/end below (there the goto target is now). We use this pattern elsewhere in the code and it is nicer.

yes, please use else { or #else. Better the first one.

[Florin Malita]

Created attachment 180332 [details]
Patch for landing

[Florin Malita]

Comment on attachment 180224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180224&action=review

>>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:460
>>>  #endif
>> 
>> Dammit, this was lost the first time through. Why oh why?
>> 
>> You can avoid the goto by putting "else {" inside the if/end and then putting "}" inside the if/end below (there the goto target is now). We use this pattern elsewhere in the code and it is nicer.
> 
> yes, please use else { or #else. Better the first one.

Thanks, done.

I guess this could be cleaned up even more by having a separate transformResultColorSpaceOpenCL() for that block, and then prefixing the transformColorSpace() call with a single ifdef-ed conditional:

#if ENABLE(OPENCL)
    if (!transformResultColorSpaceOpenCL(dstColorSpace))
#endif
        asImageBuffer()->transformColorSpace(m_resultColorSpace, dstColorSpace);

But I'm not set up for OpenCL builds and not quite comfortable with making this change blindly :)

[WebKit Review Bot]

Comment on attachment 180332 [details]
Patch for landing

Clearing flags on attachment: 180332

Committed r138250: <http://trac.webkit.org/changeset/138250>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.