Bug 25484 - SVG patterns with some scale patternTransform are not displayed correctly for small elements
Summary: SVG patterns with some scale patternTransform are not displayed correctly for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-30 08:50 PDT by Stephane Gigandet
Modified: 2010-02-09 10:42 PST (History)
3 users (show)

See Also:


Attachments
svg file with a pattern with a scale patternTransform - not correctly displayed in webkit r42962 (1.54 KB, image/svg+xml)
2009-04-30 08:52 PDT, Stephane Gigandet
no flags Details
screenshot with webkit r42962 (13.06 KB, image/png)
2009-04-30 08:55 PDT, Stephane Gigandet
no flags Details
SVG pattern size bigger than target size and scaled by patternTransformation (2.82 KB, patch)
2009-05-04 10:49 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVG pattern size bigger than target size + patternTransformation (3.54 KB, patch)
2009-05-14 00:22 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
SVG pattern size bigger than target size + patternTransformation (4.58 KB, patch)
2009-05-14 01:32 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
pattern transform fix (128.00 KB, patch)
2010-01-26 15:08 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
pattern transform fix (310.22 KB, patch)
2010-02-08 14:49 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephane Gigandet 2009-04-30 08:50:09 PDT
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.
Comment 1 Stephane Gigandet 2009-04-30 08:52:04 PDT
Created attachment 29911 [details]
svg file with a pattern with a scale patternTransform - not correctly displayed in webkit  r42962
Comment 2 Stephane Gigandet 2009-04-30 08:55:31 PDT
Created attachment 29912 [details]
screenshot with webkit r42962
Comment 3 Dirk Schulze 2009-05-04 10:27:48 PDT
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.
Comment 4 Dirk Schulze 2009-05-04 10:49:22 PDT
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.
Comment 5 Eric Seidel (no email) 2009-05-04 17:18:38 PDT
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?
Comment 6 Eric Seidel (no email) 2009-05-04 17:19:38 PDT
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..
Comment 7 Dirk Schulze 2009-05-14 00:22:34 PDT
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.
Comment 8 Stephane Gigandet 2009-05-14 00:29:58 PDT
Maybe the clipping optimization could be kept if there is no patternTransformation at all?
Comment 9 Eric Seidel (no email) 2009-05-14 00:37:41 PDT
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. :)
Comment 10 Eric Seidel (no email) 2009-05-14 00:40:21 PDT
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
Comment 11 Dirk Schulze 2009-05-14 01:32:04 PDT
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.
Comment 12 Dirk Schulze 2009-05-14 03:05:02 PDT
(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.
Comment 13 Eric Seidel (no email) 2009-05-14 07:41:20 PDT
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.
Comment 14 Eric Seidel (no email) 2009-05-15 00:39:55 PDT
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.
Comment 15 Dirk Schulze 2009-05-15 03:32:23 PDT
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.
Comment 16 Stephane Gigandet 2009-06-03 03:01:37 PDT
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
Comment 17 Stephane Gigandet 2010-01-26 03:12:52 PST
Any resolution on this? Can Dirk's patch be checked-in?
https://bugs.webkit.org/attachment.cgi?id=30318
Comment 18 Dirk Schulze 2010-01-26 12:49:11 PST
(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.
Comment 19 Dirk Schulze 2010-01-26 15:08:49 PST
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.
Comment 20 Dirk Schulze 2010-01-26 15:36:13 PST
Comment on attachment 47452 [details]
pattern transform fix

reseting the review flag. We might have a bug in DRT and schould fix this first.
Comment 21 Dirk Schulze 2010-02-08 14:49:23 PST
Created attachment 48366 [details]
pattern transform fix

updated to trunk
Comment 22 Dirk Schulze 2010-02-09 10:40:28 PST
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