Bug 137856

Summary: feComposite filter does not clip the paint rect to its effect rect when the operator is 'in' or 'atop'
Product: WebKit Reporter: Ridley Combs <rcombs>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, arnavion, commit-queue, dino, kondapallykalyan, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=123282
Attachments:
Description Flags
Example of multiple lines of text affected by the issue (on MediaCrush)
none
Patch
none
test case
none
Patch none

Description Ridley Combs 2014-10-18 14:38:46 PDT
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
Comment 1 Radar WebKit Bug Importer 2014-10-28 23:52:00 PDT
<rdar://problem/18807808>
Comment 2 Radar WebKit Bug Importer 2014-10-28 23:52:01 PDT
<rdar://problem/18807809>
Comment 3 Said Abou-Hallawa 2015-06-01 11:17:50 PDT
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.
Comment 4 Alexey Proskuryakov 2015-06-01 11:49:09 PDT
I can reproduce the issue on jsfiddle using Safari 7.1.6.
Comment 5 Alexey Proskuryakov 2015-06-01 11:51:30 PDT
Happens with the current nightly too.
Comment 6 Said Abou-Hallawa 2015-06-08 13:05:12 PDT
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.
Comment 7 Said Abou-Hallawa 2015-06-08 13:07:14 PDT
*** Bug 139237 has been marked as a duplicate of this bug. ***
Comment 8 Said Abou-Hallawa 2015-06-08 14:51:19 PDT
Created attachment 254513 [details]
Patch
Comment 9 Said Abou-Hallawa 2015-06-08 17:13:27 PDT
Created attachment 254527 [details]
test case
Comment 10 Said Abou-Hallawa 2015-06-09 08:41:57 PDT
(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.
Comment 11 Darin Adler 2015-06-09 11:40:22 PDT
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.
Comment 12 Said Abou-Hallawa 2015-06-09 13:37:01 PDT
Created attachment 254598 [details]
Patch
Comment 13 WebKit Commit Bot 2015-06-09 16:24:45 PDT
Comment on attachment 254598 [details]
Patch

Clearing flags on attachment: 254598

Committed r185392: <http://trac.webkit.org/changeset/185392>
Comment 14 WebKit Commit Bot 2015-06-09 16:24:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Said Abou-Hallawa 2015-07-31 13:04:57 PDT
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.
Comment 16 Said Abou-Hallawa 2015-08-03 14:09:13 PDT
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.