[SVG] DisplayList-based patterns.

[SVG] DisplayList-based patterns.

Instead of building an ImageBuffer tile (with all the ills of record
time rasterization), build a DisplayList tile and use Skia's
new SkPictureShader mechanism to fill/stroke the shape.

To facilitate this, Pattern is extended to support both bitmap and
picture shaders (the latter only in repeatXY mode -- the only mode
currently used by SVG).

Patterns (and their SkShaders) are cached per RenderSVGResourcePattern
per client. Internally, SkPictureShader also caches its rasterized tile
so there should be no performance degradation.

A handful of tests (about 9) require minor rebaselining (and a couple
have been updated for incorrect patternUnits).

R=pdr@chromium.org,schenney@chromium.org,fs@opera.com,ed@opera.com
BUG=401814, 425278
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184271

[f(malita), 2014-08-08 00:59:57]

9 tests require minor rebaselines (the text diffs are due to corrected
patternUnits):
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[Erik Dahlström (inactive), 2014-08-08 08:01:02]

I really like the direction of this CL, thumbs up.

Regarding the performance, would it be possible to get some tests for this to
verify that we don't regress?

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/a...
File LayoutTests/svg/custom/absolute-root-position-masking.xhtml (right):

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/a...
LayoutTests/svg/custom/absolute-root-position-masking.xhtml:7: <svg:pattern
id="pattern" width="600" height="600" patternUnits = "userSpaceOnUse">
Nit: please remove the spaces around the equals sign, and same for the one just
below.

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/p...
File LayoutTests/svg/custom/pattern-in-defs.svg (right):

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/p...
LayoutTests/svg/custom/pattern-in-defs.svg:3: <pattern id="green" x="0" y="0"
width="100" height="100" patternUnits="userSpaceOnUse">
I'm curious, why was this change needed?

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:84: FloatRect
clientBoundingBox = renderer.objectBoundingBox();
I've been working a bit in this area too recently, trying to add support for
percentages in transforms (as a followup to
https://codereview.chromium.org/423093014/). Just something to be aware of,
because to resolve those percentages we need to pass in the relevant
RenderObject's objectBoundingBox when fetching the patternTransform
attribute/property since that gets resolved in the css transforms code.

Your changes probably won't make this harder, just a heads-up that I'll be
posting such a review shortly.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.cpp (left):

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.cpp:269: return
FloatRect(absoluteTargetRect.location(),
absoluteTargetRect.size().shrunkTo(maxImageBufferSize));
Does the new codepath have similar guards against large tile sizes? From
experience, it's fairly common that patterns get huge sizes internally even from
relatively innocent-looking svg content.

[Stephen White, 2014-08-08 14:00:48]

Great stuff!

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:82: const
SVGPatternElement& pattern, FloatRect& bounds, AffineTransform& transform) const
I prefer pointers over non-const refs for outparams, but it's not against Blink
style so I can't stop you. :)

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:90: if (bounds.width() <=
0 || bounds.height() <= 0)
Do we have a layout test that exercises this case?

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:101:
transform.scale(clientBoundingBox.width(), clientBoundingBox.height());
I'm guessing this assumes the passed-in transform is identity already? If so,
maybe that should be documented.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:229:
context.concatCTM(tileTransform);
Just wondering: could the tile transform (e.g., view box) ever cause us to draw
outside the picture bounds?

[f(malita), 2014-08-08 15:30:36]

On 2014/08/08 08:01:02, Erik Dahlström wrote:
> Regarding the performance, would it be possible to get some tests for this to
> verify that we don't regress?

I'll try to come up with some tests and post the results here.

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/a...
File LayoutTests/svg/custom/absolute-root-position-masking.xhtml (right):

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/a...
LayoutTests/svg/custom/absolute-root-position-masking.xhtml:7: <svg:pattern
id="pattern" width="600" height="600" patternUnits = "userSpaceOnUse">
On 2014/08/08 08:01:02, Erik Dahlström wrote:
> Nit: please remove the spaces around the equals sign, and same for the one
just
> below.

