Bug 53055 - REGRESSION: Rendering artifacts on a rotated, pattern filled shape
: REGRESSION: Rendering artifacts on a rotated, pattern filled shape
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To: Nobody
: InRadar
Depends on:
Blocks: 67242
  Show dependency treegraph
 
Reported: 2011-01-24 17:53 PST by Tony Sung
Modified: 2011-09-01 13:27 PDT (History)
8 users (show)

See Also:


Attachments
Primary test case (1.20 KB, image/svg+xml)
2011-01-24 17:54 PST, Tony Sung
no flags Details
Updated Primary Test Case (1.94 KB, image/svg+xml)
2011-01-26 17:38 PST, Tony Sung
no flags Details
Updated Primary Test Case 2 (2.91 KB, image/svg+xml)
2011-01-27 21:21 PST, Tony Sung
no flags Details
Updated primary test case (2.92 KB, image/svg+xml)
2011-08-16 16:35 PDT, Tim Horton
no flags Details
Updated similar HTML test case (592 bytes, text/html)
2011-08-16 17:45 PDT, Tim Horton
no flags Details
Screen shot after changing rounding mode for image size (263.51 KB, image/png)
2011-08-16 18:00 PDT, Tim Horton
no flags Details
preliminary partial patch (2.41 KB, patch)
2011-08-17 16:17 PDT, Tim Horton
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Screenshot with patch (104267) (37.36 KB, image/png)
2011-08-17 16:20 PDT, Tim Horton
no flags Details
[WORKAROUND] overdraw rects in pattern (2.92 KB, image/svg+xml)
2011-08-17 16:24 PDT, Tim Horton
no flags Details
Screenshot with both patch (104267) and overdraw (104272) (36.47 KB, image/png)
2011-08-17 16:24 PDT, Tim Horton
no flags Details
patch, fixes between-pattern-tiling cracks (790.62 KB, patch)
2011-08-19 19:32 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
style fix (789.79 KB, patch)
2011-08-19 19:38 PDT, Tim Horton
zimmermann: review-
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
example of overdraw (why rounding is bad) (28.18 KB, image/png)
2011-08-22 17:36 PDT, Tim Horton
no flags Details
example of overdraw (why rounding is bad) -- SVG source (1.02 KB, image/svg+xml)
2011-08-22 17:37 PDT, Tim Horton
no flags Details
Clear Z-rotation before determining the destination pattern tile size (792.01 KB, patch)
2011-08-24 17:30 PDT, Tim Horton
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Same as last, plus a test (812.16 KB, patch)
2011-08-24 18:09 PDT, Tim Horton
krit: review-
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
patch, hopefully builds on mac this time (815.94 KB, patch)
2011-08-30 16:18 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch, fixing Darin's comments (815.10 KB, patch)
2011-08-30 16:43 PDT, Tim Horton
krit: review-
thorton: commit‑queue-
Details | Formatted Diff | Diff
Patch 1 of 2: just the WKSI portion (should work with Windows now too) (1.01 MB, patch)
2011-08-31 18:51 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern (849.51 KB, patch)
2011-09-01 12:16 PDT, Tim Horton
simon.fraser: 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 Tony Sung 2011-01-24 17:53:36 PST
Steps to reproduce:

1. Create an svg:pattern with adjoining squares of the same color, i.e. the pattern is fully filled with the color without any gap
2. Fill a rectangle with the pattern
3. Apply a slight rotation to the rectangle

Expected result:

The rectangle is rotated and is filled perfectly with the color.

Actual result:

There's gaps inside the rectangle. See attached test case.
Comment 1 Tony Sung 2011-01-24 17:54:14 PST
Created attachment 80000 [details]
Primary test case
Comment 2 Deirdre Saoirse Moen 2011-01-24 23:54:21 PST
rdar://problem/8910917
Comment 3 Tony Sung 2011-01-26 17:38:56 PST
Created attachment 80280 [details]
Updated Primary Test Case
Comment 4 Tony Sung 2011-01-26 17:46:41 PST
The updated primary test case now shows the different behavior for patterns using 'userSpaceOnUse' and 'objectBoundingBox'.

The interesting thing is that there's an additional regression for patterns defined with 'objectBoundingBox' in between r65398 and r65707.
Comment 5 Tony Sung 2011-01-26 18:10:27 PST
Probably a regression of fixing:
- https://bugs.webkit.org/show_bug.cgi?id=38704
- https://bugs.webkit.org/show_bug.cgi?id=41603
Comment 6 Tony Sung 2011-01-27 21:21:53 PST
Created attachment 80417 [details]
Updated Primary Test Case 2

