Bug 212649 – Multiple SVG Filters Unexpectedly lightens image using linearRGB

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

Created attachment 400848 [details]
List of html files that showcase the bug

feComponentTransfer, feComposite, feConvolveMatrix, feGaussianBlur, feMorphology, feSpecularLighting, feDiffuseLighting unexpectedly brightens result images when color-interpolation-filters has value of linearRGB.

For context: 
All SVG filters have an attribute called color-interpolation-filters, which indicates the color space that the filter is operating in, and it has a default value of linearRGB (except feImage, which only operates in sRGB). On CG platforms, color conversion is expected to be handled by Core Graphics automatically when drawing into image buffers. However there is an exception for the the list of filters above, which don't call CG's draw method to ImageBuffers. Instead, they get the ImageData array from the input ImageBuffer and operates on the raw data stored in the data array. The caveat here is that there is no color space conversion from input color space to current operating color space. 

For example, if we have a filter chain like this:
feImage(sRGB) -> feGaussianBlur(linearRGB) -> output(device RGB), current code won't perform a conversion from input sRGB to gaussianblur's linearRGB. This is the reason why those images looks "brightened"

[Radar WebKit Bug Importer]

<rdar://problem/63885737>

Created attachment 400859 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 400859 [details]
Patch

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

It looks like the code changes in fecomposite, feconvolvematrix, fedisplacementmap, felighting, and femorphology are untested.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:289
> +RefPtr<Uint8ClampedArray> FilterEffect::unmultipliedResult(const IntRect& rect, bool colorSpaceConversion, ColorSpace colorSpace)

Please use an enum class instead of a bool. It makes it more clear at the call site what the meaning of the argument is.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:302
> +RefPtr<Uint8ClampedArray> FilterEffect::premultipliedResult(const IntRect& rect, bool colorSpaceConversion, ColorSpace colorSpace)

Ditto.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:489
> +void FilterEffect::copyPremultipliedResult(Uint8ClampedArray& destination, const IntRect& rect, bool colorSpaceConversion, ColorSpace colorSpace)

Ditto.

[Myles C. Maxfield]

Comment on attachment 400859 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:129
> +    
> +#if USE(CG)
> +    in->copyUnmultipliedResult(*pixelArray, drawingRect, true, operatingColorSpace());
> +#else
> +    in->copyUnmultipliedResult(*pixelArray, drawingRect, false);
> +#endif
> +    

I see this pattern duplicated in many places; is there a way to refactor this, perhaps pulling it into a helper function?

[Sam Weinig]

Comment on attachment 400859 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:128
> +#if USE(CG)
> +    in->copyUnmultipliedResult(*pixelArray, drawingRect, true, operatingColorSpace());
> +#else
> +    in->copyUnmultipliedResult(*pixelArray, drawingRect, false);
> +#endif

Why is this being done for CG only? Your ChangeLog does not indicate what makes this problem CG specific.

Created attachment 400951 [details]
Patch

Created attachment 400955 [details]
Patch

Created attachment 400958 [details]
Patch

Created attachment 400961 [details]
Patch

[Darin Adler]

Comment on attachment 400961 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorSpaceConversion.h:33
> +enum class ColorSpaceConversion : bool {
> +    NoColorSpaceConversion,
> +    RequiresColorSpaceConversion,
> +};

The values here should be None and Required, because the context "ColorSpaceConversion" will always be present.

Created attachment 400978 [details]
checking the result of test cases without the patch

Created attachment 400987 [details]
Patch with updates test

Created attachment 401001 [details]
Patch

Created attachment 401007 [details]
Patch

Created attachment 401033 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 401033 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:512
> +                copyConvertedImageBufferResult(destination, rect, AlphaPremultiplication::Premultiplied, colorSpace);

It looks like the color space is only used if the enum is set to Required. Should it just be Optional<ColorSpace> With a default value of Nullopt instead?

Created attachment 401055 [details]
Patch - Made ColorSpace argument Optional