Done.

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/p...
File LayoutTests/svg/custom/pattern-in-defs.svg (right):

https://codereview.chromium.org/453653003/diff/40001/LayoutTests/svg/custom/p...
LayoutTests/svg/custom/pattern-in-defs.svg:3: <pattern id="green" x="0" y="0"
width="100" height="100" patternUnits="userSpaceOnUse">
On 2014/08/08 08:01:02, Erik Dahlström wrote:
> I'm curious, why was this change needed?

Before I've added tile clamping in Skia, the default OBB units were causing an
attempt to allocate some humongous tile (10000x10000) which was failing => pixel
diff.

With clamping in place everything works as before, but since the intent of the
test is clearly to use userSpaceOnUse units I figured we might as well take the
opportunity to fix it.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:82: const
SVGPatternElement& pattern, FloatRect& bounds, AffineTransform& transform) const
On 2014/08/08 14:00:48, Stephen White wrote:
> I prefer pointers over non-const refs for outparams, but it's not against
Blink
> style so I can't stop you. :)

The only reason I prefer refs is to avoid assert-not-null noise... I dislike
outparams in general :)

But this is simple enough to be inlined -- that way we avoid outparams
altogether. Done.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:84: FloatRect
clientBoundingBox = renderer.objectBoundingBox();
On 2014/08/08 08:01:02, Erik Dahlström wrote:
> I've been working a bit in this area too recently, trying to add support for
> percentages in transforms (as a followup to
> https://codereview.chromium.org/423093014/). Just something to be aware of,
> because to resolve those percentages we need to pass in the relevant
> RenderObject's objectBoundingBox when fetching the patternTransform
> attribute/property since that gets resolved in the css transforms code.
> 
> Your changes probably won't make this harder, just a heads-up that I'll be
> posting such a review shortly.

Thanks.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:90: if (bounds.width() <=
0 || bounds.height() <= 0)
On 2014/08/08 14:00:48, Stephen White wrote:
> Do we have a layout test that exercises this case?

Yes, apparently svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg hits that branch.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:101:
transform.scale(clientBoundingBox.width(), clientBoundingBox.height());
On 2014/08/08 14:00:48, Stephen White wrote:
> I'm guessing this assumes the passed-in transform is identity already? If so,
> maybe that should be documented.

This should be addressed by refactoring.

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:229:
context.concatCTM(tileTransform);
On 2014/08/08 14:00:48, Stephen White wrote:
> Just wondering: could the tile transform (e.g., view box) ever cause us to
draw
> outside the picture bounds?

I don't think so (or if it does, it should be OK to clip): the tile transform is
supposed to get us into "tile space", such that all pattern content coordinates
are mapped into the tile. The tile bounds should reflect the view box, so if
some content falls outside the bounds then it's not supposed to be part of the
tile anyway.

(also, SkPictureShader uses the picture bounds to size its internal tile bitmap,
so we are effectively clipping -- which I think verifies correctness)

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderingContext.cpp (left):

https://codereview.chromium.org/453653003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderingContext.cpp:269: return
FloatRect(absoluteTargetRect.location(),
absoluteTargetRect.size().shrunkTo(maxImageBufferSize));
On 2014/08/08 08:01:02, Erik Dahlström wrote:
> Does the new codepath have similar guards against large tile sizes? From
> experience, it's fairly common that patterns get huge sizes internally even
from
> relatively innocent-looking svg content.

Yes, just landed yesterday :) https://codereview.chromium.org/446243002

We also have a couple of tests that verify clamping.

[f(malita), 2014-08-11 01:15:49]

On 2014/08/08 15:30:36, Florin Malita wrote:
> On 2014/08/08 08:01:02, Erik Dahlström wrote:
> > Regarding the performance, would it be possible to get some tests for this
to
> > verify that we don't regress?
> 
> I'll try to come up with some tests and post the results here.


Ran some local pattern stress tests and found a couple of perf issues:

