Bug 77660 – SVG Composite of Offset filters incorrectly clips

• Component: SVG
• Status: RESOLVED FIXED

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 125154 [details]
Test case, suitable for LayoutTest

[Stephen Chenney]

(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.

[Stephen Chenney]

Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=112589

[Stephen Chenney]

On Chromium this test also fails. May or may not be related:
svg/filters/shadow-on-rect-with-filter.svg

[Stephen Chenney]

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.

[Dirk Schulze]

(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?

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 146323 [details]
Patch

[Stephen Chenney]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Stephen Chenney]

Created attachment 147375 [details]
Patch

New patch to retry EWS, but otherwise the same

[Dirk Schulze]

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?

[Stephen Chenney]

(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.

[Dirk Schulze]

(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?

[Stephen Chenney]

I'll check on the usage of strokeBoundingRect - it did occur to me to change it but I thought it woud have other implications.

[Stephen Chenney]

Created attachment 147624 [details]
Patch

[Stephen Chenney]

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.

[Stephen Chenney]

Created attachment 147629 [details]
Patch

[Dirk Schulze]

Comment on attachment 147629 [details]
Patch

great patch! r=me

[WebKit Review Bot]

Comment on attachment 147629 [details]
Patch

Clearing flags on attachment: 147629

Committed r120464: <http://trac.webkit.org/changeset/120464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.