Implement an SkPicture image filter source. This is required for the external-SVG reference feature…

Implement an SkPicture image filter source. This is required for the external-SVG reference feature of feImage. It simply plays back an SkPicture to a given destination rect.

R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12661

[Stephen White, 2013-12-12 17:54:09]

reed@: PTAL. Thanks!

[reed1, 2013-12-12 19:31:53]

https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
File include/effects/SkPictureSource.h (right):

https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
include/effects/SkPictureSource.h:14: class SK_API SkPictureSource : public
SkImageFilter {
Name looks a little too general... how about SkPictureSourceImageFilter?

https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
include/effects/SkPictureSource.h:16: explicit SkPictureSource(const SkPicture&
picture);
dox:
- do you 'ref' the picture argument?

I know we take a & in drawPicture, but I think that is a bug. Everywhere else we
pass around refcnt objects, we pass pointers. I think we should do that here as
well.

https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
include/effects/SkPictureSource.h:17: SkPictureSource(const SkPicture& picture,
const SkRect& rect);
dox:
- is rect a crop rect, or is it resizing the picture to fit inside?

[Stephen White, 2013-12-12 19:39:24]

On 2013/12/12 19:31:53, reed1 wrote:
>
https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
> File include/effects/SkPictureSource.h (right):
> 
>
https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
> include/effects/SkPictureSource.h:14: class SK_API SkPictureSource : public
> SkImageFilter {
> Name looks a little too general... how about SkPictureSourceImageFilter?
> 
>
https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
> include/effects/SkPictureSource.h:16: explicit SkPictureSource(const
SkPicture&
> picture);
> dox:
> - do you 'ref' the picture argument?

Actually, it's copy-constructed from a const-ref, so probably a deep copy. :(  I
thought I had to do this due to Blink silliness, but it turns out I don't. Will
fix.

> I know we take a & in drawPicture, but I think that is a bug. Everywhere else
we
> pass around refcnt objects, we pass pointers. I think we should do that here
as
> well.

Will do.

>
https://codereview.chromium.org/114263002/diff/1/include/effects/SkPictureSou...
> include/effects/SkPictureSource.h:17: SkPictureSource(const SkPicture&
picture,
> const SkRect& rect);
> dox:
> - is rect a crop rect, or is it resizing the picture to fit inside?

It's resizing the picture (unlike the crop rect, it can grow).

[Stephen White, 2013-12-12 21:11:03]

New patch up.

[reed1, 2013-12-12 21:28:39]

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:16: explicit
SkPictureImageFilter(SkPicture* picture);
Sorry I wasn't clearer. I was asking for some dox in the header itself

e.g.

// Calls ref on the picture argument

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:17: SkPictureImageFilter(SkPicture*
picture, const SkRect& rect);
e.g.

// Calls ref on the picture argument.
// The picture is scaled to fit inside the rect parameter

That rect parameter is like a convenience form of just passing a matrix to go
with the picture, right?

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:18: ~SkPictureImageFilter();
style nit: virtual ~SkPictureImageFilter();

https://codereview.chromium.org/114263002/diff/40001/src/effects/SkPictureIma...
File src/effects/SkPictureImageFilter.cpp (right):

https://codereview.chromium.org/114263002/diff/40001/src/effects/SkPictureIma...
src/effects/SkPictureImageFilter.cpp:35: // FIXME: unflatten picture here.
Can this be used in chrome w/o the flattening implemented? How will this filter
behave inside a picture?

[Stephen White, 2013-12-12 21:46:56]

Thanks for your review.

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:16: explicit
SkPictureImageFilter(SkPicture* picture);
On 2013/12/12 21:28:39, reed1 wrote:
> Sorry I wasn't clearer. I was asking for some dox in the header itself
> 
> e.g.
> 
> // Calls ref on the picture argument

Done.

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:17: SkPictureImageFilter(SkPicture*
picture, const SkRect& rect);
On 2013/12/12 21:28:39, reed1 wrote:
> e.g.
> 
> // Calls ref on the picture argument.
> // The picture is scaled to fit inside the rect parameter
> 
> That rect parameter is like a convenience form of just passing a matrix to go
> with the picture, right?

No, it's essentially a "crop or expand" rect. It doesn't resize, just affects
which subset or superset of the picture is drawn. I've tried to explain that in
the comments; let me know if I could do better.

https://codereview.chromium.org/114263002/diff/40001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:18: ~SkPictureImageFilter();
On 2013/12/12 21:28:39, reed1 wrote:
> style nit: virtual ~SkPictureImageFilter();

Done.

https://codereview.chromium.org/114263002/diff/40001/src/effects/SkPictureIma...
File src/effects/SkPictureImageFilter.cpp (right):

https://codereview.chromium.org/114263002/diff/40001/src/effects/SkPictureIma...
src/effects/SkPictureImageFilter.cpp:35: // FIXME: unflatten picture here.
On 2013/12/12 21:28:39, reed1 wrote:
> Can this be used in chrome w/o the flattening implemented? How will this
filter
> behave inside a picture?

It works as long as there is no serialization required. So for now,  it'll also
work with my work-in-progress patch for moving the "raster" path of SVG/CSS
filters off the main thread. It'll also work for accelerated filters on Mac and
Android (which don't yet have ubercomp and thus don't require serialization).

[Stephen White, 2013-12-12 21:47:13]

New patch up.

[reed1, 2013-12-12 21:51:53]

lgtm

https://codereview.chromium.org/114263002/diff/60001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/114263002/diff/60001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:19: // Refs the passed-in picture. rect
can be used to crop or expand the destination rect when
nit for the future -- when we document public headers, its better to use
doxygen-compatible forms, like

/**
 * Refs the passed-in picture
 */

Not worth blocking this CL.

[Stephen White, 2013-12-12 21:56:45]

https://codereview.chromium.org/114263002/diff/60001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/114263002/diff/60001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:19: // Refs the passed-in picture. rect
can be used to crop or expand the destination rect when
On 2013/12/12 21:51:54, reed1 wrote:
> nit for the future -- when we document public headers, its better to use
> doxygen-compatible forms, like
> 
> /**
>  * Refs the passed-in picture
>  */
> 
> Not worth blocking this CL.

Done.

[Stephen White, 2013-12-12 22:00:39]

Committed patchset #5 manually as r12656 (presubmit successful).

[Stephen White, 2013-12-12 23:24:39]

Reverted due to test crashes; reopening.

[Stephen White, 2013-12-12 23:28:59]

Committed patchset #7 manually as r12661 (presubmit successful).