1) When fully invalidating the resource (and forcing a re-record), there's about
a 10% slowdown attributable to the SKP recording overhead (lots of allocations
showing up on profile). I think this should go away when a) using the
soon-to-come new SKP record engine + b) caching the standalone display list (the
idea mentioned in the header FIXME) such that we only re-record on resource
changes, not on client changes.

2) Even in repainting-only mode (no resource cache inval), there's a significant
slowdown. This is unexpected: as mentioned in the description, the Skia picture
shader is supposed to cache rendered tiles so the new flow should be roughly
equivalent. It turns out the persistence chain is broken on SKP deserialization:
Skia always instantiates a new SkPictureShader on playback and doesn't take
advantage of the caching done in Blink. I'll need to check with the Skia gurus
what the best way to connect these caches is.

TL;DR: there's a bit more work needed before this is landable, but I think the
foundation is sane.

[f(malita), 2014-10-21 18:29:37]

Time to revisit. What changed:

* new blazing-fast Skia recording backend
* picture shader tile cache fix (pending:
https://codereview.chromium.org/671683004/)

With these in place, perf numbers look much nicer. Running
http://mali.jedimasters.net/dump/pattern_bench.svg locally, I'm seeing:

* [animation on  => exercising re-recording on each frame]  FPS meter: 51 -> 60
* [animation off => exercising tile caching] continuous paint time: 1.1ms ->
1.1ms

So I'm actually seeing a significant FPS improvement when re-recording the whole
scene - yay!

Current rebaselines:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[fs, 2014-10-21 20:12:35]

Awesome! =D

core/ bits LGTM (platform/ bits too, but not owner).

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:831: crbug.com/401814
svg/W3C-SVG-1.1/pservers-grad-06-b.svg [ NeedsRebaseline ]
I noticed that this (and other pattern with skew transforms) looked a bit
smeared. QoI issue on the Skia side? (I.e. something that the scaling tricks of
the old version may have done slightly better.)

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:168:
ASSERT(!child->needsLayout());
I think the below asserts this too.

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:169:
SVGRenderingContext::renderSubtree(&recordingContext, child, contentTransform);
I began to wonder if this call really did any good anymore - the only thing I
could come up with was text. IIRC though, it's "too late" for that when we're
here. I'd need to verify that though...

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.h:37: class GraphicsContext;
Nit: Not used in the header AFAICS

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.h:63: // FIXME: we can almost
do away with this per-object map, but not quite: the tile size can be
I think viewBox might might throw a larger spanner in works, and that the aspect
ratio might be a suitable "cache key". For laterz though.

https://codereview.chromium.org/453653003/diff/100001/Source/platform/graphic...
File Source/platform/graphics/DisplayListPattern.h (right):

https://codereview.chromium.org/453653003/diff/100001/Source/platform/graphic...
Source/platform/graphics/DisplayListPattern.h:24: virtual PassRefPtr<SkShader>
createShader() override;
No strong reason for having this public I think (it'll only ever be called
through the super-class?)

[f(malita), 2014-10-21 22:16:33]

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:831: crbug.com/401814
svg/W3C-SVG-1.1/pservers-grad-06-b.svg [ NeedsRebaseline ]
On 2014/10/21 20:12:35, fs wrote:
> I noticed that this (and other pattern with skew transforms) looked a bit
> smeared. QoI issue on the Skia side? (I.e. something that the scaling tricks
of
> the old version may have done slightly better.)

It's hard to tell (non-scaling-stroke may be the most obvious -
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...),
but I suspect now we're filtering bitmaps whereas before we were not. I haven't
really looked into why this is different because I think some of the new images
look better than then old rough-edged versions. If we you think it's important I
can try to dig deeper and maybe replicate the old behavior.

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:168:
ASSERT(!child->needsLayout());
On 2014/10/21 20:12:35, fs wrote:
> I think the below asserts this too.

Done.

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:169:
SVGRenderingContext::renderSubtree(&recordingContext, child, contentTransform);
On 2014/10/21 20:12:35, fs wrote:
> I began to wonder if this call really did any good anymore - the only thing I
> could come up with was text. IIRC though, it's "too late" for that when we're
> here. I'd need to verify that though...

