Bug 67700 - REGRESSION(65665): Pattern size being clamped to SVG size can prevent transformed elements from being fully covered by userSpaceOnUse patterns
Summary: REGRESSION(65665): Pattern size being clamped to SVG size can prevent transfo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: http://people.igalia.com/amazari/temp...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-07 01:49 PDT by Alexandre Mazari
Modified: 2011-09-27 15:47 PDT (History)
6 users (show)

See Also:


Attachments
Failing svg accompanied with the external png used in bg. (148.21 KB, application/zip)
2011-09-07 04:07 PDT, Alexandre Mazari
no flags Details
screenshot showing differences of rendering this svg with inkscape, epiphany (webkit-gtk), chrome, firefox 6 (455.53 KB, image/png)
2011-09-07 04:10 PDT, Alexandre Mazari
no flags Details
Test: left rect should look like right rect (593 bytes, image/svg+xml)
2011-09-07 04:51 PDT, Dirk Schulze
no flags Details
preliminary patch (16.19 KB, patch)
2011-09-15 18:54 PDT, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
new patch, always do clamping in SVGImageBufferTools::clampedAbsoluteTargetRect (269.46 KB, patch)
2011-09-19 16:27 PDT, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fix warning (269.78 KB, patch)
2011-09-21 11:31 PDT, Tim Horton
zimmermann: review-
zimmermann: commit-queue-
Details | Formatted Diff | Diff
patch, now with changelogs and a test (295.15 KB, patch)
2011-09-26 14:36 PDT, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
new patch, correctly scaling oversized patterns to fit (down to fit in the tile, up to fit on the screen) (291.48 KB, patch)
2011-09-27 12:56 PDT, Tim Horton
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Mazari 2011-09-07 01:49:02 PDT
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
Comment 1 Dirk Schulze 2011-09-07 02:24:54 PDT
(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.
Comment 2 Alexandre Mazari 2011-09-07 04:07:18 PDT
Created attachment 106566 [details]
Failing svg accompanied with the external png used in bg.

Very simple test case exposing the issue related.
Comment 3 Alexandre Mazari 2011-09-07 04:10:59 PDT
Created attachment 106567 [details]
screenshot showing differences of rendering this svg with inkscape, epiphany (webkit-gtk), chrome, firefox 6
Comment 4 Dirk Schulze 2011-09-07 04:51:33 PDT
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.
Comment 5 Dirk Schulze 2011-09-07 04:53:51 PDT
> 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.
Comment 6 Alexandre Mazari 2011-09-07 04:57:28 PDT
(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 ?
Comment 7 Tim Horton 2011-09-14 12:10:07 PDT
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.
Comment 8 Radar WebKit Bug Importer 2011-09-14 12:16:14 PDT
<rdar://problem/10125102>
Comment 9 Tim Horton 2011-09-14 13:10:01 PDT
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?
Comment 10 Alexandre Mazari 2011-09-14 14:03:30 PDT
(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
Comment 11 Tim Horton 2011-09-14 14:27:26 PDT
(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.
Comment 12 Nikolas Zimmermann 2011-09-15 01:30:23 PDT
(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?
Comment 13 Nikolas Zimmermann 2011-09-15 01:30:59 PDT
(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/ :-)
Comment 14 Tim Horton 2011-09-15 18:54:40 PDT
Created attachment 107583 [details]
preliminary patch
Comment 15 WebKit Review Bot 2011-09-16 16:04:35 PDT
Comment on attachment 107583 [details]
preliminary patch

Attachment 107583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9725187
Comment 16 Nikolas Zimmermann 2011-09-17 06:20:54 PDT
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.
Comment 17 Tim Horton 2011-09-19 16:27:31 PDT
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.
Comment 18 WebKit Review Bot 2011-09-20 04:51:59 PDT
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
Comment 19 Tim Horton 2011-09-21 11:31:42 PDT
Created attachment 108193 [details]
fix warning
Comment 20 Dirk Schulze 2011-09-22 01:27:35 PDT
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
Comment 21 Nikolas Zimmermann 2011-09-23 01:37:45 PDT
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.
Comment 22 Tim Horton 2011-09-26 13:50:50 PDT
(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.
Comment 23 Tim Horton 2011-09-26 14:36:01 PDT
Created attachment 108723 [details]
patch, now with changelogs and a test
Comment 24 WebKit Review Bot 2011-09-26 17:32:52 PDT
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
Comment 25 Tim Horton 2011-09-26 18:11:50 PDT
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.
Comment 26 Dirk Schulze 2011-09-27 02:25:31 PDT
(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?
Comment 27 Tim Horton 2011-09-27 10:20:41 PDT
(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.
Comment 28 Tim Horton 2011-09-27 12:56:47 PDT
Created attachment 108886 [details]
new patch, correctly scaling oversized patterns to fit (down to fit in the tile, up to fit on the screen)
Comment 29 WebKit Review Bot 2011-09-27 13:30:08 PDT
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
Comment 30 Darin Adler 2011-09-27 13:43:18 PDT
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.
Comment 31 Tim Horton 2011-09-27 14:06:36 PDT
Landed (with Darin's nitpick) in r96155. There will be new-baselining to do shortly.
Comment 32 Tim Horton 2011-09-27 15:42:24 PDT
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!!)
Comment 33 Tim Horton 2011-09-27 15:47:37 PDT
(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