Bug 55361

Summary: SVG 1.1: ineffectual transform attribute for ClipPath
Product: WebKit Reporter: Matthieu <m_guerquin>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, krit, rwlbuis, webkit.review.bot, zimmermann
Priority: P2    
Version: 412   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
clipPath with transform
none
Work in progress patch
none
Patch
none
Patch zimmermann: review+, webkit.review.bot: commit-queue-

Description Matthieu 2011-02-28 03:54:58 PST
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>
Comment 1 Dirk Schulze 2011-03-02 00:50:53 PST
Correct, clipPath is a SVGStyledTransformable element, therefor we need to deal with the transformation list. Thanks for opening the bug.
Comment 2 Dirk Schulze 2011-05-22 06:11:17 PDT
Created attachment 94340 [details]
clipPath with transform

clipPath with transform. There should be no red on the SVG.
Comment 3 Rob Buis 2011-05-23 18:57:14 PDT
Created attachment 94542 [details]
Work in progress patch
Comment 4 Dirk Schulze 2011-05-23 22:15:42 PDT
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?
Comment 5 Nikolas Zimmermann 2011-05-24 01:01:13 PDT
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).
Comment 6 Dirk Schulze 2011-05-24 01:03:25 PDT
(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.
Comment 7 Dirk Schulze 2011-05-24 01:08:00 PDT
(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()
Comment 8 Rob Buis 2011-05-24 07:18:38 PDT
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.
Comment 9 Dirk Schulze 2011-10-04 01:56:10 PDT
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.
Comment 10 Dirk Schulze 2011-10-04 10:18:21 PDT
Created attachment 109638 [details]
Patch
Comment 11 Nikolas Zimmermann 2011-10-04 12:23:59 PDT
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...
Comment 12 Dirk Schulze 2011-10-05 05:39:22 PDT
Created attachment 109778 [details]
Patch
Comment 13 Dirk Schulze 2011-10-05 05:46:33 PDT
localToParentTransform() is not implemented for RenderSVGResourceClipper. Like discussed with Niko on IRC I left this out for now.
Comment 14 Nikolas Zimmermann 2011-10-05 06:41:02 PDT
Comment on attachment 109778 [details]
Patch

r=me. But please fix up the ChangeLog a bit, as mentioned in an earlier comment.
Comment 15 WebKit Review Bot 2011-10-05 07:15:43 PDT
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
Comment 16 Dirk Schulze 2011-10-05 09:19:46 PDT
Committed r96712: <http://trac.webkit.org/changeset/96712>