I would very much like to get rid of renderSubtree() & his little
s_currentContentTransformation friend :) Like you said, I think text depends on
it - but it feels we're getting closer (IIRC it had a few more clients not long
ago).

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.h:37: class GraphicsContext;
On 2014/10/21 20:12:35, fs wrote:
> Nit: Not used in the header AFAICS

Done.

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.h:63: // FIXME: we can almost
do away with this per-object map, but not quite: the tile size can be
On 2014/10/21 20:12:35, fs wrote:
> I think viewBox might might throw a larger spanner in works, and that the
aspect
> ratio might be a suitable "cache key". For laterz though.

Acknowledged.

https://codereview.chromium.org/453653003/diff/100001/Source/platform/graphic...
File Source/platform/graphics/DisplayListPattern.h (right):

https://codereview.chromium.org/453653003/diff/100001/Source/platform/graphic...
Source/platform/graphics/DisplayListPattern.h:24: virtual PassRefPtr<SkShader>
createShader() override;
On 2014/10/21 20:12:35, fs wrote:
> No strong reason for having this public I think (it'll only ever be called
> through the super-class?)

Oh yeah - done. Also updated BitmapPatternBase, BitmapPattern &
StaticBitmapPattern.

[fs, 2014-10-22 09:21:32]

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/453653003/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:831: crbug.com/401814
svg/W3C-SVG-1.1/pservers-grad-06-b.svg [ NeedsRebaseline ]
On 2014/10/21 22:16:32, Florin Malita wrote:
> On 2014/10/21 20:12:35, fs wrote:
> > I noticed that this (and other pattern with skew transforms) looked a bit
> > smeared. QoI issue on the Skia side? (I.e. something that the scaling tricks
> of
> > the old version may have done slightly better.)
> 
> It's hard to tell (non-scaling-stroke may be the most obvious -
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...),
> but I suspect now we're filtering bitmaps whereas before we were not. I
haven't
> really looked into why this is different because I think some of the new
images
> look better than then old rough-edged versions. If we you think it's important
I
> can try to dig deeper and maybe replicate the old behavior.

Just to clear, I'm fine w/ having it like this and potentially iterate on it (I
remember going back and forth a lot on the rendering on this particular TC in
the past...). The non-scaling-stroke one definitely looks better IMHO.

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:169:
SVGRenderingContext::renderSubtree(&recordingContext, child, contentTransform);
On 2014/10/21 22:16:32, Florin Malita wrote:
> On 2014/10/21 20:12:35, fs wrote:
> > I began to wonder if this call really did any good anymore - the only thing
I
> > could come up with was text. IIRC though, it's "too late" for that when
we're
> > here. I'd need to verify that though...
> 
> I would very much like to get rid of renderSubtree() & his little
> s_currentContentTransformation friend :) Like you said, I think text depends
on
> it - but it feels we're getting closer (IIRC it had a few more clients not
long
> ago).

I think what I was getting at though is that for text this transform is only
consulted during layout/style update - with the exception of text decorations
(thickness/offset), which are resolved during paint. So right now there's a lot
stuff happening to get that case (text decorations) "right" [1]. Anyway...

[1] I'm not even convinced it's entirely right either.

[Stephen White, 2014-10-22 15:11:26]

LGTM. Great stuff!

https://codereview.chromium.org/453653003/diff/120001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/absolute-root-position-masking.xhtml (right):

https://codereview.chromium.org/453653003/diff/120001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/absolute-root-position-masking.xhtml:7: <svg:pattern
id="pattern" width="600" height="600" patternUnits="userSpaceOnUse">
What's the reason for the change here? Should we (or do we) also have a test for
patternUnits="objectBoundingBox"?

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:65: OwnPtr<PatternData>&
patternData = m_patternMap.add(&object, nullptr).storedValue->value;
Nit: This seems a bit of a convoluted way to use a map.

