Bug 25484 – SVG patterns with some scale patternTransform are not displayed correctly for small elements

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

[Stephane Gigandet]

Patterns that have some scale patternTransform specified are not displayed correctly when applied on elements that are too small. They are displayed correctly on bigger element. (see attachment).

The same SVG file displays correctly for all element sizes in Firefox, Batik and Adobe IE SVG plugin.

[Stephane Gigandet]

Created attachment 29911 [details]
svg file with a pattern with a scale patternTransform - not correctly displayed in webkit  r42962

[Stephane Gigandet]

Created attachment 29912 [details]
screenshot with webkit r42962

[Dirk Schulze]

SVGResourcePattern builds the pattern at runtime and calls SVGPatternElement::buildPattern(FloatRect), where FloatRect is the objectBoundingBox of the object, that is filled with the pattern.
buildPattern() clips the patternimage to object size, if the object is smaler than the pattern. I think this clipping is wrong in this place.
I wonder why we don't have a test for this.

[Dirk Schulze]

Created attachment 29993 [details]
SVG pattern size bigger than target size and scaled by patternTransformation

I can't create a  result of the test case. I don't have access to a Mac at the moment.

[Eric Seidel (no email)]

Comment on attachment 29993 [details]
SVG pattern size bigger than target size and scaled by patternTransformation

I don't remember what the clipping is for?  Do you know?

[Eric Seidel (no email)]

I wonder if the clipping is trying to allow us to render really really large patterns in a small object w/o having to allocate a huge pattern buffer?  As in, if the pattern tile is displayed less than once..

[Dirk Schulze]

Created attachment 30317 [details]
SVG pattern size bigger than target size + patternTransformation

I still believe that deleting this code is better. patterTransform contains skewing and rotating. It makes it realy difficult up to impossible to be sure that our optimisation can handle all combinations of patterTransform. It would need a lot of logic.
I added two test cases. One for the problem itself and one for a pattern with an excessive malloc.

[Stephane Gigandet]

Maybe the clipping optimization could be kept if there is no patternTransformation at all?

[Eric Seidel (no email)]

Comment on attachment 30317 [details]
SVG pattern size bigger than target size + patternTransformation

What I'm interested in is a test which "passes" before you remove the clips, but "fails" now that they are removed.  I want to understand the behavior change via a test.

Also your excessive-malloc values are large enough that they're wrapping negative.  console.log is showing parseMappedAttribute complaing about negative values, which is not the code path you're trying to test, I think. :)

[Eric Seidel (no email)]

The console errors I see when loading your excessive-malloc test case are:
Error: A negative value for pattern attribute <width> is not allowed
Error: A negative value for pattern attribute <height> is not allowed
Error: A negative value for rect <width> is not allowed
Error: A negative value for rect <height> is not allowed

[Dirk Schulze]

Created attachment 30318 [details]
SVG pattern size bigger than target size + patternTransformation

Added a third test to with patternTransform="skewX(45)" that also shows the problem and fixed the malloc test.

[Dirk Schulze]

