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


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 PST, Tim Horton
no flags Details
Updated similar HTML test case (592 bytes, text/html)
2011-08-16 17:45 PST, Tim Horton
no flags Details
Screen shot after changing rounding mode for image size (263.51 KB, image/png)
2011-08-16 18:00 PST, Tim Horton
no flags Details
preliminary partial patch (2.41 KB, patch)
2011-08-17 16:17 PST, Tim Horton
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Screenshot with patch (104267) (37.36 KB, image/png)
2011-08-17 16:20 PST, Tim Horton
no flags Details
[WORKAROUND] overdraw rects in pattern (2.92 KB, image/svg+xml)
2011-08-17 16:24 PST, Tim Horton
no flags Details
Screenshot with both patch (104267) and overdraw (104272) (36.47 KB, image/png)
2011-08-17 16:24 PST, Tim Horton
no flags Details
patch, fixes between-pattern-tiling cracks (790.62 KB, patch)
2011-08-19 19:32 PST, Tim Horton
no flags Review Patch | Details | Formatted Diff | Diff
style fix (789.79 KB, patch)
2011-08-19 19:38 PST, Tim Horton
webkit.review.bot: commit‑queue-
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
example of overdraw (why rounding is bad) (28.18 KB, image/png)
2011-08-22 17:36 PST, 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 PST, Tim Horton
no flags Details
Clear Z-rotation before determining the destination pattern tile size (792.01 KB, patch)
2011-08-24 17:30 PST, Tim Horton
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Same as last, plus a test (812.16 KB, patch)
2011-08-24 18:09 PST, Tim Horton
krit: review-
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch, hopefully builds on mac this time (815.94 KB, patch)
2011-08-30 16:18 PST, Tim Horton
no flags Review Patch | Details | Formatted Diff | Diff
patch, fixing Darin's comments (815.10 KB, patch)
2011-08-30 16:43 PST, Tim Horton
krit: review-
thorton: commit‑queue-
Review Patch | 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 PST, Tim Horton
simon.fraser: review+
Review Patch | 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 PST, Tim Horton
simon.fraser: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-01-24 17:54:14 PST -------
Created an attachment (id=80000) [details]
Primary test case
------- Comment #2 From 2011-01-24 23:54:21 PST -------
rdar://problem/8910917
------- Comment #3 From 2011-01-26 17:38:56 PST -------
Created an attachment (id=80280) [details]
Updated Primary Test Case
------- Comment #4 From 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 From 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 From 2011-01-27 21:21:53 PST -------
Created an attachment (id=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 From 2011-08-16 16:35:20 PST -------
Created an attachment (id=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 From 2011-08-16 17:45:12 PST -------
Created an attachment (id=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 From 2011-08-16 18:00:17 PST -------
Created an attachment (id=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 From 2011-08-17 16:17:00 PST -------
Created an attachment (id=104267) [details]
preliminary partial patch
------- Comment #11 From 2011-08-17 16:20:20 PST -------
Created an attachment (id=104268) [details]
Screenshot with patch (104267)
------- Comment #12 From 2011-08-17 16:22:51 PST -------
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 From 2011-08-17 16:24:02 PST -------
Created an attachment (id=104272) [details]
[WORKAROUND] overdraw rects in pattern
------- Comment #14 From 2011-08-17 16:24:43 PST -------
Created an attachment (id=104274) [details]
Screenshot with both patch (104267) and overdraw (104272)
------- Comment #15 From 2011-08-18 22:34:51 PST -------
(From update of attachment 104267 [details])
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 From 2011-08-19 19:32:11 PST -------
Created an attachment (id=104610) [details]
patch, fixes between-pattern-tiling cracks
------- Comment #17 From 2011-08-19 19:35:18 PST -------
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 From 2011-08-19 19:38:33 PST -------
Created an attachment (id=104612) [details]
style fix
------- Comment #19 From 2011-08-19 20:46:01 PST -------
(From update of attachment 104612 [details])
Attachment 104612 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9436568
------- Comment #20 From 2011-08-20 00:13:54 PST -------
(From update of attachment 104612 [details])
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 From 2011-08-20 13:27:20 PST -------
(From update of attachment 104612 [details])
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 From 2011-08-22 17:36:22 PST -------
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 From 2011-08-22 17:36:50 PST -------
Created an attachment (id=104773) [details]
example of overdraw (why rounding is bad)
------- Comment #24 From 2011-08-22 17:37:24 PST -------
Created an attachment (id=104774) [details]
example of overdraw (why rounding is bad) -- SVG source
------- Comment #25 From 2011-08-23 01:53:22 PST -------
(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 From 2011-08-23 10:56:40 PST -------
(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 From 2011-08-23 10:59:39 PST -------
TransformationMatrix can decompose just fine.
------- Comment #28 From 2011-08-23 11:05:09 PST -------
(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 From 2011-08-24 17:30:43 PST -------
Created an attachment (id=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 From 2011-08-24 17:55:24 PST -------
(From update of attachment 105102 [details])
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 From 2011-08-24 18:09:46 PST -------
Created an attachment (id=105112) [details]
Same as last, plus a test

Chromium will need rebaselining (as will some Mac tests).
------- Comment #32 From 2011-08-24 19:06:57 PST -------
(From update of attachment 105112 [details])
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 From 2011-08-24 20:12:18 PST -------
(From update of attachment 105112 [details])
Attachment 105112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9513173
------- Comment #34 From 2011-08-24 20:22:37 PST -------
(From update of attachment 105112 [details])
Attachment 105112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9512160
------- Comment #35 From 2011-08-24 22:40:20 PST -------
(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.

> 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 From 2011-08-24 23:40:17 PST -------
(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.

> > 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 From 2011-08-25 10:20:21 PST -------
(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 105112 [details] [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 From 2011-08-30 16:18:19 PST -------
Created an attachment (id=105715) [details]
patch, hopefully builds on mac this time
------- Comment #39 From 2011-08-30 16:20:56 PST -------
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 From 2011-08-30 16:24:48 PST -------
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 From 2011-08-30 16:27:57 PST -------
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 From 2011-08-30 16:32:13 PST -------
(From update of attachment 105715 [details])
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 From 2011-08-30 16:43:20 PST -------
Created an attachment (id=105719) [details]
patch, fixing Darin's comments
------- Comment #44 From 2011-08-30 16:45:46 PST -------
(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.
------- Comment #45 From 2011-08-30 16:47:01 PST -------
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 From 2011-08-30 16:54:41 PST -------
(In reply to comment #44)
> (From update of attachment 105719 [details] [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 From 2011-08-30 17:46:12 PST -------
(From update of attachment 105719 [details])
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 From 2011-08-30 21:55:15 PST -------
(From update of attachment 105719 [details])
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 From 2011-08-30 22:06:00 PST -------
(From update of attachment 105719 [details])
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 From 2011-08-30 22:16:52 PST -------
(From update of attachment 105719 [details])
Tim: you could land the WebKitSystemInterface parts of this first to reduce patch churn if you like. r=me on those parts.
------- Comment #51 From 2011-08-30 23:46:53 PST -------
(From update of attachment 105719 [details])
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 From 2011-08-31 05:48:47 PST -------
(From update of attachment 105719 [details])
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 From 2011-08-31 09:37:44 PST -------
(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 From 2011-08-31 18:51:14 PST -------
Created an attachment (id=105879) [details]
Patch 1 of 2: just the WKSI portion (should work with Windows now too)
------- Comment #55 From 2011-08-31 18:55:38 PST -------
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 From 2011-08-31 21:42:30 PST -------
(From update of attachment 105879 [details])
r=me. Leaving cq? because I'm not sure whether it's OK to have the commit queue automatically commit this.
------- Comment #57 From 2011-08-31 21:44:49 PST -------
(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 From 2011-09-01 01:09:32 PST -------
(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 From 2011-09-01 11:03:27 PST -------
WKSI changes landed in 94317
------- Comment #60 From 2011-09-01 12:16:21 PST -------
Created an attachment (id=106004) [details]
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern
------- Comment #61 From 2011-09-01 12:50:18 PST -------
(From update of attachment 106004 [details])
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 From 2011-09-01 13:27:34 PST -------
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.