Bug 41396 - Pattern is rasterized
Summary: Pattern is rasterized
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 38704 41603 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-30 02:08 PDT by Dusan Maliarik
Modified: 2010-08-22 03:35 PDT (History)
11 users (show)

See Also:


Attachments
pattern scale test case (436 bytes, image/svg+xml)
2010-06-30 02:09 PDT, Dusan Maliarik
no flags Details
Patch (deleted)
2010-08-19 04:59 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dusan Maliarik 2010-06-30 02:08:52 PDT
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)
Comment 1 Dusan Maliarik 2010-06-30 02:09:59 PDT
Created attachment 60101 [details]
pattern scale test case
Comment 2 Eric Seidel (no email) 2010-06-30 02:31:20 PDT
I'm not sure if this is expected behavior or not.
Comment 3 Dusan Maliarik 2010-06-30 02:49:35 PDT
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.
Comment 4 Dirk Schulze 2010-06-30 02:53:06 PDT
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).
Comment 5 Dusan Maliarik 2010-06-30 02:56:11 PDT
I searched for similar bug reports but couldn't find any. Anyway, is there any workaround we can use until it's fixed?
Comment 6 Nikolas Zimmermann 2010-07-09 23:24:47 PDT
(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.
Comment 7 Nikolas Zimmermann 2010-08-19 04:55:43 PDT
*** Bug 38704 has been marked as a duplicate of this bug. ***
Comment 8 Nikolas Zimmermann 2010-08-19 04:55:50 PDT
*** Bug 41603 has been marked as a duplicate of this bug. ***
Comment 9 Nikolas Zimmermann 2010-08-19 04:59:26 PDT
Created attachment 64828 [details]
Patch

Patch is large, as it contains several new tests and pixel results.
Comment 10 Nikolas Zimmermann 2010-08-19 05:33:21 PDT
Forgot to mention: integrated the testcase from Dusan, and the ones from the other bug reports, that I marked as duplicate in the patch.
Comment 11 Dirk Schulze 2010-08-19 05:36:13 PDT
Comment on attachment 64828 [details]
Patch

r=me With some changes discussed on IRC
Comment 12 Nikolas Zimmermann 2010-08-19 06:01:41 PDT
Landed in r65665.
Comment 13 WebKit Review Bot 2010-08-19 06:22:57 PDT
http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests)
Comment 14 Dirk Schulze 2010-08-19 06:52:02 PDT
(In reply to comment #13)
> http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests)

Not caused by this patch.
Comment 15 Eric Seidel (no email) 2010-08-19 07:36:01 PDT
Comment on attachment 64828 [details]
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.
Comment 16 Eric Seidel (no email) 2010-08-19 07:37:15 PDT
Just an FYI CC to other folks affected by SVG rendering tree changes.
Comment 17 Dirk Schulze 2010-08-19 07:52:20 PDT
(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.
Comment 18 Nikolas Zimmermann 2010-08-22 03:35:28 PDT
(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 :-)