Included effect of scale() in the test case. Artifacts also appears if scale() is applied to the rect if the pattern is defined in "userSpaceOnUse".
Comment 7 Tim Horton 2011-08-16 16:35:20 PDT
Created attachment 104117 [details]
Updated primary test case

This test was initially confusing because the text says userSpaceOnUse for both columns. Uploading a fixed version.
Comment 8 Tim Horton 2011-08-16 17:45:12 PDT
Created attachment 104133 [details]
Updated similar HTML test case

Attaching a HTML+(-webkit-transform) example which exhibits a similar problem.

*Part* of the problem introduced by r65665 can be fixed by using ceil instead of round in roundedImageBufferSize (and I'm doubly convinced that's the right thing to do because of something I'll show momentarily).
Comment 9 Tim Horton 2011-08-16 18:00:17 PDT
Created attachment 104135 [details]
Screen shot after changing rounding mode for image size

This screenshot regards what I mentioned in the previous comment.

Before r65665, we used enclosingIntRect to compute the size of the ImageBuffer; afterwards, we use roundedImageBufferSize, which uses lroundf.

Just to test, I've switched to using ceil, like enclosingIntRect, and this fixes both pattern overdraw (which you can see in the picture that I'm attaching, but couldn't see in the older tests because of a lack of content), and *some* of the jagged edges.

I'll keep looking for other broken rounding cases tomorrow and see if I can't get us back to at least the level of broken pre-65665, though the fact that it's slightly broken with HTML+(-webkit-transform) might make a total solution more difficult.
Comment 10 Tim Horton 2011-08-17 16:17:00 PDT
Created attachment 104267 [details]
preliminary partial patch
Comment 11 Tim Horton 2011-08-17 16:20:20 PDT
Created attachment 104268 [details]
Screenshot with patch (104267)
Comment 12 Tim Horton 2011-08-17 16:22:51 PDT
As a workaround to the larger problem, if you overdraw adjacent rects in the pattern, most of the rest of the gaps go away, leaving us only with gaps equivalent to what you see in the HTML test case, and exactly the same as what you see in Firefox. Attaching a screenshot and adjusted test case.
Comment 13 Tim Horton 2011-08-17 16:24:02 PDT
Created attachment 104272 [details]
[WORKAROUND] overdraw rects in pattern
Comment 14 Tim Horton 2011-08-17 16:24:43 PDT
Created attachment 104274 [details]
Screenshot with both patch (104267) and overdraw (104272)
Comment 15 WebKit Review Bot 2011-08-18 22:34:51 PDT
Comment on attachment 104267 [details]
preliminary partial patch

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

New failing tests:
svg/custom/feComponentTransfer-Gamma.svg
svg/W3C-SVG-1.1/filters-comptran-01-b.svg
svg/custom/feComponentTransfer-Discrete.svg
svg/batik/text/textFeatures.svg
svg/W3C-SVG-1.1/filters-color-01-b.svg
svg/W3C-SVG-1.1/filters-morph-01-f.svg
svg/custom/feComponentTransfer-Table.svg
svg/filters/filterRes.svg
svg/custom/feComponentTransfer-Linear.svg
svg/custom/js-late-pattern-and-object-creation.svg
svg/custom/js-late-pattern-creation.svg
Comment 16 Tim Horton 2011-08-19 19:32:11 PDT
Created attachment 104610 [details]
patch, fixes between-pattern-tiling cracks
Comment 17 WebKit Review Bot 2011-08-19 19:35:18 PDT
Attachment 104610 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/cg/PatternCG.cpp:71:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/cg/PatternCG.cpp:69:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Tim Horton 2011-08-19 19:38:33 PDT
Created attachment 104612 [details]
style fix
Comment 19 WebKit Review Bot 2011-08-19 20:46:01 PDT
Comment on attachment 104612 [details]
style fix

Attachment 104612 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9436568
Comment 20 Nikolas Zimmermann 2011-08-20 00:13:54 PDT
Comment on attachment 104612 [details]
style fix

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

I'm not sure about the SystemInterface stuff, as I have no idea how it works, but I trust you on that. Still some questions:

> Source/WebCore/ChangeLog:11
> +        Introduce wkCGPatternCreateWithImageAndTransform, and make use of it
> +        when tiling patterns in both directions. This helps to prevent
> +        pixel-cracking along pattern tiling boundaries.

It's so wonderful to have someone from Apple working on SVG, with the abilities to do such things as extend the WebKitSystemInterface :-)

> Source/WebCore/ChangeLog:15
> +        Fix a rounding error in the size of the temporary image created to
> +        render SVG pattern tiles into, which was introduced in r65665.
> +        We now round SVG tile image size up in all cases.

I think I introduced the ceilf logic on purpose. The ceilf alone makes no sense, but it's used in conjunction with following code: in PassOwnPtr<ImageBuffer> RenderSVGResourcePattern::createTileImage:

...
    if (!SVGImageBufferTools::createImageBuffer(absoluteTileBoundaries, clampedAbsoluteTileBoundaries, tileImage, ColorSpaceDeviceRGB))
        return nullptr;

    GraphicsContext* tileImageContext = tileImage->context();
    ASSERT(tileImageContext);

    // The image buffer represents the final rendered size, so the content has to be scaled (to avoid pixelation).
    tileImageContext->scale(FloatSize(absoluteTileBoundaries.width() / tileBoundaries.width(),
                                      absoluteTileBoundaries.height() / tileBoundaries.height()));

Is this obsolete now? How are other platforms affected by your change to avoid the ceil?
Maskers contain similar code to avoid fix-up rounding differences by the ceil.

> Source/WebCore/ChangeLog:17
> +        No new tests. (OOPS!)

Can you add a test?
Comment 21 Tim Horton 2011-08-20 13:27:20 PDT
Comment on attachment 104612 [details]
style fix

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

>> Source/WebCore/ChangeLog:11
>> +        pixel-cracking along pattern tiling boundaries.
> 
> It's so wonderful to have someone from Apple working on SVG, with the abilities to do such things as extend the WebKitSystemInterface :-)

:-)

