Bug 55361 - SVG 1.1: ineffectual transform attribute for ClipPath
: SVG 1.1: ineffectual transform attribute for ClipPath
Status: RESOLVED FIXED
: WebKit
SVG
: 412
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-02-28 03:54 PST by
Modified: 2011-10-05 09:19 PST (History)


Attachments
clipPath with transform (414 bytes, image/svg+xml)
2011-05-22 06:11 PST, Dirk Schulze
no flags Details
Work in progress patch (5.26 KB, patch)
2011-05-23 18:57 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (114.44 KB, patch)
2011-10-04 10:18 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (114.45 KB, patch)
2011-10-05 05:39 PST, Dirk Schulze
zimmermann: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2011-05-22 06:11:17 PST -------
Created an attachment (id=94340) [details]
clipPath with transform

clipPath with transform. There should be no red on the SVG.
------- Comment #3 From 2011-05-23 18:57:14 PST -------
Created an attachment (id=94542) [details]
Work in progress patch
------- Comment #4 From 2011-05-23 22:15:42 PST -------
(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.

> 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 From 2011-05-24 01:01:13 PST -------
(From update of attachment 94542 [details])
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 From 2011-05-24 01:03:25 PST -------
(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.
------- Comment #7 From 2011-05-24 01:08:00 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 94542 [details] [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 From 2011-05-24 07:18:38 PST -------
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] [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 From 2011-10-04 01:56:10 PST -------
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 From 2011-10-04 10:18:21 PST -------
Created an attachment (id=109638) [details]
Patch
------- Comment #11 From 2011-10-04 12:23:59 PST -------
(From update of attachment 109638 [details])
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 From 2011-10-05 05:39:22 PST -------
Created an attachment (id=109778) [details]
Patch
------- Comment #13 From 2011-10-05 05:46:33 PST -------
localToParentTransform() is not implemented for RenderSVGResourceClipper. Like discussed with Niko on IRC I left this out for now.
------- Comment #14 From 2011-10-05 06:41:02 PST -------
(From update of attachment 109778 [details])
r=me. But please fix up the ChangeLog a bit, as mentioned in an earlier comment.
------- Comment #15 From 2011-10-05 07:15:43 PST -------
(From update of attachment 109778 [details])
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 From 2011-10-05 09:19:46 PST -------
Committed r96712: <http://trac.webkit.org/changeset/96712>