Implement an SkPaint-based image filter

Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1556553002

Committed: https://skia.googlesource.com/skia/+/77b6ba3b6e23b84a3a4f3a62812e4a9eb6de4c23

[ajuma, 2015-12-29 22:12:06]

Description was changed from

==========
WIP: SkPaintImageFilter

BUG=skia:
==========

to

==========
WIP: SkPaintImageFilter

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ajuma, 2015-12-30 21:00:39]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-30 21:00:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1556553002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1556553002/20001

[commit-bot: I haz the power, 2015-12-30 21:00:48]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-12-31 03:00 UTC

[ajuma, 2015-12-30 21:01:45]

The CQ bit was unchecked by ajuma@chromium.org

[reed1, 2015-12-31 13:34:14]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-12-31 13:34:15]

I know this is just WIP, but curious about the use-case, and how this compares
to subclasses like PictureImageFilter

[ajuma, 2015-12-31 19:20:22]

On 2015/12/31 13:34:15, reed1 wrote:
> I know this is just WIP, but curious about the use-case, and how this compares
> to subclasses like PictureImageFilter

The use case is representing the FillPaint and StrokePaint filter primitives
(http://www.w3.org/TR/SVG/filters.html#FillPaint) for SVG filters. This can also
completely replace uses of SkRectShaderImageFilter, so the total number of
SkImageFilter subclasses won't increase.

One problem with using SkPictureImageFilter for this is that
SkPictureImageFilter isn't allowed to be serialized from a renderer process to
the browser, so accelerated filters (which are applied in the browser) can't use
SkPictureImageFilters.

[ajuma, 2016-01-04 16:00:15]

Description was changed from

==========
WIP: SkPaintImageFilter

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter. This is needed for
representing the FillPaint and StrokePaint filter
primitives for SVG filters.

This is also intended to replace SkRectShaderImageFilter.

BUG=502877
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ajuma, 2016-01-04 16:00:15]

ajuma@chromium.org changed reviewers:
+ senorblanco@chromium.org

[ajuma, 2016-01-04 16:05:25]

This is now ready for review.

After this lands, the existing Blink-side consumer of SkRectShaderImageFilter
can be switched over to SkPaintImageFilter, and then SkRectShaderImageFilter can
be deleted.

[reed1, 2016-01-04 16:14:37]

reed@google.com changed reviewers:
+ robertphillips@google.com

[Stephen White, 2016-01-04 16:18:17]

On 2015/12/31 19:20:22, ajuma wrote:

> One problem with using SkPictureImageFilter for this is that
> SkPictureImageFilter isn't allowed to be serialized from a renderer process to
> the browser, so accelerated filters (which are applied in the browser) can't
use
> SkPictureImageFilters.

That does provoke an important question, though: is SkPaint safe to serialize 
cross-process? At a minimum, we should add some more cases to 
samplecode/SampleFilterFuzz.cpp to exercise this.

I'm not sure sure of the entire footprint of SkPaint, but we'll need to check
that 
things like SkTypeface, SKPathEffect, SkDrawLooper, SkRasterizer and
SkAnnotation are 
security-hardened. (I mention those mostly because I know less about them than
other 
things in SkPaint.)

[Stephen White, 2016-01-04 16:21:03]

senorblanco@chromium.org changed reviewers:
+ sugoi@chromium.org

[Stephen White, 2016-01-04 16:21:24]

+sugoi for fuzzing expertise

[sugoi1, 2016-01-04 16:31:15]

https://codereview.chromium.org/1556553002/diff/40001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/40001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:376: paint.setShader(shader);
You should really try to randomly use all the API functions of SkPaint that
changes its internal data (not just setShader()). It might be a lot, but you
won't have a good idea of how the deserialization reacts to these various states
without explicitly trying them out.

[ajuma, 2016-01-04 22:39:46]

https://codereview.chromium.org/1556553002/diff/40001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/40001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:376: paint.setShader(shader);
On 2016/01/04 16:31:15, sugoi1 wrote:
> You should really try to randomly use all the API functions of SkPaint that
> changes its internal data (not just setShader()). It might be a lot, but you
> won't have a good idea of how the deserialization reacts to these various
states
> without explicitly trying them out.

