Bug 67700 – REGRESSION(65665): Pattern size being clamped to SVG size can prevent transformed elements from being fully covered by userSpaceOnUse patterns

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

[Alexandre Mazari]

Webkit exposes some artifacts when rendering the linked svg file: the bottom part of the inner screenshot acutally display some content from the hidden top content.

Both chromium and webkit-gtk display this behaviour, so the bug might lie in the common code.
Firefox 6 do not exposes the same artifacts (but is dog slow at scrolling).

Steps to reproduce:
- visit http://people.igalia.com/amazari/template256.svg with a webkit-based browser
- pan the screenshot (drag/drop)
- observe the lower part

[Dirk Schulze]

(In reply to comment #0)
> Webkit exposes some artifacts when rendering the linked svg file: the bottom part of the inner screenshot acutally display some content from the hidden top content.
> 
> Both chromium and webkit-gtk display this behaviour, so the bug might lie in the common code.
> Firefox 6 do not exposes the same artifacts (but is dog slow at scrolling).
> 
> Steps to reproduce:
> - visit http://people.igalia.com/amazari/template256.svg with a webkit-based browser
> - pan the screenshot (drag/drop)
> - observe the lower part

It would be great if you can reduce the test case a lot and upload a none dynamic reproduction.

[Alexandre Mazari]

Created attachment 106566 [details]
Failing svg accompanied with the external png used in bg.

Very simple test case exposing the issue related.

[Alexandre Mazari]

Created attachment 106567 [details]
screenshot showing differences of rendering this svg with inkscape, epiphany (webkit-gtk), chrome, firefox 6

[Dirk Schulze]

Created attachment 106570 [details]
Test: left rect should look like right rect

Thank you very much. I reduced the test case even more.

Their seems to be a problem with the transform of the object, it's current position and the patterns bounding box. Looks like a problem in RenderSVGResourcePattern.

Both rects in the example should look identical.

[Dirk Schulze]

> Both rects in the example should look identical.

Sorry, thats wrong, the fill of the first rect should be stretched to the full rect size.

[Alexandre Mazari]

(In reply to comment #4)
> Created an attachment (id=106570) [details]
> Test: left rect should look like right rect
> 
> Thank you very much. I reduced the test case even more.
> 
> Their seems to be a problem with the transform of the object, it's current position and the patterns bounding box. Looks like a problem in RenderSVGResourcePattern.
> 
> Both rects in the example should look identical.

Wow, thanks for looking into this so fast!
I am not versed at all in the rendering codepath, but is there anything I could do to help out ?

[Tim Horton]

It seems like the pattern being drawn is getting clipped by something for some reason; if you increase the size of the <svg> so that it would fit the pre-transformed rect (i.e. 300px height <svg>), everything works.

Oh, actually, the tile size is clamped against the <svg> size in RenderSVGResourcePattern::createTileImage. Removing that code fixes this; seems like we'll have to rethink our "last-ditch" clamping approach.

[Radar WebKit Bug Importer]

<rdar://problem/10125102>

[Tim Horton]

Really, is this restriction actually *useful*? I can still create a pattern tile that consumes inordinate amounts of memory by just making the <svg> really big (and by "can", I mean my computer is somewhat difficult to use right now because of WebKit thrashing it in the background...)

In any case, this will only be a policy change. Is there something better we can clamp against? Does this restriction even make sense?

[Alexandre Mazari]

(In reply to comment #9)
> Really, is this restriction actually *useful*? I can still create a pattern tile that consumes inordinate amounts of memory by just making the <svg> really big (and by "can", I mean my computer is somewhat difficult to use right now because of WebKit thrashing it in the background...)
> 
> In any case, this will only be a policy change. Is there something better we can clamp against? Does this restriction even make sense?

The specs do not define any restriction, at least:
http://www.w3.org/TR/SVG/pservers.html#Patterns

[Tim Horton]

(In reply to comment #10)
> (In reply to comment #9)
> > Really, is this restriction actually *useful*? I can still create a pattern tile that consumes inordinate amounts of memory by just making the <svg> really big (and by "can", I mean my computer is somewhat difficult to use right now because of WebKit thrashing it in the background...)
> > 
> > In any case, this will only be a policy change. Is there something better we can clamp against? Does this restriction even make sense?
> 
> The specs do not define any restriction, at least:
> http://www.w3.org/TR/SVG/pservers.html#Patterns

No, I wouldn't expect the spec to, I think this is an implementation detail (trying to prevent people from allocating massive tiles) which was never meant to have a visual impact.

[Nikolas Zimmermann]

(In reply to comment #9)
> Really, is this restriction actually *useful*? I can still create a pattern tile that consumes inordinate amounts of memory by just making the <svg> really big (and by "can", I mean my computer is somewhat difficult to use right now because of WebKit thrashing it in the background...)
That's true, I indeed didn't consider that!

>  In any case, this will only be a policy change. Is there something better we can clamp against? Does this restriction even make sense?
We need to clamp against some value, otherwise it's going to trash us - we have testcases that would hang/crash DRT in svg/custom IIRC.

(In reply to comment #11)
> No, I wouldn't expect the spec to, I think this is an implementation detail (trying to prevent people from allocating massive tiles) which was never meant to have a visual impact.
Exactly, it was meant as protection as I disliked hardcoding arbitary values like kMaxSize 1000x1000.
I think the only proper way to handle this is:
- force an upper limit of image buffer sizes
- if requested image size is smaller, compute scale factors that have to be applied before painting to the image context, to make it fit into that size (similar code exists for <filter>)

What do you think?

[Nikolas Zimmermann]

(In reply to comment #12)
> - if requested image size is smaller, compute scale factors that have to be applied before painting to the image context, to make it fit into that size (similar code exists for <filter>)
s/smaller/larger/ :-)

[Tim Horton]

Created attachment 107583 [details]
preliminary patch

[WebKit Review Bot]

Comment on attachment 107583 [details]
preliminary patch

Attachment 107583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9725187

[Nikolas Zimmermann]

Comment on attachment 107583 [details]
preliminary patch

I like the patch, though mac doesn't build. What about masker / filter btw? Shouldn't we impose the same upper limit on all SVG users of SVGImageBufferTools/ImageBuffer.

I'd rather like to have a kMaxImageBufferSize that's used for gradients (text fallback for CG platforms) / masks / filter / clipper.

[Tim Horton]

Created attachment 107942 [details]
new patch, always do clamping in SVGImageBufferTools::clampedAbsoluteTargetRect

Not sure why mac build was failing, looking at the logs, it looks like things were just really broken when I submitted that patch.

[WebKit Review Bot]

Comment on attachment 107942 [details]
new patch, always do clamping in SVGImageBufferTools::clampedAbsoluteTargetRect

Attachment 107942 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9763214

[Tim Horton]

Created attachment 108193 [details]
fix warning

[Dirk Schulze]

Comment on attachment 108193 [details]
fix warning

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

Any need to change the tests to use just userSpaceOnUse? Don't we want to test both, userSpaceOnUse and objectBoundingBox?

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:129
> +FloatRect SVGImageBufferTools::clampedAbsoluteTargetRect(const FloatRect& absoluteTargetRect)
>  {
> -    ASSERT(renderer);
> -
> -    const RenderSVGRoot* svgRoot = SVGRenderSupport::findTreeRootObject(renderer);
>      FloatRect clampedAbsoluteTargetRect = absoluteTargetRect;
> -    clampedAbsoluteTargetRect.intersect(svgRoot->frameRect());
> +    
> +    if (clampedAbsoluteTargetRect.width() > kMaxImageBufferSize)
> +        clampedAbsoluteTargetRect.setWidth(kMaxImageBufferSize);
> +    
> +    if (clampedAbsoluteTargetRect.height() > kMaxImageBufferSize)
> +        clampedAbsoluteTargetRect.setHeight(kMaxImageBufferSize);
> +
>      return clampedAbsoluteTargetRect;
>  }

Now that this function does not need a renderer: Can you consider to make most parts of SVGImageBufferTools independent of SVG? This way we could remove the if ENABLE(SVG) switch on platform/graphics/FETile.cpp

[Nikolas Zimmermann]

Comment on attachment 108193 [details]
fix warning

I'm tempted to r+, though ChangeLogs are missing! Thus it's unclear why you changed lots of cases from objectBoundingBox to userSpaceOnUse.

[Tim Horton]

(In reply to comment #21)
> (From update of attachment 108193 [details])
> I'm tempted to r+, though ChangeLogs are missing! Thus it's unclear why you changed lots of cases from objectBoundingBox to userSpaceOnUse.

Err, not sure what happened to my ChangeLogs.

Anyway, their pixel results were (probably unknowingly, certainly unrelated to the tests themselves) depending on being clamped to the size of the SVG. Since we allow them to be bigger now, that doesn't make any sense.

I'll cook up some ChangeLog entries since I can't find my old ones.

[Tim Horton]

Created attachment 108723 [details]
patch, now with changelogs and a test

[WebKit Review Bot]

Comment on attachment 108723 [details]
patch, now with changelogs and a test

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

New failing tests:
svg/custom/transformed-pattern-clamp-svg-root.svg

[Tim Horton]

Comment on attachment 108723 [details]
patch, now with changelogs and a test

Looking again I'm not seeing the test failures I was then, I'll take a look tomorrow, but some of this might be unnecessary.

[Dirk Schulze]

(In reply to comment #25)
> (From update of attachment 108723 [details])
> Looking again I'm not seeing the test failures I was then, I'll take a look tomorrow, but some of this might be unnecessary.

Any opinions about comment #20 Tim?

[Tim Horton]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 108723 [details] [details])
> > Looking again I'm not seeing the test failures I was then, I'll take a look tomorrow, but some of this might be unnecessary.
> 
> Any opinions about comment #20 Tim?

There are two parts to comment #20:

> Any need to change the tests to use just userSpaceOnUse? Don't we want to test both, userSpaceOnUse and objectBoundingBox?

Right, I'm going to find a different fix for the few that need fixing, this is comment #25.

> Now that this function does not need a renderer: Can you consider to make most parts of SVGImageBufferTools independent of SVG? This way we could remove the if ENABLE(SVG) switch on platform/graphics/FETile.cpp

I think this is a likely future direction to go as we try to make filters more generic. Separate from this patch, though.

[Tim Horton]

Created attachment 108886 [details]
new patch, correctly scaling oversized patterns to fit (down to fit in the tile, up to fit on the screen)

[WebKit Review Bot]

Comment on attachment 108886 [details]
new patch, correctly scaling oversized patterns to fit (down to fit in the tile, up to fit on the screen)

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

New failing tests:
svg/custom/oversized-pattern-scale.svg
svg/custom/transformed-pattern-clamp-svg-root.svg

[Darin Adler]

Comment on attachment 108886 [details]
new patch, correctly scaling oversized patterns to fit (down to fit in the tile, up to fit on the screen)

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

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:31
> +static float kMaxImageBufferSize = 4096.0;

Nitpick: This should just be 4096 (an int) rather than 4096.0 (a double). Neither is a float, but that's OK.

[Tim Horton]

Landed (with Darin's nitpick) in r96155. There will be new-baselining to do shortly.

[Tim Horton]

Err, when going to add new baselines, I noticed that DRT isn't rendering the whole of the second test (svg/custom/oversized-pattern-scale.svg) because DRT is a fixed size. I hadn't noticed because of the lack of scrollbars... I'll have to rethink that test (I'm not sure there's a good way to do it, though!!)

[Tim Horton]

(In reply to comment #32)
> Err, when going to add new baselines, I noticed that DRT isn't rendering the whole of the second test (svg/custom/oversized-pattern-scale.svg) because DRT is a fixed size. I hadn't noticed because of the lack of scrollbars... I'll have to rethink that test (I'm not sure there's a good way to do it, though!!)

Oh, actually it's simple: https://bugs.webkit.org/show_bug.cgi?id=68945