>> Source/WebCore/ChangeLog:15
>> +        We now round SVG tile image size up in all cases.
> 
> I think I introduced the ceilf logic on purpose. The ceilf alone makes no sense, but it's used in conjunction with following code: in PassOwnPtr<ImageBuffer> RenderSVGResourcePattern::createTileImage:
> 
> ...
>     if (!SVGImageBufferTools::createImageBuffer(absoluteTileBoundaries, clampedAbsoluteTileBoundaries, tileImage, ColorSpaceDeviceRGB))
>         return nullptr;
> 
>     GraphicsContext* tileImageContext = tileImage->context();
>     ASSERT(tileImageContext);
> 
>     // The image buffer represents the final rendered size, so the content has to be scaled (to avoid pixelation).
>     tileImageContext->scale(FloatSize(absoluteTileBoundaries.width() / tileBoundaries.width(),
>                                       absoluteTileBoundaries.height() / tileBoundaries.height()));
> 
> Is this obsolete now? How are other platforms affected by your change to avoid the ceil?
> Maskers contain similar code to avoid fix-up rounding differences by the ceil.

You mean the lroundf? I made it ceilf again in this patch (like you used to have it). I'll show you a variety of examples where lroundf breaks things at some point later this weekend.

I'm not avoiding a ceil, I'm adding one.

>> Source/WebCore/ChangeLog:17
>> +        No new tests. (OOPS!)
> 
> Can you add a test?

Planning on it!
Comment 22 Tim Horton 2011-08-22 17:36:22 PDT
Niko: here's one of the cases that I was talking about; rounding down here causes us to begin to draw the pattern again (because it's not big enough) -- note the bright orange line on the right side of the image, even though the rectangles are exactly the same size.

I also question why we're drawing a significantly larger copy of the image for the rotation case; there's no scaling going on here, so it shouldn't have anything to do with pixelation when drawn on the destination. Is the target size being miscomputed?
Comment 23 Tim Horton 2011-08-22 17:36:50 PDT
Created attachment 104773 [details]
example of overdraw (why rounding is bad)
Comment 24 Tim Horton 2011-08-22 17:37:24 PDT
Created attachment 104774 [details]
example of overdraw (why rounding is bad) -- SVG source
Comment 25 Dirk Schulze 2011-08-23 01:53:22 PDT
(In reply to comment #22)
> Niko: here's one of the cases that I was talking about; rounding down here causes us to begin to draw the pattern again (because it's not big enough) -- note the bright orange line on the right side of the image, even though the rectangles are exactly the same size.
> 
> I also question why we're drawing a significantly larger copy of the image for the rotation case; there's no scaling going on here, so it shouldn't have anything to do with pixelation when drawn on the destination. Is the target size being miscomputed?