Added a test case below.

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:487: SkIntToScalar(kBitmapSize)));
Is it worthwhile also adding test cases that use some subset of these calls, or
will the fuzzer handle that automatically? Also, is the fuzzer continuously
being run by a bot (like ClusterFuzz) or do we need to do something to trigger
it?

[sugoi1, 2016-01-05 15:59:55]

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:487: SkIntToScalar(kBitmapSize)));
On 2016/01/04 22:39:46, ajuma wrote:
> Is it worthwhile also adding test cases that use some subset of these calls,
or
> will the fuzzer handle that automatically? Also, is the fuzzer continuously
> being run by a bot (like ClusterFuzz) or do we need to do something to trigger
> it?

You could choose to use each of these functions randomly, which would be nice
(as in 'if(random value) paint.function();'). You should also use make_scalar()
for scalars instead of hardcoded values. Same thing with bools, where you could
use '(R(2) == 1)' instead (you can create a make_bool() function if you find
that too tedious). Enums can also be randomized using the R() function and
casting to result to the appropriate enum. Try to randomize as many values as
possible.

[ajuma, 2016-01-06 20:53:39]

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/60001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:487: SkIntToScalar(kBitmapSize)));
On 2016/01/05 15:59:55, sugoi1 wrote:
> On 2016/01/04 22:39:46, ajuma wrote:
> > Is it worthwhile also adding test cases that use some subset of these calls,
> or
> > will the fuzzer handle that automatically? Also, is the fuzzer continuously
> > being run by a bot (like ClusterFuzz) or do we need to do something to
trigger
> > it?
> 
> You could choose to use each of these functions randomly, which would be nice
> (as in 'if(random value) paint.function();'). You should also use
make_scalar()
> for scalars instead of hardcoded values. Same thing with bools, where you
could
> use '(R(2) == 1)' instead (you can create a make_bool() function if you find
> that too tedious). Enums can also be randomized using the R() function and
> casting to result to the appropriate enum. Try to randomize as many values as
> possible.

Thanks for the tips! (And also thanks for all the info about fuzzing.) I've made
the test more randomized now.

[sugoi1, 2016-01-07 16:06:20]

Nice patch. Just one nit.

Have you tried it out locally to see if you catch anything?

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:123: return "Times New Roman";
Nit: Maybe add an option to get a garbage string.

[Stephen White, 2016-01-07 16:17:33]

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:524:
SkColorFilter::CreateLightingFilter(make_color(), make_color()));
Not new to this CL, but when it comes to SkColorFilters, it looks
like we're only testing the lighting and mode filters. We should be 
exercising all of the ones exposed to serialization, including
SkColorMatrixFilter, SkLumaColorFilter, SkTableColorFilter.

I'd make a helper that randomly creates one of those (or null).

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:527: SkAutoTUnref<SkPathEffect>
pathEffect(SkCornerPathEffect::Create(make_scalar()));
We should be exercising all the serializable path effects: SkArcToPathEffect,
SkComposePathEffect, SkDashPathEffect, SkDiscretePathEffect, SkPath1DPathEffect,
SkLine2DPathEffect, SkPath2DPathEffect, SkSumPathEffect.

Unlike the color filters above, this *is* new to this CL.

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:530: SkBlurMaskFilter::Create(make_blur_style(),
Also SkEmbossMaskFilter.

[ajuma, 2016-01-08 17:44:28]

With the additional coverage for path effects, I found two crashes when running
locally; these are fixed in the latest patch set.

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:123: return "Times New Roman";
On 2016/01/07 16:06:20, sugoi1 wrote:
> Nit: Maybe add an option to get a garbage string.

Done.

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:524:
SkColorFilter::CreateLightingFilter(make_color(), make_color()));
On 2016/01/07 16:17:33, Stephen White wrote:
> Not new to this CL, but when it comes to SkColorFilters, it looks
> like we're only testing the lighting and mode filters. We should be 
> exercising all of the ones exposed to serialization, including
> SkColorMatrixFilter, SkLumaColorFilter, SkTableColorFilter.
> 
> I'd make a helper that randomly creates one of those (or null).