(In reply to Myles C. Maxfield from comment #16)
> Comment on attachment 401033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401033&action=review
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:512
> > +                copyConvertedImageBufferResult(destination, rect, AlphaPremultiplication::Premultiplied, colorSpace);
> 
> It looks like the color space is only used if the enum is set to Required.
> Should it just be Optional<ColorSpace> With a default value of Nullopt
> instead?

Good point. That would make the code more readable

[Darin Adler]

Comment on attachment 401055 [details]
Patch - Made ColorSpace argument Optional

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

> Source/WebCore/platform/graphics/ColorSpaceConversion.h:33
> +enum class ColorSpaceConversion : bool {
> +    None,
> +    Required,
> +};

With the Optional<ColorSpace> approach, I don’t think we need this enum at all any more.

(In reply to Darin Adler from comment #19)
> Comment on attachment 401055 [details]
> Patch - Made ColorSpace argument Optional
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401055&action=review
> 
> > Source/WebCore/platform/graphics/ColorSpaceConversion.h:33
> > +enum class ColorSpaceConversion : bool {
> > +    None,
> > +    Required,
> > +};
> 
> With the Optional<ColorSpace> approach, I don’t think we need this enum at
> all any more.

I do think with that flag the code is more readable - the flag that explicitly says we are doing color space conversion

[Darin Adler]

(In reply to guowei_yang from comment #20)
> I do think with that flag the code is more readable - the flag that
> explicitly says we are doing color space conversion

OK. I think that specifying a color space also explicitly says we are doing color space conversion.

[Darin Adler]

Comment on attachment 401055 [details]
Patch - Made ColorSpace argument Optional

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

>>> Source/WebCore/platform/graphics/ColorSpaceConversion.h:33
>>> +enum class ColorSpaceConversion : bool {
>>> +    None,
>>> +    Required,
>>> +};
>> 
>> With the Optional<ColorSpace> approach, I don’t think we need this enum at all any more.
> 
> I do think with that flag the code is more readable - the flag that explicitly says we are doing color space conversion

That’s not how I think about it. Color space conversion is done because you must do it to be correct. If you pass the color space then you are requesting that conversion.

It’s like passing in one argument that says "look at this other argument"; we don’t need to say it twice.

Created attachment 401061 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 401061 [details]
Patch

I'm not really a fan of this patch. It's adding more #if USE(CG) everywhere. I'd much prefer some virtual function somewhere that just returns a different answer for CG platforms.

[Simon Fraser (smfr)]

This patch is also adding to the complexity of color-space stuff in the filter code, rather than simplifying.

Why do we still have FilterEffect::transformResultColorSpace() which a no-op for CG, then this new colorspace conversion code in so many other places?

Created attachment 401394 [details]
Patch

Created attachment 401395 [details]
Patch

[Darin Adler]

Comment on attachment 401395 [details]
Patch

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

Not setting review+ because I think Simon will want to review.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:499
> +    // Filters that directly manipulates ImageData pixels requires
> +    // color space conversion that's not handled by CG on CG platforms

Not sure that we want to keep mentioning CG every time we call requiresAdditionalColorSpaceConversion. Comment can go inside that function rather than at every call site.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:519
> +            // Filters that directly manipulates ImageData pixels requires
> +            // color space conversion that's not handled by CG on CG platforms

Comment should be indented.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:520
> +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Premultiplied);

Should use auto here.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:540
> +    // Filters that directly manipulates ImageData pixels requires
> +    // color space conversion that's not handled by CG on CG platforms

Ditto.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:182
> +    bool requiresAdditionalColorSpaceConversion(Optional<ColorSpace>);

This seems like a good abstraction.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:183
> +    bool usesCGPlatform();

This doesn’t; I would not add this. It’s OK to have a couple #if USE(CG), just not more than those two.

(In reply to Darin Adler from comment #28)
> Comment on attachment 401395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401395&action=review
> 
> Not setting review+ because I think Simon will want to review.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:499
> > +    // Filters that directly manipulates ImageData pixels requires
> > +    // color space conversion that's not handled by CG on CG platforms
> 
> Not sure that we want to keep mentioning CG every time we call
> requiresAdditionalColorSpaceConversion. Comment can go inside that function
> rather than at every call site.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:519
> > +            // Filters that directly manipulates ImageData pixels requires
> > +            // color space conversion that's not handled by CG on CG platforms
> 
> Comment should be indented.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:520
> > +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Premultiplied);
> 
> Should use auto here.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:540
> > +    // Filters that directly manipulates ImageData pixels requires
> > +    // color space conversion that's not handled by CG on CG platforms
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.h:182
> > +    bool requiresAdditionalColorSpaceConversion(Optional<ColorSpace>);
> 
> This seems like a good abstraction.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.h:183
> > +    bool usesCGPlatform();
> 
> This doesn’t; I would not add this. It’s OK to have a couple #if USE(CG),
> just not more than those two.

What is the general rule of thumb for using those complier flags? I was under the impression that we want those flags as less as possible

[Sam Weinig]

Comment on attachment 401395 [details]
Patch

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

What is the reason for not using ImageBuffer::transformColorSpace(srcSpace, dstSpace)? Do the CG/IOSurface based ImageBufferBackends not support that?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:462
> +    if (!convertedData)
> +        return nullptr;
> +    return convertedData;

This null check doesn't really do anything. Just returning the result of convertedBuffer->getImageData(...) would be fine here. The caller will have to do a null check either way.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:480
> +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Unpremultiplied);
> +                copyImageBytes(*convertedImageData->data(), destination, rect);

Since convertImageBufferToColorSpace() can return null, this doesn't seem safe. (Same for a few other callers).

(In reply to Sam Weinig from comment #30)
> Comment on attachment 401395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401395&action=review
> 
> What is the reason for not using ImageBuffer::transformColorSpace(srcSpace,
> dstSpace)? Do the CG/IOSurface based ImageBufferBackends not support that?
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:462
> > +    if (!convertedData)
> > +        return nullptr;
> > +    return convertedData;
> 
> This null check doesn't really do anything. Just returning the result of
> convertedBuffer->getImageData(...) would be fine here. The caller will have
> to do a null check either way.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:480
> > +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Unpremultiplied);
> > +                copyImageBytes(*convertedImageData->data(), destination, rect);
> 
> Since convertImageBufferToColorSpace() can return null, this doesn't seem
> safe. (Same for a few other callers).


In replying to your comment on why I didn't use transformColorSpace: this function is a no-op on CG platforms, because the author assumed that all the filters will be interacting with the input on the ImageBuffer level, however, the filters that I mentioned does not go through that path - they pull the raw pixel data either from an ImageBuffer or directly from the {un/pre}multiplied ImageData pointer. This process does not go through CG color conversion. Therefore it needs to be handled separately inside copy{un/pre}multipliedData function where ImageData retrieval happens.

With regards your other comments: good call, null pointer check is indeed required. Thank you for the feedback

[Sam Weinig]

(In reply to guowei_yang from comment #31)
> (In reply to Sam Weinig from comment #30)
> > Comment on attachment 401395 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401395&action=review
> > 
> > What is the reason for not using ImageBuffer::transformColorSpace(srcSpace,
> > dstSpace)? Do the CG/IOSurface based ImageBufferBackends not support that?
> > 
> > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:462
> > > +    if (!convertedData)
> > > +        return nullptr;
> > > +    return convertedData;
> > 
> > This null check doesn't really do anything. Just returning the result of
> > convertedBuffer->getImageData(...) would be fine here. The caller will have
> > to do a null check either way.
> > 
> > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:480
> > > +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Unpremultiplied);
> > > +                copyImageBytes(*convertedImageData->data(), destination, rect);
> > 
> > Since convertImageBufferToColorSpace() can return null, this doesn't seem
> > safe. (Same for a few other callers).
> 
> 
> In replying to your comment on why I didn't use transformColorSpace: this
> function is a no-op on CG platforms, because the author assumed that all the
> filters will be interacting with the input on the ImageBuffer level,
> however, the filters that I mentioned does not go through that path - they
> pull the raw pixel data either from an ImageBuffer or directly from the
> {un/pre}multiplied ImageData pointer. This process does not go through CG
> color conversion. Therefore it needs to be handled separately inside
> copy{un/pre}multipliedData function where ImageData retrieval happens.

This is really making me feel like the real problem here is that the ImageBuffer abstraction needs to be fixed.

[Simon Fraser (smfr)]

Comment on attachment 401395 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:607
> +    if (usesCGPlatform()) {

Agree with other comments that say this could be a #if USE(CG). Or maybe the CG behavior should be the default, and the other ports should be fixed; after all, doing colorspace conversion on painting is something any reasonable graphics framework should be able to do.

(In reply to Sam Weinig from comment #32)
> (In reply to guowei_yang from comment #31)
> > (In reply to Sam Weinig from comment #30)
> > > Comment on attachment 401395 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=401395&action=review
> > > 
> > > What is the reason for not using ImageBuffer::transformColorSpace(srcSpace,
> > > dstSpace)? Do the CG/IOSurface based ImageBufferBackends not support that?
> > > 
> > > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:462
> > > > +    if (!convertedData)
> > > > +        return nullptr;
> > > > +    return convertedData;
> > > 
> > > This null check doesn't really do anything. Just returning the result of
> > > convertedBuffer->getImageData(...) would be fine here. The caller will have
> > > to do a null check either way.
> > > 
> > > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:480
> > > > +                RefPtr<ImageData> convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), m_imageBufferResult.get(), AlphaPremultiplication::Unpremultiplied);
> > > > +                copyImageBytes(*convertedImageData->data(), destination, rect);
> > > 
> > > Since convertImageBufferToColorSpace() can return null, this doesn't seem
> > > safe. (Same for a few other callers).
> > 
> > 
> > In replying to your comment on why I didn't use transformColorSpace: this
> > function is a no-op on CG platforms, because the author assumed that all the
> > filters will be interacting with the input on the ImageBuffer level,
> > however, the filters that I mentioned does not go through that path - they
> > pull the raw pixel data either from an ImageBuffer or directly from the
> > {un/pre}multiplied ImageData pointer. This process does not go through CG
> > color conversion. Therefore it needs to be handled separately inside
> > copy{un/pre}multipliedData function where ImageData retrieval happens.
> 
> This is really making me feel like the real problem here is that the
> ImageBuffer abstraction needs to be fixed.

Yes indeed. However that would require a huge amount of work and time to refactor, so I believe a good approach is to land this first to at least fix the issues, and then refactor the structure.

Created attachment 401414 [details]
Patch

Created attachment 401417 [details]
Patch

Created attachment 401418 [details]
Patch

[Darin Adler]

> What is the general rule of thumb for using those complier flags?
> I was under the impression that we want those flags as less as possible

The rule of thumb is that we want to create platform abstractions and write unconditional code that does not itself have platform specifics, rather than having different code for different platforms.

But, there is no preference for runtime checks "if (isCG)" over compile-time checks "#if USE(CG)". So adding an "is CG" function doesn't help.

We want to avoid having large stretches of code that aren’t used at all on certain platforms. This is our primary job. To create the right types of operations so platform conditionals aren’t needed. To understand platform differences conceptually, and either abstract them away entirely, or make them easy to understand concepts rather than names of platforms.

Created attachment 401479 [details]
Patch - resolved a merge conflict

[Darin Adler]

Comment on attachment 401479 [details]
Patch - resolved a merge conflict

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

Looks OK to me; Simon may also want to review again.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:460
> +    auto convertedData = convertedBuffer->getImageData(outputFormat, { IntPoint(), convertedBuffer->logicalSize() });
> +    return convertedData;

No need for the local variable here. Can just return.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:65
> +    

Please don’t add the whitespace here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:67
> +    void copyPremultipliedResult(Uint8ClampedArray& destination, const IntRect&,  Optional<ColorSpace> = WTF::nullopt);

Extra space before the word Optional here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:68
> +    

Please don’t add the trailing whitespace here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:184
> +    RefPtr<ImageData> convertImageDataToColorSpace(ColorSpace, RefPtr<ImageData>, AlphaPremultiplication);

It’s peculiar to take a RefPtr<ImageData> here. I’d expect it to be ImageData& instead. Implementation can’t handle a null.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:185
> +    RefPtr<ImageData> convertImageBufferToColorSpace(ColorSpace, ImageBuffer*, AlphaPremultiplication);

Argument type should be ImageBuffer&. Implementation can’t handle a null.

Created attachment 401482 [details]
Patch - addressed Darin's comments

[Darin Adler]

Comment on attachment 401482 [details]
Patch - addressed Darin's comments

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

> Source/WebCore/platform/graphics/filters/FilterEffect.h:181
> +    RefPtr<ImageData> convertImageDataToColorSpace(ColorSpace, RefPtr<ImageData>, AlphaPremultiplication);

Argument should be ImageData&, not RefPtr<ImageData>.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:182
> +    RefPtr<ImageData> convertImageBufferToColorSpace(ColorSpace, ImageBuffer*, AlphaPremultiplication);

Should be ImageBuffer&, not ImageBuffer*.

Created attachment 401496 [details]
Patch

[Darin Adler]

Comment on attachment 401496 [details]
Patch

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

r=me as is; some ideas for further improvement, and still keeping in mind Simon’s suggestion that we fine a better long term solution without using #if CG to compensate for the fact that we didn’t make a good enough abstraction.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:443
> +    auto inputImgBuffer = ImageBuffer::create(clampedSize, m_filter.renderingMode(), m_filter.filterScale(), operatingColorSpace());

WebKit coding style discourages using abbreviations like "img". I suggest just naming this local "buffer". I don’t think a longer name is important to help a reader understand the code or disambiguate anything.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:447
> +    return convertImageBufferToColorSpace(targetColorSpace, *inputImgBuffer.get(), outputFormat);

Should not need the ".get()" here.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:472
> +            if (requiresAdditionalColorSpaceConversion(colorSpace)) {

Seems we could factor this slightly better. It’s the same code as below in this function and as the code in copyPremultipliedResult.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:473
> +                auto convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), *m_imageBufferResult.get(), AlphaPremultiplication::Unpremultiplied);

Should not need the ".get()" here.

Also, not sure if others have the same preference, but I prefer the "*colorSpace" syntax to the "colorSpace.value()" syntax.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:500
> +    RefPtr<ImageData> convertedImageData;
> +    if (requiresAdditionalColorSpaceConversion(colorSpace))
> +        convertedImageData = convertImageDataToColorSpace(colorSpace.value(), *m_unmultipliedImageResult, AlphaPremultiplication::Unpremultiplied);
> +    else
> +        convertedImageData = m_unmultipliedImageResult;
> +    if (!convertedImageData)
> +        return;
> +    copyImageBytes(*convertedImageData->data(), destination, rect);

Seems that we could factor this better. This is the same epilogue as in copyPremultipliedResult just below.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:518
> +            if (requiresAdditionalColorSpaceConversion(colorSpace)) {
> +                auto convertedImageData = convertImageBufferToColorSpace(colorSpace.value(), *m_imageBufferResult.get(), AlphaPremultiplication::Premultiplied);
> +                if (!convertedImageData)
> +                    return;
> +                copyImageBytes(*convertedImageData->data(), destination, rect);
> +                return;
> +            }

Ditto.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:540
> +    RefPtr<ImageData> convertedImageData;
> +    if (requiresAdditionalColorSpaceConversion(colorSpace))
> +        convertedImageData = convertImageDataToColorSpace(colorSpace.value(), *m_premultipliedImageResult, AlphaPremultiplication::Premultiplied);
> +    else
> +        convertedImageData = m_premultipliedImageResult;
> +    if (!convertedImageData)
> +        return;
> +    copyImageBytes(*convertedImageData->data(), destination, rect);

Ditto.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:597
> +    if (!dstColorSpace || resultColorSpace() == dstColorSpace.value())
> +        return false;
> +    return true;

I would have written:

    return dstColorSpace && resultColorSpace() != *dstColorSpace;

[Simon Fraser (smfr)]

Comment on attachment 401496 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:441
> +    IntRect destinationRect(IntPoint(), m_absolutePaintRect.size());
> +    FloatSize clampedSize = ImageBuffer::clampedSize(absolutePaintRect().size());

I don't get why this is using m_absolutePaintRect rather than just the height/width fo the inputData. Why does it need to consult any geometry state from this FilterEffect?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:452
> +    FloatSize clampedSize = ImageBuffer::clampedSize(absolutePaintRect().size());

I don't get why this is using m_absolutePaintRect rather than just the height/width fo the inputBuffer. Why does it need to consult any geometry state from this FilterEffect?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:457
> +    FloatRect drawingRegion = drawingRegionOfInputImage(absolutePaintRect());

Why use this?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:589
> +bool FilterEffect::requiresAdditionalColorSpaceConversion(Optional<ColorSpace> dstColorSpace)

We should try to find a better word than "additional". I don't know what it means at the call sites.

[Said Abou-Hallawa]

I might be confused or missing something here but I think the approach of this patch is not right.

-- Having two different code path for color transformation does not seem right: (1) the conversion for non-CG in FilterEffect::transformResultColorSpace() and (2) the new conversion for CG in FilterEffect::copyUnmultipliedResult() and FilterEffect::copyPremultipliedResult().

-- I did not get why we have to keep the m_imageBufferResult in the original color space and have to convert it to the operating color space only in platformApplySoftware(). Why do we not convert the result ImageBuffer of all the FilterEffects in FilterEffect::apply() as none-CG platform do?

-- Can't this bug be fixed by implementing ImageBufferCGBackend::transformColorSpace() and fixing FilterEffect::transformResultColorSpace() to work for all platforms?

[Said Abou-Hallawa]

Comment on attachment 401496 [details]
Patch

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

> Source/WebCore/ChangeLog:67
> +        (WebCore::FilterEffect::transformResultColorSpace): removed compiler flag that 
> +              distinguishes CG platforms, replaced with function call FilterEffect::usesCGPlatform()

I think this comment is not correct.

(In reply to Said Abou-Hallawa from comment #46)
> I might be confused or missing something here but I think the approach of
> this patch is not right.
> 
> -- Having two different code path for color transformation does not seem
> right: (1) the conversion for non-CG in
> FilterEffect::transformResultColorSpace() and (2) the new conversion for CG
> in FilterEffect::copyUnmultipliedResult() and
> FilterEffect::copyPremultipliedResult().
> 
> -- I did not get why we have to keep the m_imageBufferResult in the original
> color space and have to convert it to the operating color space only in
> platformApplySoftware(). Why do we not convert the result ImageBuffer of all
> the FilterEffects in FilterEffect::apply() as none-CG platform do?
> 
> -- Can't this bug be fixed by implementing
> ImageBufferCGBackend::transformColorSpace() and fixing
> FilterEffect::transformResultColorSpace() to work for all platforms?

Sorry for the confusion, I think I do need to modify the ChangeLog to further explain the problem in details. 

First things first, the image information are passed from filters to filters in one of 3 forms: ImageBuffer (m_imageBufferResult), ImageData (m_premultipliedImageResult) and ImageData (m_unmultipliedImageResult). The two ImageData exists because some filters operates on unpremultiplied data, and others operates in premultiplied data. Having those two separate ImageData will optimize the conversion between the two (so you don't have to save read from ImageBuffer, convert from unmultiplied to premultiplied, and save back to ImageBuffer)

First let me explain the function: FilterEffect::transformResultColorSpace, 
1) on CG platforms, this is a no-op, because in fact, CG handles color space conversion when you write an CG-backed ImageBuffer to another CG-backed ImageBuffer
2) on Non-CG platforms, ImageBuffer::transformColorSpace calls the CARIO backend that has an in-place color space conversion code

Having said that, in theory, the current code setup should work correctly without any color space conversion confusions. 

HOWEVER, here's where things get interesting: the 7 filters I mentioned in this patch, feComponentTransfer, feComposite, feConvolveMatrix, feGaussianBlur, feMorphology, feSpecularLighting, feDiffuseLighting, they don't go through this simple ImageBuffer -> ImageBuffer path. Instead, they manipulate the input image in the pixel level. Those 7 filters access the input buffer by calling copy{pre/un}multipliedData(). 

Note that this exact behavior is the direct cause of "Having two different code path for color transformation". Now, those 7 filters wants pixel data, so it retrieves pixels stored in the form of ImageData by calling copy{pre/un}multipliedData(). BUT! This operation doesn't go through any color space conversion! This is the exact bug that this patch is trying to address. Also note that since CAIRO converted ImageBuffers in place in FilterEffect::apply(), so this step is not needed on CAIRO platforms. 

So now, let me try to answer your first question: on non-CG platform, everything is good. On CG platforms, for the 11 filters out of 18 SVG filters, we don't need any extra code to convert color space - CG-backed ImageBuffer handles it under the hood. The 7 filters I mentioned, extra work is needed because they used a different path to get input pixels, they didn't write ImageBuffer to ImageBuffer; they extracted the ImageData and manipulate it directly. 

The second question: CG handles color space conversion already, it's those 7 filters didn't use ImageBuffer, so they lost that support.

Created attachment 401587 [details]
Patch

[Darin Adler]

Comment on attachment 401587 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Reviewed by Myles C. Maxfield, Simon Fraser, Darin Adler

Reviewed by is repeated here.

Created attachment 401596 [details]
Patch

[Said Abou-Hallawa]

Thanks for the explanation. It was informative. But here is my opinion:

1) Unifying the code for m_imageBufferResult conversion for CG and none-CG will result in a cleaner and smaller patch.

2) Converting them_unmultipliedImageResult and m_premultipliedImageResult is fine and should fix the bug for all platforms.

(In reply to Said Abou-Hallawa from comment #52)
> Thanks for the explanation. It was informative. But here is my opinion:
> 
> 1) Unifying the code for m_imageBufferResult conversion for CG and none-CG
> will result in a cleaner and smaller patch.
> 
> 2) Converting them_unmultipliedImageResult and m_premultipliedImageResult is
> fine and should fix the bug for all platforms.

Thank you for the feedback! I intend to refactor the code so that color conversion and pixel data retrieval isn't this complicated. However that would require a deep refactor process and result in a huge patch for other parts of the code, which I think is worth a separate patch.

[EWS]

Committed r262893: <https://trac.webkit.org/changeset/262893>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401596 [details].

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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