Bug 111587 – SVG Pattern pixelated on inline SVG with CSS transforms

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Created attachment 191761 [details]
Pixelated pattern caused by scaling HTML elements

If an SVG document is embedded inline into an HTML element, SVG patterns of this SVG document do not respect the transform of ancestor HTML elements. This causes pixelation.

[Florin Malita]

This is most likely affecting clips & masks also, as they all use SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem() when calculating the image buffer size.

[Dirk Schulze]

(In reply to comment #1)
> This is most likely affecting clips & masks also, as they all use SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem() when calculating the image buffer size.

Could you find a way to get the transformation matrix from SVG root to page transform? I think you said need to go to the tree and calculate the transform manually, right?

[Florin Malita]

(In reply to comment #2)
> (In reply to comment #1)
> > This is most likely affecting clips & masks also, as they all use SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem() when calculating the image buffer size.
> 
> Could you find a way to get the transformation matrix from SVG root to page transform? I think you said need to go to the tree and calculate the transform manually, right?

Yes, I do have a patch that continues walking up the layer tree from SVGRoot and picks up the affine portion of CSS transforms.

Seems to work and fixes the problem at hand, but I was just trying to figure whether there's a more straightforward approach or if there are some strange interactions with more complex CSS transforms.

It may be good enough though, so I'll add a test and put it up for review.

[Florin Malita]

Created attachment 192265 [details]
Patch

[WebKit Review Bot]

Attachment 192265 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/custom/resources-css-scaled.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h']" exit_code: 1
LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

Created attachment 192266 [details]
Same problem with clips, masks & filters.

[Simon Fraser (smfr)]

Comment on attachment 192265 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:215
> +    for (RenderLayer* layer = renderer->enclosingLayer(); layer; layer = layer->parent()) {
> +        TransformationMatrix* layerTransform = layer->transform();
> +        if (layerTransform)
> +            absoluteTransform = layerTransform->toAffineTransform() * absoluteTransform;

I think this should probably stop at a composited layer. Then the resolution will match that of the composited layer contents (but we might have an extra scale to take into account when we fix bug 27684).

[Florin Malita]

Created attachment 192279 [details]
Patch

Thanks Simon, updated.

[WebKit Review Bot]

Attachment 192279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/custom/resources-css-scaled.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h']" exit_code: 1
LayoutTests/platform/chromium-linux/svg/custom/resources-css-scaled-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 192279 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:202
> +    for (; renderer; renderer = renderer->parent()) {

For these kind of loops I like while more and it is used in a lot of other places as well. But I guess it is a matter of taste :)

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:212
> +    // Continue walking up the layer tree, accumulating CSS transforms.
> +    for (RenderLayer* layer = renderer->enclosingLayer(); layer; layer = layer->parent()) {

What if the SVG root is in a foreignObject? Maybe the check above should be updated to look if there is a parent SVG renderer?

> LayoutTests/ChangeLog:8
> +        * platform/chromium-linux/svg/custom/resources-css-scaled-expected.png: Added.

You should be able to create a ref test instead of the image.

[Philip Rogers]

Comment on attachment 192279 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)

I don't think we are doing the right thing here.

What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.

[Florin Malita]

(In reply to comment #11)
> (From update of attachment 192279 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> 
> > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> > +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)
> 
> I don't think we are doing the right thing here.
> 
> What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.

The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.

[Philip Rogers]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 192279 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> > 
> > > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> > > +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)
> > 
> > I don't think we are doing the right thing here.
> > 
> > What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.
> 
> The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.

On every paint you're going to crawl up the entire tree doing matrix multiplies at each step so the QT port doesn't break? Lets just fix QT's getCTM() if necessary :)

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 192279 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> > > 
> > > > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> > > > +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)
> > > 
> > > I don't think we are doing the right thing here.
> > > 
> > > What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.
> > 
> > The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.
> 
> On every paint you're going to crawl up the entire tree doing matrix multiplies at each step so the QT port doesn't break? Lets just fix QT's getCTM() if necessary :)

Nothing guarantees that CTM of 1,0,0,1,0,0 is in real screen coordinates. I don't think that helps.

[Philip Rogers]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (From update of attachment 192279 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> > > > 
> > > > > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> > > > > +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)
> > > > 
> > > > I don't think we are doing the right thing here.
> > > > 
> > > > What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.
> > > 
> > > The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.
> > 
> > On every paint you're going to crawl up the entire tree doing matrix multiplies at each step so the QT port doesn't break? Lets just fix QT's getCTM() if necessary :)
> 
> Nothing guarantees that CTM of 1,0,0,1,0,0 is in real screen coordinates. I don't think that helps.

My thinking is that we should be rasterizing in layer coordinates, not screen coordinates.

Maybe that is not best for users? Is this specified?

[Dirk Schulze]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (In reply to comment #11)
> > > > > (From update of attachment 192279 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> > > > > 
> > > > > > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:196
> > > > > > +void SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(const RenderObject* renderer, AffineTransform& absoluteTransform)
> > > > > 
> > > > > I don't think we are doing the right thing here.
> > > > > 
> > > > > What is returned from this function (now that you're going with smfr's suggestion) should be the exact same as context->getCTM(). In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.
> > > > 
> > > > The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.
> > > 
> > > On every paint you're going to crawl up the entire tree doing matrix multiplies at each step so the QT port doesn't break? Lets just fix QT's getCTM() if necessary :)
> > 
> > Nothing guarantees that CTM of 1,0,0,1,0,0 is in real screen coordinates. I don't think that helps.
> 
> My thinking is that we should be rasterizing in layer coordinates, not screen coordinates.
> 
> Maybe that is not best for users? Is this specified?

This is an implementation detail. It is not specified :)

[Build Bot]

Comment on attachment 192279 [details]
Patch

Attachment 192279 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17077614

[Build Bot]

Comment on attachment 192279 [details]
Patch

Attachment 192279 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17047580

[Build Bot]

Comment on attachment 192279 [details]
Patch

Attachment 192279 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17109407

[Florin Malita]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 192279 [details] [details] [details])
> > > In most of the callers of this function, the context is readily available, and in the others it's easily plumbed through.

This is not 100% correct: one of its users is SVGRenderingContext::calculateScreenFontSizeScalingFactor() which is also called on layout()/styleDidChange() - where a GC is simply not available. So it seems we would have to keep the old approach for this path alone.


> > The reason I'm hesitant about using getCTM() is the very existance of this method: why did we not rely on getCTM() from the beginning? From chatting with Dirk on IRC, it seems there might have been valid reasons (Qt platform differences) which forced us in this direction.
> 
> On every paint you're going to crawl up the entire tree doing matrix multiplies at each step so the QT port doesn't break? Lets just fix QT's getCTM() if necessary :)

We would be crawling the layer tree which is quite sparse - so the overhead is minimal. That said, using CTM directly does sound appealing (if correct) on the paint path. I can whip up an experimental patch just to see what breaks.

[Florin Malita]

Created attachment 192467 [details]
Switch paint() paths to getCTM.

Experimental patch to observe bots fallout - not for review.

[WebKit Review Bot]

Comment on attachment 192467 [details]
Switch paint() paths to getCTM.

Attachment 192467 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17113594

New failing tests:
svg/custom/absolute-sized-content-with-resources.xhtml
svg/custom/non-scaling-stroke.svg

[Florin Malita]

(In reply to comment #10)
> (From update of attachment 192279 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192279&action=review
> 
> > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:202
> > +    for (; renderer; renderer = renderer->parent()) {
> 
> For these kind of loops I like while more and it is used in a lot of other places as well. But I guess it is a matter of taste :)

Sounds good, I'll update :)


> > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:212
> > +    // Continue walking up the layer tree, accumulating CSS transforms.
> > +    for (RenderLayer* layer = renderer->enclosingLayer(); layer; layer = layer->parent()) {
> 
> What if the SVG root is in a foreignObject? Maybe the check above should be updated to look if there is a parent SVG renderer?

I've seen that type of test in other places, but I don't think it's buying us much: it only catches a narrow case where the SVG root is a direct descendant of fO - but if it's further down the tree (say there's a <div> between fO and the embedded SVG) then we're not noticing. It seems we would have to walk the whole render tree to detect this, which is probably not acceptable. Besides, transformed fO content is quite busted at the moment anyway - if we land the SVG/fO layer fixes then I believe this should just work as-is.


> > LayoutTests/ChangeLog:8
> > +        * platform/chromium-linux/svg/custom/resources-css-scaled-expected.png: Added.
> 
> You should be able to create a ref test instead of the image.

Yes, sounds doable.

[Florin Malita]

(In reply to comment #21)
> Created an attachment (id=192467) [details]
> Switch paint() paths to getCTM.
> 
> Experimental patch to observe bots fallout - not for review.

The results look encouraging: two CR failures which appear to be edge/rounding artifacts, with no visually noticeable effect.

I think we should move ahead with the current approach and pursue a getCTM switch in a separate bug.

[Florin Malita]

Created attachment 192525 [details]
Patch

Updated per review.

[Dirk Schulze]

Comment on attachment 192525 [details]
Patch

I am very much in favor for having a working version now, that may can be improved later. I expect a lot more work on getCTM in the short term. And, as fmailita says, would always require a context, which is maybe not the case in some situations. r+ from me.

[Build Bot]

Comment on attachment 192525 [details]
Patch

Attachment 192525 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17173107

New failing tests:
editing/selection/selection-modify-crash.html

[Florin Malita]

(In reply to comment #27)
> (From update of attachment 192525 [details])
> Attachment 192525 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17173107
> 
> New failing tests:
> editing/selection/selection-modify-crash.html

I don't think this is related, hopefully a flake. Let's see if the CQ bots agree.

[WebKit Review Bot]

Comment on attachment 192525 [details]
Patch

Clearing flags on attachment: 192525

Committed r145541: <http://trac.webkit.org/changeset/145541>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Did you intend to accumulate CSS transforms translation components too? This is breaking painting of a few test cases I have (I'll try to reduce them to a postable state).

[Florin Malita]

(In reply to comment #31)
> Did you intend to accumulate CSS transforms translation components too? This is breaking painting of a few test cases I have (I'll try to reduce them to a postable state).

Are CSS translations not reflected in the layer transform? If they're stored as layer offsets only, then yes, this patch misses that.

But I don't think the patch itself introduced this breakage - the previous tree crawl was stopping even earlier, at the SVG root boundary.

[Tim Horton]

(In reply to comment #32)
> (In reply to comment #31)
> > Did you intend to accumulate CSS transforms translation components too? This is breaking painting of a few test cases I have (I'll try to reduce them to a postable state).
> 
> Are CSS translations not reflected in the layer transform? If they're stored as layer offsets only, then yes, this patch misses that.
> 
> But I don't think the patch itself introduced this breakage - the previous tree crawl was stopping even earlier, at the SVG root boundary.

They *are* in the transform, and I don't think we want to include them! Stopping at the SVG root boundary would cause them to not be included. Including them means that the GraphicsContexts returned by SVG image buffer creation code will be translated by an additional offset that SVG doesn't know about (and doesn't include when painting!).

Definitely caused by this patch.

I'm going to remove the translation component when crawling the tree, that fixes my bug without reintroducing yours.

[Florin Malita]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > Did you intend to accumulate CSS transforms translation components too? This is breaking painting of a few test cases I have (I'll try to reduce them to a postable state).
> > 
> > Are CSS translations not reflected in the layer transform? If they're stored as layer offsets only, then yes, this patch misses that.
> > 
> > But I don't think the patch itself introduced this breakage - the previous tree crawl was stopping even earlier, at the SVG root boundary.
> 
> They *are* in the transform, and I don't think we want to include them! Stopping at the SVG root boundary would cause them to not be included. Including them means that the GraphicsContexts returned by SVG image buffer creation code will be translated by an additional offset that SVG doesn't know about (and doesn't include when painting!).

Ah, I see - I misread your comment as asking for translation to be included. I'm away from my workstation now, but at the time I remember convincing myself that all users only cared about the scale component anyway. Seems that I was wrong.

> Definitely caused by this patch.
> 
> I'm going to remove the translation component when crawling the tree, that fixes my bug without reintroducing yours.

Sure, that's the obvious middle ground but then I'd also suggest renaming the method to something more appropriate.

I'm kind of curious how cumulative SVG translations are not causing the same problem if present in the ImageBuffer GC. I assume we compensate for them somehow, but not for CSS. Will find out tomorrow...

[Tim Horton]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > Did you intend to accumulate CSS transforms translation components too? This is breaking painting of a few test cases I have (I'll try to reduce them to a postable state).
> > > 
> > > Are CSS translations not reflected in the layer transform? If they're stored as layer offsets only, then yes, this patch misses that.
> > > 
> > > But I don't think the patch itself introduced this breakage - the previous tree crawl was stopping even earlier, at the SVG root boundary.
> > 
> > They *are* in the transform, and I don't think we want to include them! Stopping at the SVG root boundary would cause them to not be included. Including them means that the GraphicsContexts returned by SVG image buffer creation code will be translated by an additional offset that SVG doesn't know about (and doesn't include when painting!).
> 
> Ah, I see - I misread your comment as asking for translation to be included. I'm away from my workstation now, but at the time I remember convincing myself that all users only cared about the scale component anyway. Seems that I was wrong.

I was wrong, slightly. It's the call from RenderSVGResourceMasker::applyResource to SVGRenderingContext::clipToImageBuffer that goes wrong. I'm going to file another bug so we can stop talking on this resolved one :)

> > Definitely caused by this patch.
> > 
> > I'm going to remove the translation component when crawling the tree, that fixes my bug without reintroducing yours.
> 
> Sure, that's the obvious middle ground but then I'd also suggest renaming the method to something more appropriate.
> 
> I'm kind of curious how cumulative SVG translations are not causing the same problem if present in the ImageBuffer GC. I assume we compensate for them somehow, but not for CSS. Will find out tomorrow...