Done.

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:527: SkAutoTUnref<SkPathEffect>
pathEffect(SkCornerPathEffect::Create(make_scalar()));
On 2016/01/07 16:17:33, Stephen White wrote:
> We should be exercising all the serializable path effects: SkArcToPathEffect,
> SkComposePathEffect, SkDashPathEffect, SkDiscretePathEffect,
SkPath1DPathEffect,
> SkLine2DPathEffect, SkPath2DPathEffect, SkSumPathEffect.
> 
> Unlike the color filters above, this *is* new to this CL.

Done.

https://codereview.chromium.org/1556553002/diff/80001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:530: SkBlurMaskFilter::Create(make_blur_style(),
On 2016/01/07 16:17:33, Stephen White wrote:
> Also SkEmbossMaskFilter.

Done.

[Stephen White, 2016-01-08 18:20:48]

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:387: default:
Nit: for clarity, explicitly set colorFilter = nullptr here, and don't
initialize it above?

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:397: if (R(2) == 1) {
How about cubicTo(), quadTo(), conicTo()?

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:475: case 2:
Nit: set maskFilter = nullptr here explicitly, and don't initialize it above?

[ajuma, 2016-01-08 19:09:08]

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:387: default:
On 2016/01/08 18:20:47, Stephen White wrote:
> Nit: for clarity, explicitly set colorFilter = nullptr here, and don't
> initialize it above?

Done.

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:397: if (R(2) == 1) {
On 2016/01/08 18:20:48, Stephen White wrote:
> How about cubicTo(), quadTo(), conicTo()?

Done.

https://codereview.chromium.org/1556553002/diff/120001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:475: case 2:
On 2016/01/08 18:20:48, Stephen White wrote:
> Nit: set maskFilter = nullptr here explicitly, and don't initialize it above?

Done.

https://codereview.chromium.org/1556553002/diff/140001/samplecode/SampleFilte...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1556553002/diff/140001/samplecode/SampleFilte...
samplecode/SampleFilterFuzz.cpp:536: paintForRasterizer = make_paint();
Made this paint random too.

[Stephen White, 2016-01-08 19:11:05]

LGTM

Will leave for sugoi@ (and you'll need reed@'s as well for the API changes).

Awesome work!

[sugoi1, 2016-01-08 19:16:46]

LGMT for the fuzzer changes. Nice work.

[sugoi1, 2016-01-08 19:17:05]

LGTM, oops :)

[reed1, 2016-01-08 19:55:35]

How does this (help) fix the referenced chrome bug? (I'm avail for a VC if the
answer is complicated)

[reed1, 2016-01-08 20:13:16]

BTW -- perhaps we should land all of the not-new-shader-specific changes (like
improvements to fuzzing) separately (sooner).

[reed1, 2016-01-08 20:58:49]

Lets create a skia bug that more clear states that this CL is about:

Need a way to control dithering when creating a shader-imagefilter

[ajuma, 2016-01-08 21:24:12]

Description was changed from

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter. This is needed for
representing the FillPaint and StrokePaint filter
primitives for SVG filters.

This is also intended to replace SkRectShaderImageFilter.

BUG=502877
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ajuma, 2016-01-08 21:24:32]

Description was changed from

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ajuma, 2016-01-08 21:25:03]

On 2016/01/08 20:58:49, reed1 wrote:
> Lets create a skia bug that more clear states that this CL is about:
> 
> Need a way to control dithering when creating a shader-imagefilter

Filed https://bugs.chromium.org/p/skia/issues/detail?id=4780

[reed1, 2016-01-08 21:46:18]

lgtm

[ajuma, 2016-01-08 22:41:11]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-01-08 22:41:23]

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

[commit-bot: I haz the power, 2016-01-08 22:58:44]

Description was changed from

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement an SkPaint-based image filter

This implements SkPaintImageFilter, and is intended to replace
SkRectShaderImageFilter. By allowing a paint and not just a
shader as input, this allows consumers to control dithering.

BUG=skia:4780
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/77b6ba3b6e23b84a3a4f3a62812e4a9eb6de4c23
==========

[commit-bot: I haz the power, 2016-01-08 22:58:45]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/77b6ba3b6e23b84a3a4f3a62812e4a9eb6de4c23

[reed1, 2016-01-12 11:10:19]

Update the skia bug when it is safe to remove SkRectShaderImageFilter... i.e.
when chrome no longer uses it.