We try to get the affected area not the screen in screen coordinates. While this works great for scaling, it looks a bit scaring on rotations and skews (like you noted above). I'm not sure if I remember correctly, but I think their were some problems to divide a transformation matrix in the two parts translation, scaling, skewing parts and the rotation. If we can divide the rotation from the other parts, we will get better results for rotated elements.

I think I can remember to have a negative example:

<pattern width="20" height="200" patternTransform="rotate(90)">
<image width="20" height="200"/>
</pattern>

IIRC we will create an image buffer with the size 200x20 and scale the image to match in this rect. The image will of course loose in quality.
Comment 26 Tim Horton 2011-08-23 10:56:40 PDT
(In reply to comment #25)
> (In reply to comment #22)
> > Niko: here's one of the cases that I was talking about; rounding down here causes us to begin to draw the pattern again (because it's not big enough) -- note the bright orange line on the right side of the image, even though the rectangles are exactly the same size.
> > 
> > I also question why we're drawing a significantly larger copy of the image for the rotation case; there's no scaling going on here, so it shouldn't have anything to do with pixelation when drawn on the destination. Is the target size being miscomputed?
> 
> We try to get the affected area not the screen in screen coordinates. While this works great for scaling, it looks a bit scaring on rotations and skews (like you noted above). I'm not sure if I remember correctly, but I think their were some problems to divide a transformation matrix in the two parts translation, scaling, skewing parts and the rotation. If we can divide the rotation from the other parts, we will get better results for rotated elements.

Ah, yes, our inability to decompose the transformation explains this, but it's not really a problem (except for efficiency's sake) as it stands, I was just wondering. Though, as you mention, leaving rotations out of this would get rid of some gaps in the rotated case, simply because we wouldn't be scaling.

> I think I can remember to have a negative example:
> 
> <pattern width="20" height="200" patternTransform="rotate(90)">
> <image width="20" height="200"/>
> </pattern>
> 
> IIRC we will create an image buffer with the size 200x20 and scale the image to match in this rect. The image will of course loose in quality.

