Bug 77660 - SVG Composite of Offset filters incorrectly clips
Summary: SVG Composite of Offset filters incorrectly clips
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-02 11:12 PST by Stephen Chenney
Modified: 2012-06-15 08:37 PDT (History)
7 users (show)

See Also:


Attachments
Test case, suitable for LayoutTest (1.56 KB, image/svg+xml)
2012-02-02 11:13 PST, Stephen Chenney
no flags Details
Simpler repro case (710 bytes, image/svg+xml)
2012-06-05 13:16 PDT, Stephen Chenney
no flags Details
Patch (7.69 KB, patch)
2012-06-07 10:53 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (570.76 KB, application/zip)
2012-06-07 16:38 PDT, WebKit Review Bot
no flags Details
Patch (7.73 KB, patch)
2012-06-13 11:37 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2012-06-14 11:35 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (8.41 KB, patch)
2012-06-14 12:27 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Chenney 2012-02-02 11:12:44 PST
See the attached test case. With a filter tree that composites an offset image, the filter region is clearly incorrect. It appears to be a tight bound rather than the necessary default 120% region. The example works on Firefox.

This may, or may not, be Chromium specific. It looks like the preview on my Ubuntu box is correct.
Comment 1 Stephen Chenney 2012-02-02 11:13:25 PST
Created attachment 125154 [details]
Test case, suitable for LayoutTest
Comment 2 Stephen Chenney 2012-02-02 11:14:04 PST
(In reply to comment #0)
> See the attached test case. With a filter tree that composites an offset image, the filter region is clearly incorrect. It appears to be a tight bound rather than the necessary default 120% region. The example works on Firefox.
> 
> This may, or may not, be Chromium specific. It looks like the preview on my Ubuntu box is correct.

Nope, the Ubuntu behavior is the same as the Opera behavior, which is off by one pixel for some reason.
Comment 3 Stephen Chenney 2012-02-03 09:26:36 PST
Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=112589
Comment 4 Stephen Chenney 2012-04-16 12:48:50 PDT
On Chromium this test also fails. May or may not be related:
svg/filters/shadow-on-rect-with-filter.svg
Comment 5 Stephen Chenney 2012-05-23 09:20:13 PDT
The issue appears to be that the image buffer for intermediate filter results is sized to the filter target's bounding box, not the filter's paint region clipped to the filter's filter region. This is just plain wrong. The paint region for the final result should be the intersection of the filter region with the effect subregions and the actual painted pixels for each filter step.
Comment 6 Dirk Schulze 2012-05-24 10:07:17 PDT
(In reply to comment #5)
> The issue appears to be that the image buffer for intermediate filter results is sized to the filter target's bounding box, not the filter's paint region clipped to the filter's filter region. This is just plain wrong. The paint region for the final result should be the intersection of the filter region with the effect subregions and the actual painted pixels for each filter step.

Does this just happen when you filter the parent of the filtered element? The example looks quite complex. As far as i know, just applying the offset to an element does not cause these problems. That is why I am wondering. Can you reduce the example more please?
Comment 7 Stephen Chenney 2012-06-05 13:16:08 PDT
Created attachment 145859 [details]
Simpler repro case

The spec has this very explicit statement about the behavior of filters.
------------
3.7 Filtering painted regions

SVG allows any painting operation to be filtered. (See Filter Effects.)

In this case the result must be as though the paint operations had been applied to an intermediate canvas initialized to transparent black, of a size determined by the rules given in Filter Effects then filtered by the processes defined in Filter Effects.
------------

Our bug is to only filter the stroke bounding box of any element. This is fine for simple elements, where the stroke bounding box contains all of the pixels but not the result of filters, clips or masks. However, it is wrong for containers, where the stroke bounding rect does not include the effect of filters, clips etc applied to the children of the group.

As I read the spec, for a container the filter applies to the complete paint of the container's contents, including filtering and clipping of each child. It's like you called paint on the container without it's own resources (filter, clip, etc), put the result into the filter region buffer, then filter the result. That means we need to use the union of the child repaint rects as the container's painted region for the purposes of filtering.

Otherwise, it would be possible for a filter to pull in pixels clipped off a child of the group. It would also make it impossible to correctly filter an already filtered child.

Patch forthcoming, as soon as I make the ref-test result for this.
Comment 8 Stephen Chenney 2012-06-07 10:53:46 PDT
Created attachment 146323 [details]
Patch
Comment 9 Stephen Chenney 2012-06-07 10:54:41 PDT
Note that the test is expected to fail on some platforms before the fix for https://bugs.webkit.org/show_bug.cgi?id=80321 is committed.
Comment 10 WebKit Review Bot 2012-06-07 16:38:47 PDT
Comment on attachment 146323 [details]
Patch

Attachment 146323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12907786

New failing tests:
svg/filters/container-with-filters.svg
Comment 11 WebKit Review Bot 2012-06-07 16:38:51 PDT
Created attachment 146413 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 12 Stephen Chenney 2012-06-13 11:37:26 PDT
Created attachment 147375 [details]
Patch

New patch to retry EWS, but otherwise the same
Comment 13 Dirk Schulze 2012-06-13 15:44:11 PDT
Comment on attachment 147375 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> +    if (object->isSVGContainer()) {
> +        FloatRect targetRegion;
> +        for (RenderObject* current = object->firstChild(); current; current = current->nextSibling()) {
> +            if (current->isSVGHiddenContainer())
> +                continue;
> +
> +            const AffineTransform& transform = current->localToParentTransform();
> +            if (transform.isIdentity())
> +                targetRegion.unite(current->repaintRectInLocalCoordinates());
> +            else
> +                targetRegion.unite(transform.mapRect(current->repaintRectInLocalCoordinates()));
> +        }
> +        return targetRegion;
> +    }

Why not just the repaintRectInLocalCoordinates of the group element? Isn't it that what you want?
Comment 14 Stephen Chenney 2012-06-13 16:05:42 PDT
(In reply to comment #13)
> (From update of attachment 147375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147375&action=review
> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> > +    if (object->isSVGContainer()) {
> > +        FloatRect targetRegion;
> > +        for (RenderObject* current = object->firstChild(); current; current = current->nextSibling()) {
> > +            if (current->isSVGHiddenContainer())
> > +                continue;
> > +
> > +            const AffineTransform& transform = current->localToParentTransform();
> > +            if (transform.isIdentity())
> > +                targetRegion.unite(current->repaintRectInLocalCoordinates());
> > +            else
> > +                targetRegion.unite(transform.mapRect(current->repaintRectInLocalCoordinates()));
> > +        }
> > +        return targetRegion;
> > +    }
> 
> Why not just the repaintRectInLocalCoordinates of the group element? Isn't it that what you want?

Nope. The repaint rect of the group include the resources applied to the group, which include the filter we are trying to size. We want the total repaint of all the elements in the group, but not the resources of the group itself.

A bigger issue is actually clip regions applied along with a filter to a group. The spec says filter, then clip, then mask (I think) and so we must not use the clipped region when applying the filter.
Comment 15 Dirk Schulze 2012-06-13 18:39:40 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 147375 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147375&action=review
> > 
> > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> > > +    if (object->isSVGContainer()) {
> > > +        FloatRect targetRegion;
> > > +        for (RenderObject* current = object->firstChild(); current; current = current->nextSibling()) {
> > > +            if (current->isSVGHiddenContainer())
> > > +                continue;
> > > +
> > > +            const AffineTransform& transform = current->localToParentTransform();
> > > +            if (transform.isIdentity())
> > > +                targetRegion.unite(current->repaintRectInLocalCoordinates());
> > > +            else
> > > +                targetRegion.unite(transform.mapRect(current->repaintRectInLocalCoordinates()));
> > > +        }
> > > +        return targetRegion;
> > > +    }
> > 
> > Why not just the repaintRectInLocalCoordinates of the group element? Isn't it that what you want?
> 
> Nope. The repaint rect of the group include the resources applied to the group, which include the filter we are trying to size. We want the total repaint of all the elements in the group, but not the resources of the group itself.
> 
> A bigger issue is actually clip regions applied along with a filter to a group. The spec says filter, then clip, then mask (I think) and so we must not use the clipped region when applying the filter.

We mainly introduced strokeBoundingRect because of filters. Maybe the logic should got to the renderer of the group. Or is strokeBoundingRect used for other things now?
Comment 16 Stephen Chenney 2012-06-14 04:28:07 PDT
I'll check on the usage of strokeBoundingRect - it did occur to me to change it but I thought it woud have other implications.
Comment 17 Stephen Chenney 2012-06-14 11:35:10 PDT
Created attachment 147624 [details]
Patch
Comment 18 Stephen Chenney 2012-06-14 11:37:32 PDT
I've looked at the use of strokeBoundingBox and we can modify it's behavior for containers, as I have done here. The impact, aside form fixing the filters problem, is to account for resources applied to container children when fitting filters and inline content around containers. That seems to me like the correct behavior.
Comment 19 Stephen Chenney 2012-06-14 12:27:49 PDT
Created attachment 147629 [details]
Patch
Comment 20 Dirk Schulze 2012-06-14 13:28:38 PDT
Comment on attachment 147629 [details]
Patch

great patch! r=me
Comment 21 WebKit Review Bot 2012-06-15 08:37:30 PDT
Comment on attachment 147629 [details]
Patch

Clearing flags on attachment: 147629

Committed r120464: <http://trac.webkit.org/changeset/120464>
Comment 22 WebKit Review Bot 2012-06-15 08:37:35 PDT
All reviewed patches have been landed.  Closing bug.