Couldn't we use HashMap::find() to get an iterator, and if found, do an
early-return? Then you would only need to add() in the not-found case.

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
File Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:70: m_pattern = createShader();
Tangentially, it looks like the only reason we need to lazy-create the SkShader
is to support a mutable patternSpaceTransform. If we could change the call sites
to not require that (and create a new Pattern when the transform changes), we
could make Pattern a thin wrapper around SkShader, and we wouldn't need the
Pattern subclasses at all (createDisplayListPattern()/createBitmapPattern()
would construct the appropriate SkShader, and pass it to the Pattern). That
would move us closer to the Skia model, too.

[fs, 2014-10-22 15:29:03]

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
File Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:70: m_pattern = createShader();
On 2014/10/22 15:11:26, Stephen White wrote:
> Tangentially, it looks like the only reason we need to lazy-create the
SkShader
> is to support a mutable patternSpaceTransform. If we could change the call
sites
> to not require that (and create a new Pattern when the transform changes), we
> could make Pattern a thin wrapper around SkShader, and we wouldn't need the
> Pattern subclasses at all (createDisplayListPattern()/createBitmapPattern()
> would construct the appropriate SkShader, and pass it to the Pattern). That
> would move us closer to the Skia model, too.

I think this should feasible at least for SVG (I don't really expect the
transform to be used elsewhere though...). It's mostly a matter of cargoing
around that (fairly large - 48 bytes) AffineTransform. Assuming you meant
something like:

class WrappedShader {
   AffineTransform shaderTransform;
   RefPtr<SkShader> shader;
};

I'll add it to my pile if no one gets to it first.

[fs, 2014-10-22 15:45:53]

On 2014/10/22 15:29:03, fs wrote:
> ... Assuming you meant
> something like:
> 
> class WrappedShader {
>    AffineTransform shaderTransform;
>    RefPtr<SkShader> shader;
> };

Uhm, scratch this part, that's obviously not what you meant.

[Stephen White, 2014-10-22 15:48:22]

On 2014/10/22 15:45:53, fs wrote:
> On 2014/10/22 15:29:03, fs wrote:
> > ... Assuming you meant
> > something like:
> > 
> > class WrappedShader {
> >    AffineTransform shaderTransform;
> >    RefPtr<SkShader> shader;
> > };
> 
> Uhm, scratch this part, that's obviously not what you meant.

Yeah, what I meant was that we would still use Pattern, but construct a new one
when the 
transform changes.

[f(malita), 2014-10-22 16:23:47]

https://codereview.chromium.org/453653003/diff/120001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/absolute-root-position-masking.xhtml (right):

https://codereview.chromium.org/453653003/diff/120001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/absolute-root-position-masking.xhtml:7: <svg:pattern
id="pattern" width="600" height="600" patternUnits="userSpaceOnUse">
On 2014/10/22 15:11:26, Stephen White wrote:
> What's the reason for the change here? Should we (or do we) also have a test
for
> patternUnits="objectBoundingBox"?

We have plenty of coverage for OBB, this (and the other test change) is just an
unrelated test fix: given the actual values, it is clear the intent is to use
userSpaceOnUse (600 in OBB units is pretty damn huge :).

The reason I've included it is that (at least before we landed tile clamping in
Skia) this would yield slightly different results due to different tile scaling.
Since we get to touch the baseline anyway, I figured we might as well fix the
test too.

With tile clamping in place though, I don't think there is a difference anymore.
Let me try to take these out (and fix the tests in a follow-up CL).

Done.

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:65: OwnPtr<PatternData>&
patternData = m_patternMap.add(&object, nullptr).storedValue->value;
On 2014/10/22 15:11:26, Stephen White wrote:
> Nit: This seems a bit of a convoluted way to use a map.
> 
> Couldn't we use HashMap::find() to get an iterator, and if found, do an
> early-return? Then you would only need to add() in the not-found case.

I agree it's convoluted, but it avoids a double hash lookup in the not-found
case. The idiom is quite common (we use it in RenderSVGResourceGradient and a
bunch of other places if you grep around for "add.*, nullptr"), but maybe I can
make it more clear with an AddResult like the other users.