This example looks correct to me, both on ToT and ToT+this patch, and matches Opera/Firefox. Perhaps I misunderstand?
Comment 27 Simon Fraser (smfr) 2011-08-23 10:59:39 PDT
TransformationMatrix can decompose just fine.
Comment 28 Tim Horton 2011-08-23 11:05:09 PDT
(In reply to comment #27)
> TransformationMatrix can decompose just fine.

Indeed it can, now that I look at the code. I'll see if we can't make use of that.
Comment 29 Tim Horton 2011-08-24 17:30:43 PDT
Created attachment 105102 [details]
Clear Z-rotation before determining the destination pattern tile size

This patch seems to solve all of the problems except between adjacent rectangles during *scaling*, which is not something that we expect to fix, and is a problem in many other places. I next have to understand what to do with the tests a little better, but the results from the SVG subdirectory so far look promising. I'm in the middle of making a workable test uniquely for this, because most of the changes in the existing tests are extremely subtle.
Comment 30 WebKit Review Bot 2011-08-24 17:55:24 PDT
Comment on attachment 105102 [details]
Clear Z-rotation before determining the destination pattern tile size

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

New failing tests:
svg/custom/pattern-rotate.svg
Comment 31 Tim Horton 2011-08-24 18:09:46 PDT
Created attachment 105112 [details]
Same as last, plus a test

Chromium will need rebaselining (as will some Mac tests).
Comment 32 WebKit Review Bot 2011-08-24 19:06:57 PDT
Comment on attachment 105112 [details]
Same as last, plus a test

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

New failing tests:
svg/custom/pattern-rotate.svg
svg/custom/pattern-rotate-gaps.svg
Comment 33 WebKit Review Bot 2011-08-24 20:12:18 PDT
Comment on attachment 105112 [details]
Same as last, plus a test

Attachment 105112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9513173
Comment 34 WebKit Review Bot 2011-08-24 20:22:37 PDT
Comment on attachment 105112 [details]
Same as last, plus a test

Attachment 105112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9512160
Comment 35 Dirk Schulze 2011-08-24 22:40:20 PDT
Comment on attachment 105112 [details]
Same as last, plus a test

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

because of the use of TransformationMatrix.

> Source/WebCore/ChangeLog:11
> +        Introduce wkCGPatternCreateWithImageAndTransform, and make use of it
> +        when tiling patterns in both directions. This helps to prevent
> +        pixel-cracking along pattern tiling boundaries.

This won't have affect to other platforms. Is their a negative visual affect left on other platforms? If so I'd like to have a more general solution, that works for all ports.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:133
> +    TransformationMatrix::DecomposedType decomposition;
> +    TransformationMatrix tm = transform.toTransformationMatrix();
> +    tm.decompose(decomposition);
> +    decomposition.quaternionZ = 0.0;
> +    tm.recompose(decomposition);
> +    transform = tm.toAffineTransform();

Can't we port recompose and decompose to AffineTransform? If z = 0 anyway, it shouldn't be a big problem, but avoids creating a TransformationMatrix. IIRC AffineTransform uses floats while TransformationMatrix uses double (didn't check it). If so, we have unnecessary transformations float --> double --> float. Also TransformationMatrix needs 16 floating point numbers, while AffineTransform just needs 6.

Btw. decomposition.quaternionZ = 0; .0 is superflous.
Comment 36 Tim Horton 2011-08-24 23:40:17 PDT
(In reply to comment #35)
> (From update of attachment 105112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105112&action=review
> 
> because of the use of TransformationMatrix.
> 
> > Source/WebCore/ChangeLog:11
> > +        Introduce wkCGPatternCreateWithImageAndTransform, and make use of it
> > +        when tiling patterns in both directions. This helps to prevent
> > +        pixel-cracking along pattern tiling boundaries.
> 
> This won't have affect to other platforms. Is their a negative visual affect left on other platforms? If so I'd like to have a more general solution, that works for all ports.

This isn't something we can fix in a general way, it's basically working around a CoreGraphics bug (actually, a accuracy vs. performance tradeoff which has already been decided). It'll have to be fixed platform to platform, and I wouldn't be surprised to find that it already works on some platforms.

> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:133
> > +    TransformationMatrix::DecomposedType decomposition;
> > +    TransformationMatrix tm = transform.toTransformationMatrix();
> > +    tm.decompose(decomposition);
> > +    decomposition.quaternionZ = 0.0;
> > +    tm.recompose(decomposition);
> > +    transform = tm.toAffineTransform();
> 
> Can't we port recompose and decompose to AffineTransform? If z = 0 anyway, it shouldn't be a big problem, but avoids creating a TransformationMatrix. IIRC AffineTransform uses floats while TransformationMatrix uses double (didn't check it). If so, we have unnecessary transformations float --> double --> float. Also TransformationMatrix needs 16 floating point numbers, while AffineTransform just needs 6.

Am I missing it (it's late!) or is there no function to decompose/recompose an AffineTranform? Also, it looks like they both use doubles.

> Btw. decomposition.quaternionZ = 0; .0 is superflous.

Sure.
Comment 37 Dirk Schulze 2011-08-25 10:20:21 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 105112 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=105112&action=review
> > 
> > because of the use of TransformationMatrix.
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        Introduce wkCGPatternCreateWithImageAndTransform, and make use of it
> > > +        when tiling patterns in both directions. This helps to prevent
> > > +        pixel-cracking along pattern tiling boundaries.
> > 
> > This won't have affect to other platforms. Is their a negative visual affect left on other platforms? If so I'd like to have a more general solution, that works for all ports.
> 
> This isn't something we can fix in a general way, it's basically working around a CoreGraphics bug (actually, a accuracy vs. performance tradeoff which has already been decided). It'll have to be fixed platform to platform, and I wouldn't be surprised to find that it already works on some platforms.
Does it even exist on other platforms? I don't know a way to influence quality of patterns in Qt or Cairo. If it is just an issue with CG, than I'm fine with the fix.

> 
> > > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:133
> > > +    TransformationMatrix::DecomposedType decomposition;
> > > +    TransformationMatrix tm = transform.toTransformationMatrix();
> > > +    tm.decompose(decomposition);
> > > +    decomposition.quaternionZ = 0.0;
> > > +    tm.recompose(decomposition);
> > > +    transform = tm.toAffineTransform();
> > 
> > Can't we port recompose and decompose to AffineTransform? If z = 0 anyway, it shouldn't be a big problem, but avoids creating a TransformationMatrix. IIRC AffineTransform uses floats while TransformationMatrix uses double (didn't check it). If so, we have unnecessary transformations float --> double --> float. Also TransformationMatrix needs 16 floating point numbers, while AffineTransform just needs 6.
> 
> Am I missing it (it's late!) or is there no function to decompose/recompose an AffineTranform? Also, it looks like they both use doubles.
AffineTranform is using doubles, yes. And it lacks the decompose functions. But we already have an affine matrix. So you could do a lot of the decomposing a lot faster for an affine matrix if you implement it. It would omit creating a TransformationMatrix with 16 additional doubles.

> 
> > Btw. decomposition.quaternionZ = 0; .0 is superflous.
> 
> Sure.
Comment 38 Tim Horton 2011-08-30 16:18:19 PDT
Created attachment 105715 [details]
patch, hopefully builds on mac this time
Comment 39 WebKit Review Bot 2011-08-30 16:20:56 PDT
Attachment 105715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Tim Horton 2011-08-30 16:24:48 PDT
I'd like to get this fix in ASAP, so I added a bug https://bugs.webkit.org/show_bug.cgi?id=67242 to track fixing the round-tripping through TransformationMatrix.

Also, it fails the style bot because of using argument names in WebKitSystemInterface.h which don't add any information, something which we don't seem to have a consistent decision about. Do we have a preference for that particular file?
Comment 41 Tim Horton 2011-08-30 16:27:57 PDT
This will also cause svg/custom/pattern-rotate.svg and the newly-added svg/custom/pattern-rotate-gaps.svg to need rebaselining on other platforms, but I'll take care of that when it lands and I can get results.
Comment 42 Darin Adler 2011-08-30 16:32:13 PDT
Comment on attachment 105715 [details]
patch, hopefully builds on mac this time

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

> Source/WebCore/platform/graphics/cg/PatternCG.cpp:73
> +    // The pattern will release the tile when it's done rendering in patternReleaseCallback
> +    tileImage()->ref();

Why does this need to be moved?

> Source/WebCore/platform/graphics/cg/PatternCG.cpp:85
> -        kCGPatternTilingConstantSpacing, TRUE, &patternCallbacks);
> +                           kCGPatternTilingConstantSpacing, TRUE, &patternCallbacks);

This change should not be made. The old code conforms to WebKit coding standards and the new indenting does not.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:45
> +    static void clearAffineTransform2DRotation(AffineTransform&);

I’m not sure this function needs the word Affine or even the word Transform in its name, since this is C++ with overloading.
Comment 43 Tim Horton 2011-08-30 16:43:20 PDT
Created attachment 105719 [details]
patch, fixing Darin's comments
Comment 44 Darin Adler 2011-08-30 16:45:46 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

> Source/WebCore/platform/graphics/cg/PatternCG.cpp:68
> +    // If we're repeating in both directions, we can use image-backed patterns
> +    // instead of custom patterns, and fix tiling-edge pixel cracks

In WebKit coding style we use a period at the end of a sentence like this one. I would say “avoid” rather than “fix”.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:120
> +        // Ignore 2D rotation, as it doesn't affect the size of the tile

Again, periods at the ends of comments, even ones that aren’t full sentences.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:128
> +    // FIXME67242: Don't round-trip through TransformationMatrix.

This is not our usual format. Ramming the bug number up against FIXME is not how we do it.

I’m not sure this really needs a FIXME. Sure, it would be more elegant, but what’s the big deal? Probably overkill to include the comment. Unless you said in the comment *why* it needs to be fixed.
Comment 45 WebKit Review Bot 2011-08-30 16:47:01 PDT
Attachment 105719 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Tim Horton 2011-08-30 16:54:41 PDT
(In reply to comment #44)
> (From update of attachment 105719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105719&action=review
> 
> > Source/WebCore/platform/graphics/cg/PatternCG.cpp:68
> > +    // If we're repeating in both directions, we can use image-backed patterns
> > +    // instead of custom patterns, and fix tiling-edge pixel cracks
> 
> In WebKit coding style we use a period at the end of a sentence like this one. I would say “avoid” rather than “fix”.
> 
> > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:120
> > +        // Ignore 2D rotation, as it doesn't affect the size of the tile
> 
> Again, periods at the ends of comments, even ones that aren’t full sentences.
> 
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:128
> > +    // FIXME67242: Don't round-trip through TransformationMatrix.
> 
> This is not our usual format. Ramming the bug number up against FIXME is not how we do it.
> 
> I’m not sure this really needs a FIXME. Sure, it would be more elegant, but what’s the big deal? Probably overkill to include the comment. Unless you said in the comment *why* it needs to be fixed.

I clearly need a trip back to the style page.

I've made these changes locally; I don't see any reason to clog up the EWS bots since they don't change any code, so I'll keep them here.

I agree about the FIXME, now that you mention it -- it's not actually *broken* (just a very minor performance difference), so the bug should suffice.
Comment 47 WebKit Review Bot 2011-08-30 17:46:12 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

New failing tests:
svg/custom/pattern-rotate.svg
svg/custom/pattern-rotate-gaps.svg
Comment 48 Dirk Schulze 2011-08-30 21:55:15 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

r- because of the build failures and style issues.

>> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:128
>> +    // FIXME67242: Don't round-trip through TransformationMatrix.
> 
> This is not our usual format. Ramming the bug number up against FIXME is not how we do it.
> 
> I’m not sure this really needs a FIXME. Sure, it would be more elegant, but what’s the big deal? Probably overkill to include the comment. Unless you said in the comment *why* it needs to be fixed.

I don't agree. First, I think this should be fixed before landing. SVGImageBufferTools is not really fast, we shouldn't make it slower with unnecessary copy operations. We never use TransformationMatrix in SVG, and even if we have to switch to TransformationMatrix for supporting semi-3D, we can't use this function as is. Second, if we add a fix me instead of fixing it at once, it is very likely that it will never get fixed. I don't see a big deal to add a new decompose function to AffineTransform. The necessary coder for affine matrices can be found online.
Comment 49 Dirk Schulze 2011-08-30 22:06:00 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:122
> +        SVGImageBufferTools::clear2DRotation(absoluteTransformIgnoringRotation);
> +        FloatRect absoluteTileBoundaries = absoluteTransformIgnoringRotation.mapRect(tileBoundaries);

Have you tried to make this a inherent part of calculateTransformationToOutermostSVGCoordinateSystem? This could be interesting for masking and clipping as well. It might need more work to check the behavior of SVGFilter. Nevertheless, it would be interesting for filters as well!

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:132
> +    decomposition.quaternionZ = 0.0;

just 0, not 0.0.
Comment 50 Simon Fraser (smfr) 2011-08-30 22:16:52 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

Tim: you could land the WebKitSystemInterface parts of this first to reduce patch churn if you like. r=me on those parts.
Comment 51 Alexey Proskuryakov 2011-08-30 23:46:53 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

> LayoutTests/svg/custom/pattern-rotate-gaps.svg:22
> +      <tspan x="0" y="0">For this test case to be successful, the</tspan>
> +      <tspan x="0" y="15">square should be pure white. </tspan>

FWIW, this is not what I'm seeing in expected results - there is a rotated white square inside a black one.
Comment 52 Nikolas Zimmermann 2011-08-31 05:48:47 PDT
Comment on attachment 105719 [details]
patch, fixing Darin's comments

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

>> LayoutTests/svg/custom/pattern-rotate-gaps.svg:22
>> +      <tspan x="0" y="15">square should be pure white. </tspan>
> 
> FWIW, this is not what I'm seeing in expected results - there is a rotated white square inside a black one.

Yeah, this should better read "For this test case to be successful, the rotated shape in the black square should be pure white.

>> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:122
>> +        FloatRect absoluteTileBoundaries = absoluteTransformIgnoringRotation.mapRect(tileBoundaries);
> 
> Have you tried to make this a inherent part of calculateTransformationToOutermostSVGCoordinateSystem? This could be interesting for masking and clipping as well. It might need more work to check the behavior of SVGFilter. Nevertheless, it would be interesting for filters as well!

Hm, I don't think you want this for non-patterns. For example when rendering into a mask image buffer, you want the content to be equal as if you would render the <mask> children on screen, including any rotation etc. Of course the underlying mask image buffer must be large enough to hold the transformed content -- here you can't ignore the rotation.
(Same for my RenderSVGDuplicate work - where I'm using SVGImageBufferTools to render the referenced <use> content to an image buffer - rotation needs to be included there as well).

>>> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:128
>>> +    // FIXME67242: Don't round-trip through TransformationMatrix.
>> 
>> This is not our usual format. Ramming the bug number up against FIXME is not how we do it.
>> 
>> I’m not sure this really needs a FIXME. Sure, it would be more elegant, but what’s the big deal? Probably overkill to include the comment. Unless you said in the comment *why* it needs to be fixed.
> 
> I don't agree. First, I think this should be fixed before landing. SVGImageBufferTools is not really fast, we shouldn't make it slower with unnecessary copy operations. We never use TransformationMatrix in SVG, and even if we have to switch to TransformationMatrix for supporting semi-3D, we can't use this function as is. Second, if we add a fix me instead of fixing it at once, it is very likely that it will never get fixed. I don't see a big deal to add a new decompose function to AffineTransform. The necessary coder for affine matrices can be found online.

I tend to agree with Dirk - though if you want to work on this right after landing this patch, I'm fine with it.
Ideally, you'd refactor the decomposition code first in a seperated patch, and then make use of it here, after it landed.

> Source/WebKit2/ChangeLog:9
> +        Introduce wkCGPatternCreateWithImageAndTransform.

I agree this could be landed first and
Comment 53 Dirk Schulze 2011-08-31 09:37:44 PDT
(In reply to comment #52)
> > Have you tried to make this a inherent part of calculateTransformationToOutermostSVGCoordinateSystem? This could be interesting for masking and clipping as well. It might need more work to check the behavior of SVGFilter. Nevertheless, it would be interesting for filters as well!
> 
> Hm, I don't think you want this for non-patterns. For example when rendering into a mask image buffer, you want the content to be equal as if you would render the <mask> children on screen, including any rotation etc. Of course the underlying mask image buffer must be large enough to hold the transformed content -- here you can't ignore the rotation.
> (Same for my RenderSVGDuplicate work - where I'm using SVGImageBufferTools to render the referenced <use> content to an image buffer - rotation needs to be included there as well).

To your mask example. You have of course apply the rotation to the context  right before masking. But the quality would get better and we might don't need to invalidate the mask if just the rotation changes (see time travel example of Erik Dahlström). Right now we invalidate on every animation step - unnecessarily!

But I agree that it doesn't need to get tested with this patch and of course needs more checking for <use> and filter.
Comment 54 Tim Horton 2011-08-31 18:51:14 PDT
Created attachment 105879 [details]
Patch 1 of 2: just the WKSI portion (should work with Windows now too)
Comment 55 WebKit Review Bot 2011-08-31 18:55:38 PDT
Attachment 105879 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:194:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:194:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/WebKitSystemInterface.h:172:  The parameter name "transform" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Simon Fraser (smfr) 2011-08-31 21:42:30 PDT
Comment on attachment 105879 [details]
Patch 1 of 2: just the WKSI portion (should work with Windows now too)

r=me. Leaving cq? because I'm not sure whether it's OK to have the commit queue automatically commit this.
Comment 57 Simon Fraser (smfr) 2011-08-31 21:44:49 PDT
(In reply to comment #48)
> I don't agree. First, I think this should be fixed before landing. SVGImageBufferTools is not really fast, we shouldn't make it slower with unnecessary copy operations. We never use TransformationMatrix in SVG, and even if we have to switch to TransformationMatrix for supporting semi-3D, we can't use this function as is. Second, if we add a fix me instead of fixing it at once, it is very likely that it will never get fixed. I don't see a big deal to add a new decompose function to AffineTransform. The necessary coder for affine matrices can be found online.

This patch has been held up for too long by this issue, and we'd really like to commit it soon. I'll make sure that Tim addresses the AffineTransform decomposition issue soon; it won't get left this way for long.
Comment 58 Dirk Schulze 2011-09-01 01:09:32 PDT
(In reply to comment #57)
> (In reply to comment #48)
> > I don't agree. First, I think this should be fixed before landing. SVGImageBufferTools is not really fast, we shouldn't make it slower with unnecessary copy operations. We never use TransformationMatrix in SVG, and even if we have to switch to TransformationMatrix for supporting semi-3D, we can't use this function as is. Second, if we add a fix me instead of fixing it at once, it is very likely that it will never get fixed. I don't see a big deal to add a new decompose function to AffineTransform. The necessary coder for affine matrices can be found online.
> 
> This patch has been held up for too long by this issue, and we'd really like to commit it soon. I'll make sure that Tim addresses the AffineTransform decomposition issue soon; it won't get left this way for long.

Ok.
Comment 59 Tim Horton 2011-09-01 11:03:27 PDT
WKSI changes landed in 94317
Comment 60 Tim Horton 2011-09-01 12:16:21 PDT
Created attachment 106004 [details]
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern
Comment 61 WebKit Review Bot 2011-09-01 12:50:18 PDT
Comment on attachment 106004 [details]
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern 

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

New failing tests:
svg/custom/pattern-rotate.svg
svg/custom/pattern-rotate-gaps.svg
Comment 62 Tim Horton 2011-09-01 13:27:34 PDT
Landed as r94338, though without the needed chromium-linux pixel rebaselines, as -- though they were generated on my chromium-linux install, don't seem to match the bots. I'll grab them and commit them shortly once the bots cycle.