Summary: | SVG 1.1: ineffectual transform attribute for ClipPath | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthieu <m_guerquin> | ||||||||||
Component: | SVG | Assignee: | 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
Matthieu
2011-02-28 03:54:58 PST
Correct, clipPath is a SVGStyledTransformable element, therefor we need to deal with the transformation list. Thanks for opening the bug. Created attachment 94340 [details]
clipPath with transform
clipPath with transform. There should be no red on the SVG.
Created attachment 94542 [details]
Work in progress patch
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 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). (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. (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() 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. 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. Created attachment 109638 [details]
Patch
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... Created attachment 109778 [details]
Patch
localToParentTransform() is not implemented for RenderSVGResourceClipper. Like discussed with Niko on IRC I left this out for now. Comment on attachment 109778 [details]
Patch
r=me. But please fix up the ChangeLog a bit, as mentioned in an earlier comment.
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 Committed r96712: <http://trac.webkit.org/changeset/96712> |