Done.

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
File Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:70: m_pattern = createShader();
On 2014/10/22 15:11:26, Stephen White wrote:
> Tangentially, it looks like the only reason we need to lazy-create the
SkShader
> is to support a mutable patternSpaceTransform. If we could change the call
sites
> to not require that (and create a new Pattern when the transform changes), we
> could make Pattern a thin wrapper around SkShader, and we wouldn't need the
> Pattern subclasses at all (createDisplayListPattern()/createBitmapPattern()
> would construct the appropriate SkShader, and pass it to the Pattern). That
> would move us closer to the Skia model, too.

Acknowledged.

[Stephen White, 2014-10-22 17:50:48]

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourcePattern.cpp (right):

https://codereview.chromium.org/453653003/diff/120001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:65: OwnPtr<PatternData>&
patternData = m_patternMap.add(&object, nullptr).storedValue->value;
On 2014/10/22 16:23:47, Florin Malita wrote:
> On 2014/10/22 15:11:26, Stephen White wrote:
> > Nit: This seems a bit of a convoluted way to use a map.
> > 
> > Couldn't we use HashMap::find() to get an iterator, and if found, do an
> > early-return? Then you would only need to add() in the not-found case.
> 
> I agree it's convoluted, but it avoids a double hash lookup in the not-found
> case. The idiom is quite common (we use it in RenderSVGResourceGradient and a
> bunch of other places if you grep around for "add.*, nullptr"), but maybe I
can
> make it more clear with an AddResult like the other users.
> 
> Done.

Acknowledged.

[f(malita), 2014-10-23 12:53:11]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-10-23 12:53:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/453653003/140001

[commit-bot: I haz the power, 2014-10-23 12:54:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 12:54:05]

Failed to apply patch for
Source/core/rendering/svg/RenderSVGResourcePattern.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/svg/RenderSVGResourcePattern.cpp
  Hunk #5 FAILED at 146.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/rendering/svg/RenderSVGResourcePattern.cpp.rej

Patch:       Source/core/rendering/svg/RenderSVGResourcePattern.cpp
Index: Source/core/rendering/svg/RenderSVGResourcePattern.cpp
diff --git a/Source/core/rendering/svg/RenderSVGResourcePattern.cpp
b/Source/core/rendering/svg/RenderSVGResourcePattern.cpp
index
a15d4f3813c225a06ade3659b7b2e4fbc96cb201..bfaac4f1f5693fe903049341b4f97ecb863d2eff
100644
--- a/Source/core/rendering/svg/RenderSVGResourcePattern.cpp
+++ b/Source/core/rendering/svg/RenderSVGResourcePattern.cpp
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2006 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
+ * Copyright 2014 The Chromium Authors. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -24,10 +25,19 @@
 #include "core/dom/ElementTraversal.h"
 #include "core/rendering/svg/SVGRenderingContext.h"
 #include "core/svg/SVGFitToViewBox.h"
