Bug 41396 – Pattern is rasterized

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

[Dusan Maliarik]

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)

[Dusan Maliarik]

Created attachment 60101 [details]
pattern scale test case

[Eric Seidel (no email)]

I'm not sure if this is expected behavior or not.

[Dusan Maliarik]

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.

[Dirk Schulze]

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

[Dusan Maliarik]

I searched for similar bug reports but couldn't find any. Anyway, is there any workaround we can use until it's fixed?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

*** Bug 38704 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 41603 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 64828 [details]
Patch

Patch is large, as it contains several new tests and pixel results.

[Nikolas Zimmermann]

Forgot to mention: integrated the testcase from Dusan, and the ones from the other bug reports, that I marked as duplicate in the patch.

[Dirk Schulze]

Comment on attachment 64828 [details]
Patch

r=me With some changes discussed on IRC

[Nikolas Zimmermann]

Landed in r65665.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests)

[Dirk Schulze]

(In reply to comment #13)
> http://trac.webkit.org/changeset/65665 might have broken SnowLeopard Intel Release (Tests)

Not caused by this patch.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Just an FYI CC to other folks affected by SVG rendering tree changes.

[Dirk Schulze]

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

[Nikolas Zimmermann]

(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 :-)