41396 2010-06-30 02:08:52 -0700 Pattern is rasterized 2010-08-22 03:35:28 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) PC OS X 10.6 RESOLVED FIXED P2 Normal --- 1 dusan.maliarik zimmermann abarth eric jamesr kbr krit pkchan+bugzilla=webkit rspierer s3lance simon.fraser webkit.review.bot zimmermann oldest_to_newest 244694 0 dusan.maliarik 2010-06-30 02:08:52 -0700 After scaling an element with a pattern fill, the pattern is pixelized. This effectively cripples any usage of patterns. It becomes even bigger issue when one wants to use different coordinate space. Let's say our SVG is 500px for 500px, and we use meter units, which means the whole graphic is scaled up, with viewBox around 0 0 50 50. Sample SVG demonstrating this issue is attached, please compare with other SVG implementations (mozilla's) 244695 1 60101 dusan.maliarik 2010-06-30 02:09:59 -0700 Created attachment 60101 pattern scale test case 244699 2 eric 2010-06-30 02:31:20 -0700 I'm not sure if this is expected behavior or not. 244703 3 dusan.maliarik 2010-06-30 02:49:35 -0700 There is no mention of rasterization in SVG spec. so I believe this is some premature optimization (evil) that kicks in, and destroys developer's day ;) Otherwise it works like a charm, much faster than mozilla's impl. 244704 4 krit 2010-06-30 02:53:06 -0700 The Pattern and Gradient resources need to invalidate, on scaling or resizing the window. This doesn't happen at the moment, that's why you see the rasterization. We have some more bug reports about this. And this is also a duplication of another bug report (don't know the bug number atm). 244705 5 dusan.maliarik 2010-06-30 02:56:11 -0700 I searched for similar bug reports but couldn't find any. Anyway, is there any workaround we can use until it's fixed? 249476 6 zimmermann 2010-07-09 23:24:47 -0700 (In reply to comment #4) > The Pattern and Gradient resources need to invalidate, on scaling or resizing the window. This doesn't happen at the moment, that's why you see the rasterization. We have some more bug reports about this. And this is also a duplication of another bug report (don't know the bug number atm). No, this has nothing to do with invalidation. A pattern of a specific size is created and applied to an object, which gets scaled - this alone creates the artifacts. 266651 7 zimmermann 2010-08-19 04:55:43 -0700 *** Bug 38704 has been marked as a duplicate of this bug. *** 266654 8 zimmermann 2010-08-19 04:55:50 -0700 *** Bug 41603 has been marked as a duplicate of this bug. *** 266656 9 64828 zimmermann 2010-08-19 04:59:26 -0700 Created attachment 64828 Patch Patch is large, as it contains several new tests and pixel results. 266675 10 zimmermann 2010-08-19 05:33:21 -0700 Forgot to mention: integrated the testcase from Dusan, and the ones from the other bug reports, that I marked as duplicate in the patch. 266676 11 64828 krit 2010-08-19 05:36:13 -0700 Comment on attachment 64828 Patch r=me With some changes discussed on IRC 266687 12 zimmermann 2010-08-19 06:01:41 -0700 Landed in r65665. 266697 13 webkit.review.bot 2010-08-19 06:22:57 -0700 http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests) 266702 14 krit 2010-08-19 06:52:02 -0700 (In reply to comment #13) > http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests) Not caused by this patch. 266722 15 64828 eric 2010-08-19 07:36:01 -0700 Comment on attachment 64828 Patch WebCore/rendering/RenderSVGResourcePattern.cpp:191 + return FloatRect(attributes.x().valueAsPercentage() * objectBoundingBox.width() + objectBoundingBox.x(), Why this change? WebCore/rendering/RenderSVGResourcePattern.cpp:202 + AffineTransform RenderSVGResourcePattern::buildTileImageTransform(RenderObject* renderer, I've been told to avoid returning AffineTransforms as they're very expensive to copy. return PassOwnPtr<ImageBuffer>();: Can't you just reuturn 0 here? // Compensate rounding effects, as the absolute target rect is using floating-point numbers and the image buffer size is integer. I'm not sure I understand what you're doing here? WebCore/rendering/RenderSVGResourcePattern.cpp:263 + if (!tileImageTransform.isIdentity()) Does concatCTM assert it's not identity? I'm surprised we have to check outisde concatCTM. WebCore/rendering/SVGImageBufferTools.cpp:32 + AffineTransform SVGImageBufferTools::screenCoordinateSystemForRenderer(const RenderObject* renderer) Yeah, these should probably be changed to return the AffineTransform as a reference variable. At least according to smfr last time I talked with him about it. WebCore/rendering/SVGImageBufferTools.cpp:37 + ASSERT(current); This assert is redundant. No need to even put renderer in a local variable, the argument is already a local variable, I would just use it as the loop variable. WebCore/rendering/SVGImageBufferTools.cpp:81 + return IntSize(static_cast<int>(lroundf(size.width())), static_cast<int>(lroundf(size.height()))); Really? We don't already have a method like this on IntSize? Why wouldn't this go on IntSize? WebCore/rendering/SVGRenderSupport.h:83 + static const RenderObject* findTextRootObject(const RenderObject* start); We should just remove the RenderObject* one. I don't think I really fully understand the patch, but in general it looked fine. I'm surprised krit had 0 comments. :p I guess he made them all over IRC. Thanks for doing this. 266723 16 eric 2010-08-19 07:37:15 -0700 Just an FYI CC to other folks affected by SVG rendering tree changes. 266736 17 krit 2010-08-19 07:52:20 -0700 (In reply to comment #15) > (From update of attachment 64828 [details]) > WebCore/rendering/RenderSVGResourcePattern.cpp:191 > + return FloatRect(attributes.x().valueAsPercentage() * objectBoundingBox.width() + objectBoundingBox.x(), > Why this change? Because it was wrong not to do it. > > WebCore/rendering/RenderSVGResourcePattern.cpp:202 > + AffineTransform RenderSVGResourcePattern::buildTileImageTransform(RenderObject* renderer, > I've been told to avoid returning AffineTransforms as they're very expensive to copy. Not AffineTransform, maybe TransformationMatrix with it's 16 floats. > > return PassOwnPtr<ImageBuffer>();: > Can't you just reuturn 0 here? > > // Compensate rounding effects, as the absolute target rect is using floating-point numbers and the image buffer size is integer. > I'm not sure I understand what you're doing here? Because we may have a width or height with decimal places, but don't want artefacts on the tile. > > WebCore/rendering/RenderSVGResourcePattern.cpp:263 > + if (!tileImageTransform.isIdentity()) > Does concatCTM assert it's not identity? I'm surprised we have to check outisde concatCTM. tileImageTransform is the AffineTransform for the tile. Why shouldn't we check if it is identity? If it is, we don't need to transform the ctm. > > WebCore/rendering/SVGImageBufferTools.cpp:81 > + return IntSize(static_cast<int>(lroundf(size.width())), static_cast<int>(lroundf(size.height()))); > Really? We don't already have a method like this on IntSize? Why wouldn't this go on IntSize? Just IntRect has it. > > WebCore/rendering/SVGRenderSupport.h:83 > + static const RenderObject* findTextRootObject(const RenderObject* start); > We should just remove the RenderObject* one. No, the first is searching for the text root, the second one for the outermost svg renderer. > > I don't think I really fully understand the patch, but in general it looked fine. I'm surprised krit had 0 comments. :p I guess he made them all over IRC. Thanks for doing this. Like I wrote on my r=me comment, I gave comments on IRC. But thanks for rechecking the patch. Four eyes are better than two. 267673 18 zimmermann 2010-08-22 03:35:28 -0700 (In reply to comment #15) > (From update of attachment 64828 [details]) > WebCore/rendering/RenderSVGResourcePattern.cpp:191 > + return FloatRect(attributes.x().valueAsPercentage() * objectBoundingBox.width() + objectBoundingBox.x(), > Why this change? Because it was not correct before, we hacked around it with an additional transformation before. I just fixed it. > WebCore/rendering/RenderSVGResourcePattern.cpp:202 > + AffineTransform RenderSVGResourcePattern::buildTileImageTransform(RenderObject* renderer, > I've been told to avoid returning AffineTransforms as they're very expensive to copy. Going to fix this in my next patch. > > return PassOwnPtr<ImageBuffer>();: > Can't you just reuturn 0 here? #ifdef LOOSE_PASS_OWN_PTR PassOwnPtr(PtrType ptr) : m_ptr(ptr) { } PassOwnPtr& operator=(PtrType); ... // Remove this once we make all WebKit code compatible with stricter rules about PassOwnPtr. #define LOOSE_PASS_OWN_PTR My understanding is that the implicit 0 -> PassOwnPtr conversion is deprecated. > > // Compensate rounding effects, as the absolute target rect is using floating-point numbers and the image buffer size is integer. > I'm not sure I understand what you're doing here? I'm fixing scaling artefacts. ImageBuffers are using integer based sizes, and the content may use floating-point numbers, so we scale so it fits inside the integer based box. > > WebCore/rendering/RenderSVGResourcePattern.cpp:263 > + if (!tileImageTransform.isIdentity()) > Does concatCTM assert it's not identity? I'm surprised we have to check outisde concatCTM. It doesn't :( We should fix this ASAP, and remove lots of these checks from SVG code. > WebCore/rendering/SVGImageBufferTools.cpp:32 > + AffineTransform SVGImageBufferTools::screenCoordinateSystemForRenderer(const RenderObject* renderer) > Yeah, these should probably be changed to return the AffineTransform as a reference variable. At least according to smfr last time I talked with him about it. Fixing in my next patch, thanks for the hint. > > WebCore/rendering/SVGImageBufferTools.cpp:37 > + ASSERT(current); > This assert is redundant. No need to even put renderer in a local variable, the argument is already a local variable, I would just use it as the loop variable. Yes, fixing in my next patch. > > WebCore/rendering/SVGImageBufferTools.cpp:81 > + return IntSize(static_cast<int>(lroundf(size.width())), static_cast<int>(lroundf(size.height()))); > Really? We don't already have a method like this on IntSize? Why wouldn't this go on IntSize? Because I specifically need lroundf here, no ceil or floor solution. > > WebCore/rendering/SVGRenderSupport.h:83 > + static const RenderObject* findTextRootObject(const RenderObject* start); > We should just remove the RenderObject* one. As Dirk already said, there's findTextRootObject and findTreeRootObject, yes looks confusing, that's why there's a FIXME above, that it should be moved away :-) > > I don't think I really fully understand the patch, but in general it looked fine. I'm surprised krit had 0 comments. :p I guess he made them all over IRC. Thanks for doing this. Yeah, we generally talk a lot on IRC before I submit the acutal patch, and after submitting. Usually Dirk has comments :-) 60101 2010-06-30 02:09:59 -0700 2010-08-19 04:59:26 -0700 pattern scale test case webkitfail.svg image/svg+xml 436 dusan.maliarik PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciCiAgICB3aWR0aD0iNDAwcHgi IGhlaWdodD0iNDAwcHgiPgogICAgCiAgICA8ZGVmcz4KICAgICAgICA8cGF0dGVybiBpZD0iZmFp bCIKICAgICAgICAgICAgeD0iMCIgeT0iMCIKICAgICAgICAgICAgd2lkdGg9IjMwIiBoZWlnaHQ9 IjMwIiAKICAgICAgICAgICAgcGF0dGVyblVuaXRzPSJ1c2VyU3BhY2VPblVzZSI+CgogICAgICAg ICAgICA8Y2lyY2xlIGN4PSIxNSIgY3k9IjE1IiByPSIxNSIgZmlsbD0iI2YwMCIgLz4KICAgICAg ICA8L3BhdHRlcm4+CiAgICA8L2RlZnM+CgogICAgPHJlY3QgeD0iLTQwIiB5PSItNDAiIHdpZHRo PSI4MCIgaGVpZ2h0PSI4MCIgZmlsbD0idXJsKCNmYWlsKSIKICAgICAgICB0cmFuc2Zvcm09InRy YW5zbGF0ZSgyMDAsMjAwKSBzY2FsZSg0KSIgLz4KPC9zdmc+Cg== 64828 2010-08-19 04:59:26 -0700 2010-08-19 07:36:01 -0700 Patch PatternPatch.diff text/plain 0 zimmermann