Bug 53055 – REGRESSION: Rendering artifacts on a rotated, pattern filled shape

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

[Tony Sung]

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.

[Tony Sung]

Created attachment 80000 [details]
Primary test case

[Deirdre Saoirse Moen]

rdar://problem/8910917

[Tony Sung]

Created attachment 80280 [details]
Updated Primary Test Case

[Tony Sung]

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.

[Tony Sung]

Probably a regression of fixing:
- https://bugs.webkit.org/show_bug.cgi?id=38704
- https://bugs.webkit.org/show_bug.cgi?id=41603

[Tony Sung]

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".

[Tim Horton]

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.

[Tim Horton]

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).

[Tim Horton]

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.

[Tim Horton]

Created attachment 104267 [details]
preliminary partial patch

[Tim Horton]

Created attachment 104268 [details]
Screenshot with patch (104267)

[Tim Horton]

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.

[Tim Horton]

Created attachment 104272 [details]
[WORKAROUND] overdraw rects in pattern

[Tim Horton]

Created attachment 104274 [details]
Screenshot with both patch (104267) and overdraw (104272)

[WebKit Review Bot]

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

[Tim Horton]

Created attachment 104610 [details]
patch, fixes between-pattern-tiling cracks

[WebKit Review Bot]

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.

[Tim Horton]

Created attachment 104612 [details]
style fix

[WebKit Review Bot]

Comment on attachment 104612 [details]
style fix

Attachment 104612 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9436568

[Nikolas Zimmermann]

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?

[Tim Horton]

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!

[Tim Horton]

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?

[Tim Horton]

Created attachment 104773 [details]
example of overdraw (why rounding is bad)

[Tim Horton]

Created attachment 104774 [details]
example of overdraw (why rounding is bad) -- SVG source

[Dirk Schulze]

(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.

[Tim Horton]

(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?

[Simon Fraser (smfr)]

TransformationMatrix can decompose just fine.

[Tim Horton]

(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.

[Tim Horton]

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.

[WebKit Review Bot]

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

[Tim Horton]

Created attachment 105112 [details]
Same as last, plus a test

Chromium will need rebaselining (as will some Mac tests).

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dirk Schulze]

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.

[Tim Horton]

(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.

[Dirk Schulze]

(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.

[Tim Horton]

Created attachment 105715 [details]
patch, hopefully builds on mac this time

[WebKit Review Bot]

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.

[Tim Horton]

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?

[Tim Horton]

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.

[Darin Adler]

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.

[Tim Horton]

Created attachment 105719 [details]
patch, fixing Darin's comments

[Darin Adler]

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.

[WebKit Review Bot]

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.

[Tim Horton]

(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.

[WebKit Review Bot]

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

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Simon Fraser (smfr)]

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.

[Alexey Proskuryakov]

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.

[Nikolas Zimmermann]

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

[Dirk Schulze]

(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.

[Tim Horton]

Created attachment 105879 [details]
Patch 1 of 2: just the WKSI portion (should work with Windows now too)

[WebKit Review Bot]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

(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.

[Dirk Schulze]

(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.

[Tim Horton]

WKSI changes landed in 94317

[Tim Horton]

Created attachment 106004 [details]
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern

[WebKit Review Bot]

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

[Tim Horton]

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.