|
Description
Tony Sung
2011-01-24 17:53:36 PST
Created attachment 80000 [details]
Primary test case
Created attachment 80280 [details]
Updated Primary Test Case
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. Probably a regression of fixing: - https://bugs.webkit.org/show_bug.cgi?id=38704 - https://bugs.webkit.org/show_bug.cgi?id=41603 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".
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.
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). 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. Created attachment 104267 [details]
preliminary partial patch
Created attachment 104268 [details]
Screenshot with patch (104267)
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. Created attachment 104272 [details]
[WORKAROUND] overdraw rects in pattern
Created attachment 104274 [details]
Screenshot with both patch (104267) and overdraw (104272)
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 Created attachment 104610 [details]
patch, fixes between-pattern-tiling cracks
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.
Created attachment 104612 [details]
style fix
Comment on attachment 104612 [details] style fix Attachment 104612 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9436568 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 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! 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? Created attachment 104773 [details]
example of overdraw (why rounding is bad)
Created attachment 104774 [details]
example of overdraw (why rounding is bad) -- SVG source
(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. (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? TransformationMatrix can decompose just fine. (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. 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 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 Created attachment 105112 [details]
Same as last, plus a test
Chromium will need rebaselining (as will some Mac tests).
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 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 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 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. (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. (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. Created attachment 105715 [details]
patch, hopefully builds on mac this time
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.
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? 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 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. Created attachment 105719 [details]
patch, fixing Darin's comments
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. 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.
(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 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 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 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 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 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 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 (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. Created attachment 105879 [details]
Patch 1 of 2: just the WKSI portion (should work with Windows now too)
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 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.
(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. (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. WKSI changes landed in 94317 Created attachment 106004 [details]
Patch 2 of 2: make use of wkCGPatternCreateWithImageAndTransform and clears 2D rotation when determining the size of a pattern
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 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. |