Bug 55361 – SVG 1.1: ineffectual transform attribute for ClipPath

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

[Matthieu]

I observed unexpected display of SVG pictures using Chrome/Safari/Webkit (cf. attached picture). Non webkit-based browser, e.g. Opera/Firefox, do display the figure correctly.

The cause might be that the transform attribute for clippath is unimplemented or ineffectual. In fact, in the attached test SVG, removing the transform attribute line 7 has no effect on the display.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg" version="1.1" width="512" height="512">
<g>
<ellipse id="ell1" cx="200" cy="256" rx="20" ry="100" transform="rotate(-18,200,256)" style="fill:rgb(0%,0%,0%);stroke:none"/>
<ellipse id="ell2" cx="256" cy="256" rx="50" ry="60" style="fill:rgb(30%,30%,30%);stroke:none"/>
<clipPath id="ell1-ell2" transform="rotate(18,200,256) scale(2,2)">
<ellipse id="ell2-2" cx="256" cy="256" rx="50" ry="60" style="fill:rgb(30%,30%,30%);stroke:none"/>
</clipPath>
<ellipse clip-path="url(#ell1-ell2)" cx="200" cy="256" rx="20" ry="100" transform="rotate(-18,200,256)" style="fill:rgb(100%,100%,100%);stroke:none"/>
</g>
</svg>

[Dirk Schulze]

Correct, clipPath is a SVGStyledTransformable element, therefor we need to deal with the transformation list. Thanks for opening the bug.

[Dirk Schulze]

Created attachment 94340 [details]
clipPath with transform

clipPath with transform. There should be no red on the SVG.

[Rob Buis]

Created attachment 94542 [details]
Work in progress patch

[Dirk Schulze]

Comment on attachment 94542 [details]
Work in progress patch

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

> LayoutTests/svg/custom/transformed-clip-path.svg:6
> +<clipPath id="clip" clipPathUnits="objectBoundingBox" transform="scale(2)">
> +    <rect width="50%" height="50%"/>
> +    <!-- second rect causes masking -->
> +</clipPath>

If this is for testing the fastClip option, I'm fine but please remove the comment. Nevertheless we need a test where we switch to masking, thats why I added a second rect (which you removed). Please also take the comment below into account. This test is in the wrong folder, we have a own folder for clipping tests.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:144
> +    clipPath.transform(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());

This looks ok.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
> +        maskContext->concatCTM(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());

Shouldn't that happen before we create the imageBuffer to get the correct size? Imagine a scale of 10 or 1000. Wouldn't we get pixelation here? Can you add a test case with such a scale level?

[Nikolas Zimmermann]

Comment on attachment 94542 [details]
Work in progress patch

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

> LayoutTests/platform/mac/svg/custom/transformed-clip-path-expected.txt:1
> +layer at (0,0) size 800x600