+#include "core/svg/SVGPatternElement.h"
+#include "platform/graphics/DisplayList.h"
 #include "platform/graphics/GraphicsContext.h"
 
 namespace blink {
 
+struct PatternData {
+    WTF_MAKE_FAST_ALLOCATED;
+public:
+    RefPtr<Pattern> pattern;
+    AffineTransform transform;
+};
+
 const RenderSVGResourceType RenderSVGResourcePattern::s_resourceType =
PatternResourceType;
 
 RenderSVGResourcePattern::RenderSVGResourcePattern(SVGPatternElement* node)
@@ -50,63 +60,59 @@ void
RenderSVGResourcePattern::removeClientFromCache(RenderObject* client, bool
     markClientForInvalidation(client, markForInvalidation ? PaintInvalidation :
ParentOnlyInvalidation);
 }
 
-PatternData* RenderSVGResourcePattern::buildPattern(const RenderObject& object,
const SVGPatternElement* patternElement)
+PatternData* RenderSVGResourcePattern::patternForRenderer(const RenderObject&
object)
 {
-    PatternData* currentData = m_patternMap.get(&object);
-    if (currentData && currentData->pattern)
-        return currentData;
+    auto addResult = m_patternMap.add(&object, nullptr);
+    OwnPtr<PatternData>& patternData = addResult.storedValue->value;
+
+    if (addResult.isNewEntry)
+        patternData = buildPatternData(object);
+
+    ASSERT(!m_shouldCollectPatternAttributes);
+    return patternData.get();
+}
 
+PassOwnPtr<PatternData> RenderSVGResourcePattern::buildPatternData(const
RenderObject& object)
+{
     // If we couldn't determine the pattern content element root, stop here.
     if (!m_attributes.patternContentElement())
-        return 0;
+        return nullptr;
 
     // An empty viewBox disables rendering.
     if (m_attributes.hasViewBox() && m_attributes.viewBox().isEmpty())
-        return 0;
-
-    // Compute all necessary transformations to build the tile image & the
pattern.
-    FloatRect tileBoundaries;
-    AffineTransform tileImageTransform;
-    if (!buildTileImageTransform(object, m_attributes, patternElement,
tileBoundaries, tileImageTransform))
-        return 0;
-
-    AffineTransform absoluteTransformIgnoringRotation;
-    SVGRenderingContext::calculateDeviceSpaceTransformation(&object,
absoluteTransformIgnoringRotation);
-
-    // Ignore 2D rotation, as it doesn't affect the size of the tile.
-    SVGRenderingContext::clear2DRotation(absoluteTransformIgnoringRotation);
-    FloatRect absoluteTileBoundaries =
absoluteTransformIgnoringRotation.mapRect(tileBoundaries);
-
-    // Scale the tile size to match the scale level of the patternTransform.
-   
absoluteTileBoundaries.scale(static_cast<float>(m_attributes.patternTransform().xScale()),
-        static_cast<float>(m_attributes.patternTransform().yScale()));
+        return nullptr;
 
-    // Build tile image.
-    OwnPtr<ImageBuffer> tileImage = createTileImage(m_attributes,
tileBoundaries, absoluteTileBoundaries, tileImageTransform);
-    if (!tileImage)
-        return 0;
+    ASSERT(element());
+    // Compute tile metrics.
+    FloatRect clientBoundingBox = object.objectBoundingBox();
+    FloatRect tileBounds = SVGLengthContext::resolveRectangle(element(),
+        m_attributes.patternUnits(), clientBoundingBox,
+        m_attributes.x(), m_attributes.y(), m_attributes.width(),
m_attributes.height());
+    if (tileBounds.isEmpty())
+        return nullptr;
 
-    RefPtr<Image> copiedImage = tileImage->copyImage(CopyBackingStore);
-    if (!copiedImage)
-        return 0;
+    AffineTransform tileTransform;
+    if (m_attributes.hasViewBox()) {
+        if (m_attributes.viewBox().isEmpty())
+            return nullptr;
+        tileTransform =
SVGFitToViewBox::viewBoxToViewTransform(m_attributes.viewBox(),
+            m_attributes.preserveAspectRatio(), tileBounds.width(),
tileBounds.height());
+    } else {
+        // A viewbox overrides patternContentUnits, per spec.
+        if (m_attributes.patternContentUnits() ==
SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX)
+            tileTransform.scale(clientBoundingBox.width(),
clientBoundingBox.height());
+    }
 
-    // Build pattern.
     OwnPtr<PatternData> patternData = adoptPtr(new PatternData);
-    patternData->pattern = Pattern::createBitmapPattern(copiedImage);
+    patternData->pattern =
Pattern::createDisplayListPattern(asDisplayList(tileBounds, tileTransform));
 
     // Compute pattern space transformation.
-    const IntSize tileImageSize = tileImage->size();
-    patternData->transform.translate(tileBoundaries.x(), tileBoundaries.y());
-    patternData->transform.scale(tileBoundaries.width() /
tileImageSize.width(), tileBoundaries.height() / tileImageSize.height());
-
+    patternData->transform.translate(tileBounds.x(), tileBounds.y());
     AffineTransform patternTransform = m_attributes.patternTransform();
     if (!patternTransform.isIdentity())
         patternData->transform = patternTransform * patternData->transform;
 
-    // Various calls above may trigger invalidations in some fringe cases
(ImageBuffer allocation
-    // failures in the SVG image cache for example). To avoid having our
PatternData deleted by
-    // removeAllClientsFromCache(), we only make it visible in the cache at the
very end.
-    return m_patternMap.set(&object,
patternData.release()).storedValue->value.get();
+    return patternData.release();
 }
 
 SVGPaintServer RenderSVGResourcePattern::preparePaintServer(const RenderObject&
object)
@@ -131,7 +137,7 @@ SVGPaintServer
RenderSVGResourcePattern::preparePaintServer(const RenderObject&
     if (m_attributes.patternUnits() ==
SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX && objectBoundingBox.isEmpty())
         return SVGPaintServer::invalid();
 
-    PatternData* patternData = buildPattern(object, patternElement);
+    PatternData* patternData = patternForRenderer(object);
     if (!patternData)
         return SVGPaintServer::invalid();
 
@@ -140,80 +146,30 @@ SVGPaintServer
RenderSVGResourcePattern::preparePaintServer(const RenderObject&
     return SVGPaintServer(patternData->pattern);
 }
 
-static inline FloatRect calculatePatternBoundaries(const PatternAttributes&
attributes,
-                                                   const FloatRect&
objectBoundingBox,
-                                                   const SVGPatternElement*
patternElement)
-{
-    ASSERT(patternElement);
-    return SVGLengthContext::resolveRectangle(patternElement,
attributes.patternUnits(), objectBoundingBox, attributes.x(), attributes.y(),
attributes.width(), attributes.height());
-}
-
-bool RenderSVGResourcePattern::buildTileImageTransform(const RenderObject&
renderer,
-                                                       const PatternAttributes&
attributes,
-                                                       const SVGPatternElement*
patternElement,
-                                                       FloatRect&
patternBoundaries,
-                                                       AffineTransform&
tileImageTransform) const
+PassRefPtr<DisplayList> RenderSVGResourcePattern::asDisplayList(const
FloatRect& tileBounds,
+    const AffineTransform& tileTransform) const
 {
-    ASSERT(patternElement);
+    ASSERT(!m_shouldCollectPatternAttributes);
 
-    FloatRect objectBoundingBox = renderer.objectBoundingBox();
-    patternBoundaries = calculatePatternBoundaries(attributes,
objectBoundingBox, patternElement);
-    if (patternBoundaries.width() <= 0 || patternBoundaries.height() <= 0)
-        return false;
+    AffineTransform contentTransform;
+    if (m_attributes.patternContentUnits() ==
SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX)
+        contentTransform = tileTransform;
 
-    AffineTransform viewBoxCTM =
SVGFitToViewBox::viewBoxToViewTransform(attributes.viewBox(),
attributes.preserveAspectRatio(), patternBoundaries.width(),
patternBoundaries.height());
+    // Draw the content into a DisplayList.
+    GraphicsContext recordingContext(nullptr);
+    recordingContext.beginRecording(FloatRect(FloatPoint(),
tileBounds.size()));
+    recordingContext.concatCTM(tileTransform);
 
-    // Apply viewBox/objectBoundingBox transformations.
-    if (!viewBoxCTM.isIdentity())
-        tileImageTransform = viewBoxCTM;
-    else if (attributes.patternContentUnits() ==
SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX)
-        tileImageTransform.scale(objectBoundingBox.width(),
objectBoundingBox.height());
+    ASSERT(m_attributes.patternContentElement());
+    RenderSVGResourceContainer* patternRenderer =
+       
toRenderSVGResourceContainer(m_attributes.patternContentElement()->renderer());
+    ASSERT(patternRenderer);
+    ASSERT(!patternRenderer->needsLayout());
 
-    return true;
-}
-
-PassOwnPtr<ImageBuffer> RenderSVG…
(message too large)

[f(malita), 2014-10-23 13:22:30]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-10-23 13:22:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/453653003/160001

[commit-bot: I haz the power, 2014-10-23 14:31:29]

Committed patchset #9 (id:160001) as 184271