Bug 137856 – feComposite filter does not clip the paint rect to its effect rect when the operator is 'in' or 'atop'

• Component: SVG
• Status: RESOLVED FIXED

[Ridley Combs]

Created attachment 240070 [details]
Example of multiple lines of text affected by the issue (on MediaCrush)

This seems like it might be related to https://code.google.com/p/chromium/issues/detail?id=391200
The issue is visible in http://jsfiddle.net/jhmsh/
I've reproduced on Safari Version 8.0 (10600.1.25) on OS X 10.10 GM, the latest WebKit nightly [r174761], and iOS 8.1.
I first noticed it happening with libjass in the subtitles on this video: https://mediacru.sh/6tfk1_DgcFFM

[Radar WebKit Bug Importer]

<rdar://problem/18807808>

[Radar WebKit Bug Importer]

<rdar://problem/18807809>

[Said Abou-Hallawa]

I can't reproduce this bug. The link https://mediacru.sh/6tfk1_DgcFFM is not working. And the page http://jsfiddle.net/jhmsh/ displays the text clearly. The only problem I see there in this page is the width of the border of the text is always 1 pixel regardless of the zooming factor. This seems to be different from what other browsers do.

[Alexey Proskuryakov]

I can reproduce the issue on jsfiddle using Safari 7.1.6.

[Alexey Proskuryakov]

Happens with the current nightly too.

[Said Abou-Hallawa]

I have found out that the problem is in calculating the absolutePaintRect of the feComposite filter when the operator is either 'in' or 'atop'. We were setting it to the absolutePaintRect of the background FilterEffect without clipping it to the maxEffectRect which we do with other non-arthimtaic operators.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 254513 [details]
Patch

[Said Abou-Hallawa]

Created attachment 254527 [details]
test case

[Said Abou-Hallawa]

(In reply to comment #0)
> Created attachment 240070 [details]
> Example of multiple lines of text affected by the issue (on MediaCrush)
> 
> This seems like it might be related to
> https://code.google.com/p/chromium/issues/detail?id=391200
> The issue is visible in http://jsfiddle.net/jhmsh/
> I've reproduced on Safari Version 8.0 (10600.1.25) on OS X 10.10 GM, the
> latest WebKit nightly [r174761], and iOS 8.1.
> I first noticed it happening with libjass in the subtitles on this video:
> https://mediacru.sh/6tfk1_DgcFFM

The blink change is wrong. We should not stretch when drawing from the source FilterEffects images to the result image. We should be using the absolute paint size of the input FilterEffect as the source we want to draw. And I guess that’s why this change was deleted from Blink.

[Darin Adler]

Comment on attachment 254513 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:295
> +        IntPoint destinationPoint(destinationRect.location() - absolutePaintRect().location());

This is hard to understand. We have destinationRect and destinationPoint, with nearly identical names, but in different coordinate systems.

Separately, I think these would read more clearly with "=" syntax:

    IntPoint destinationPoint = destinationRect.location() - absolutePaintRect().location();

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:298
> +        IntRect sourceRect(IntPoint(destinationRect.location() - in->absolutePaintRect().location()), destinationRect.size());
> +        IntRect source2Rect(IntPoint(destinationRect.location() - in2->absolutePaintRect().location()), destinationRect.size());

Seems like we should add:

    IntRect operator-(IntRect, IntPoint);
    IntRect& operator-=(IntRect&, IntPoint);

Then we could write this way more cleanly. I would write something more like this:

    IntRect adjustedDestinationRect = destinationRect - absolutePaintRect().location();
    filterContext->drawImageBuffer(imageBuffer2, ColorSpaceDeviceRGB, adjustedDestinationRect, destinationRect - in2->absolutePaintRect().location());
    filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, adjustedDestinationRect, destinationRect - in->absolutePaintRect().location(), CompositeSourceIn);

Or something like that which still uses local variables, but like this:

    IntRect source2Rect = destinationRect - in->absolutePaintRect().location();

This syntax is much easier to read I think.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:59
> +    for (auto effect : m_inputEffects)

Should use auto& here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:181
>  
> +    void clipAbsolutePaintRect();
>  private:

Should have blank line below this and before private.

[Said Abou-Hallawa]

Created attachment 254598 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254598 [details]
Patch

Clearing flags on attachment: 254598

Committed r185392: <http://trac.webkit.org/changeset/185392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

The fix http://trac.webkit.org/changeset/185392 was not tried on Retina display. The display is fixed on non-Retina display. No corruption is shown when opening the test case http://jsfiddle.net/jhmsh/ on non Retina display. The attached test case https://bug-137856-attachments.webkit.org/attachment.cgi?id=254527 is displaying fine on all kids of display. Without this fix, the test case http://jsfiddle.net/jhmsh/ displays image corruption with all kids of display especially when zooming in.  So the bug is have-way fixed and it should not be a security bug.

[Said Abou-Hallawa]

The problem is in displaying feMorphology filter on Retina display. The bug 
https://bugs.webkit.org/show_bug.cgi?id=147589 was created to track this separate issue. So resolving this one as fixed again.