The corresponding png is broken or missing in the review tool...

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146
> +    clipPath.transform(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
>      // Only one visible shape/path was found. Directly continue clipping and transform the content to userspace if necessary.
>      if (static_cast<SVGClipPathElement*>(node())->clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {

Can you store the SVGClipPathElement in a local variable, to save the casting several times...

>> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
>> +        maskContext->concatCTM(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
> 
> Shouldn't that happen before we create the imageBuffer to get the correct size? Imagine a scale of 10 or 1000. Wouldn't we get pixelation here? Can you add a test case with such a scale level?

That's correct, the animatedLocalTransform needs to be taken into account when creating the ImageBuffer, see the calculateTransformToOutermostSVGRenderer (or however it was named) in SVGImageBufferTools. Let's see if you can create a testcase first, with this patch, that leads to pixelation (easiest visible with text).

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 94542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94542&action=review
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146
> > +    clipPath.transform(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
> >      // Only one visible shape/path was found. Directly continue clipping and transform the content to userspace if necessary.
> >      if (static_cast<SVGClipPathElement*>(node())->clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> 
> Can you store the SVGClipPathElement in a local variable, to save the casting several times...
These are two different code paths. You'll not end up in casting the node twice during runtime.

[Dirk Schulze]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 94542 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=94542&action=review
> > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146
> > > +    clipPath.transform(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
> > >      // Only one visible shape/path was found. Directly continue clipping and transform the content to userspace if necessary.
> > >      if (static_cast<SVGClipPathElement*>(node())->clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> > 
> > Can you store the SVGClipPathElement in a local variable, to save the casting several times...
> These are two different code paths. You'll not end up in casting the node twice during runtime.

Checked it again, it can happen that you end up casting the node twice, sorry. Maybe you can ask for the transform at the beginning of applyClippingToContext()

[Rob Buis]

Hi Dirk,

BTW I just wanted to show a patch since this work has been lingering (started the weekend you were in Toronto) because of the mask case being tricky, so I just thought I'd show the simple clipping to check on that.

(In reply to comment #4)
> (From update of attachment 94542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94542&action=review
> 
> > LayoutTests/svg/custom/transformed-clip-path.svg:6
> > +<clipPath id="clip" clipPathUnits="objectBoundingBox" transform="scale(2)">
> > +    <rect width="50%" height="50%"/>
> > +    <!-- second rect causes masking -->
> > +</clipPath>
> 
> If this is for testing the fastClip option, I'm fine but please remove the comment. Nevertheless we need a test where we switch to masking, thats why I added a second rect (which you removed). Please also take the comment below into account. This test is in the wrong folder, we have a own folder for clipping tests.

Points taken! My idea is to make two tests, one simple(clip) and complex(mask). The comment should go from the simple test indeed.

> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:144
> > +    clipPath.transform(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
> 
> This looks ok.
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
> > +        maskContext->concatCTM(static_cast<SVGClipPathElement*>(node())->animatedLocalTransform());
> 
> Shouldn't that happen before we create the imageBuffer to get the correct size? Imagine a scale of 10 or 1000. Wouldn't we get pixelation here? Can you add a test case with such a scale level?

Ah, that was maybe what I was missing! Is there any limiting being done for the imageBuffer or do I need to do that myself? Obviously taking large scale values is dangerous when allocating, but I think this is probably fixed in other places (like patterns).
So I'll keep working on a new patch, thanks!
Cheers,

Rob.

[Dirk Schulze]

Taking over this bug. Is is more to do than just the visual component. Repaint rect and hit testing must be taken into account as well. I have a patch that fixes everything but masking. Working on masking now.

[Dirk Schulze]

Created attachment 109638 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 109638 [details]
Patch

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

In general this looks good, I just think querying animatedLocalTransform() itself is unnecessary.

> Source/WebCore/ChangeLog:8
> +        Apply transform to clip path. If a image mask is used the mask context gets transformed.

"Respect 'transform' attribute/property for <clip-path>" sounds cleaner
"If the masking code path is used the mask context gets transformed, otherwise the path itself."

> Source/WebCore/ChangeLog:18
> +        (WebCore::RenderSVGResourceClipper::calculateClipContentRepaintRect): Repaint rect must get concatenated with current animation transform.

with the current animated transformation.

> Source/WebCore/ChangeLog:19
> +        (WebCore::RenderSVGResourceClipper::hitTestClipContent): Point for hit testing must be transformed to current animation transform.

transformed by the current animated transformation.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:170
> +    AffineTransform animatedLocalTransform = static_cast<SVGClipPathElement*>(node())->animatedLocalTransform();

This info should be present in the renderer() of the Clipper, see next comment below.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:301
> +    m_clipBoundaries = static_cast<SVGClipPathElement*>(node())->animatedLocalTransform().mapRect(m_clipBoundaries);

That looks a bit suspicious.
The clipBoundaries are computed by traversing through node()->firstChild().
Though the localToParentTransform() of the (node()->) renderer() also contains the animatedLocalTransform() there should be no need to ask SVGClipPathElement for this information.
This would be a step backwards. can you investigate?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:318
> +    point = clipPathElement->animatedLocalTransform().inverse().mapPoint(point);

Same comment as above.

> LayoutTests/ChangeLog:8
> +        Test that the clip path gets concatenated with current animation transform and

...with the current animated transformation...

[Dirk Schulze]

Created attachment 109778 [details]
Patch

[Dirk Schulze]

localToParentTransform() is not implemented for RenderSVGResourceClipper. Like discussed with Niko on IRC I left this out for now.

[Nikolas Zimmermann]

Comment on attachment 109778 [details]
Patch

r=me. But please fix up the ChangeLog a bit, as mentioned in an earlier comment.

[WebKit Review Bot]

Comment on attachment 109778 [details]
Patch

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

New failing tests:
svg/dynamic-updates/SVGClipPathElement-transform-influences-hitTesting.html
svg/clip-path/clip-path-transform-1.svg
svg/clip-path/clip-path-transform-2.svg

[Dirk Schulze]

Committed r96712: <http://trac.webkit.org/changeset/96712>