(In reply to comment #8)
> Maybe the clipping optimization could be kept if there is no
> patternTransformation at all?

Sorry, haven't seen your comment. Yes and no. It should work but it doesn't like expected. The code doesnt' respect patternUnits. If you move your object, it can happen that the pattern is cut on the wrong place. Short example: 

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg">
<defs>
<pattern id="pattern" height="200" width="200" patternUnits="userSpaceOnUse">
<rect x="0" y="0" width="100" height="200" fill="blue" />
<rect x="60" y="0" width="100" height="200" fill="red" />
</pattern>
</defs>
<rect x="10" y="140" width="100" height="100" fill="url(#pattern)" />
</svg>

I don't use patternTransform, but the calculation of the cutting is still wrong.

[Eric Seidel (no email)]

Comment on attachment 30318 [details]
SVG pattern size bigger than target size + patternTransformation

This is an improvement over our previous behavior, even if it "breaks" extra-large patterns which "worked" before.

[Eric Seidel (no email)]

Comment on attachment 30318 [details]
SVG pattern size bigger than target size + patternTransformation

These tests change with this patch:

svg/W3C-SVG-1.1/coords-units-01-b.svg	expected	actual	diff	pretty diff
svg/W3C-SVG-1.1/pservers-grad-03-b.svg	expected	actual	diff	pretty diff
svg/W3C-SVG-1.1/pservers-grad-06-b.svg	expected	actual	diff	pretty diff
svg/W3C-SVG-1.1/pservers-pattern-01-b.svg	expected	actual	diff	pretty diff
svg/batik/paints/patternPreserveAspectRatioA.svg	expected	actual	diff	pretty diff
svg/batik/paints/patternRegions.svg	expected	actual	diff	pretty diff
svg/custom/deep-dynamic-updates.svg	expected	actual	diff	pretty diff
svg/custom/js-late-pattern-and-object-creation.svg	expected	actual	diff	pretty diff
svg/custom/js-late-pattern-creation.svg	expected	actual	diff	pretty diff
svg/custom/js-update-pattern-child.svg	expected	actual	diff	pretty diff
svg/custom/js-update-pattern.svg	expected	actual	diff	pretty diff
svg/custom/pattern-cycle-detection.svg	expected	actual	diff	pretty diff
svg/custom/pattern-deep-referencing.svg	expected	actual	diff	pretty diff
svg/custom/pattern-in-defs.svg	expected	actual	diff	pretty diff
svg/custom/pattern-with-transformation.svg	expected	actual	diff	pretty diff
svg/custom/pattern-y-offset.svg	expected	actual	diff	pretty diff
svg/custom/pointer-events-path.svg	expected image	image diffs	0.60%
svg/custom/stroked-pattern.svg	expected	actual	diff	pretty diff

I was not comfortable making the changes on my own.

[Dirk Schulze]

The failing pixel test has nothing to do with our pattern patch. The other tests fail because LayoutTests display the values of pattternBoundaries and we don't modify the patternBoundaries now (like the clipping code did).
I think it's the best to update the tests.  The clipping is not only wrong for the use with patternTransform, userSpaceOnUse, but for objectBoundingBox too. Short example:
<pattern id="pattern" height="100%" width="110%">
The Clipping would cut of the patter size at 100% width.

I still believe it's the best to delete the code and not to add a new but also wrong code and just update the LayoutTests.

[Stephane Gigandet]

Can the patch be checked-in or does it need more review?

You can see this bug in a real application here: http://scrapcoloring.com

I would love to be able to recommend Webkit as much as Firefox. :-)

Thanks a lot for your work,

Stephane

[Stephane Gigandet]

Any resolution on this? Can Dirk's patch be checked-in?
https://bugs.webkit.org/attachment.cgi?id=30318

[Dirk Schulze]

(In reply to comment #17)
> Any resolution on this? Can Dirk's patch be checked-in?
> https://bugs.webkit.org/attachment.cgi?id=30318

I checked the patch with trunk. I can confirm the failing tests eric mentioned. The pixel tests are still passing. I check if they just need an update.

[Dirk Schulze]

Created attachment 47452 [details]
pattern transform fix

The current pattern sizes in some DRT test do not relate to the real pattern sizes. This is fixed aft the patch and the results got an update.

[Dirk Schulze]

Comment on attachment 47452 [details]
pattern transform fix

reseting the review flag. We might have a bug in DRT and schould fix this first.

[Dirk Schulze]

Created attachment 48366 [details]
pattern transform fix

updated to trunk

[Dirk Schulze]

Talked with Niko on IRC and he reviewed it. The Changes on DRT are covered by another bug: DRT ans Resources https://bugs.webkit.org/show_bug.cgi?id=33105
landed in 54560