Implement crop rect for SkImageFilter

This patch implements a crop rect for SkImageFilter. It has been implemented for SkColorFilterImageFilter and SkBlurImageFilter as examples.

In order to preserve the immutability of SkImageFilters, the crop rect is passed as a constructor parameter. If NULL (the default), the bounds of the input image are used, as before.

This also tightens up the boundary handling for SkImageBlurFilter on the GPU backend. Where we were previously using clamping semantics, we now respect decal semantics (so we don't oversaturate the edges). This brings the GPU and raster backends into closer alignment, but will require some new baselines for the GPU tests.

At a minimum, the following tests will need new baselines: imageblur, imagefiltersbase, imagefilterscropped, spritebitmap.

R=reed@google.com

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

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

[Stephen White, 2013-07-18 21:49:55]

PTAL. This is a work-in-progress, but I welcome feedback on the API.

[bsalomon, 2013-07-19 13:16:58]

On 2013/07/18 21:49:55, Stephen White wrote:
> PTAL. This is a work-in-progress, but I welcome feedback on the API.

Hey Stephen, my first bit of feedback would be that the ImageFilter header
should say something about what space the rect is in and how it is applied (e.g.
something about what filters that have spatial extent should do at the edge of
the crop rect.)

[Stephen White, 2013-07-19 20:54:02]

On 2013/07/19 13:16:58, bsalomon wrote:
> On 2013/07/18 21:49:55, Stephen White wrote:
> > PTAL. This is a work-in-progress, but I welcome feedback on the API.
> 
> Hey Stephen, my first bit of feedback would be that the ImageFilter header
> should say something about what space the rect is in and how it is applied
(e.g.
> something about what filters that have spatial extent should do at the edge of
> the crop rect.)

Done.

I've fixed a few issues and updated to Tot; this should be ready for full review
now.

[reed1, 2013-07-19 21:34:03]

https://codereview.chromium.org/19775006/diff/8001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/8001/include/core/SkImageFilter...
include/core/SkImageFilter.h:157: explicit SkImageFilter(SkImageFilter* input,
const SkIRect* cropRect = NULL);
is explicit correct w/ more than one parameter?

https://codereview.chromium.org/19775006/diff/8001/include/effects/SkBlurImag...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/8001/include/effects/SkBlurImag...
include/effects/SkBlurImageFilter.h:16: SkBlurImageFilter(SkScalar sigmaX,
SkScalar sigmaY, SkImageFilter* input = NULL, const SkIRect* cropRect = NULL);
line-wrapping

https://codereview.chromium.org/19775006/diff/8001/include/effects/SkColorFil...
File include/effects/SkColorFilterImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/8001/include/effects/SkColorFil...
include/effects/SkColorFilterImageFilter.h:17: static SkColorFilterImageFilter*
Create(SkColorFilter* cf, SkImageFilter* input = NULL, const SkIRect* cropRect =
NULL);
line-wrap

https://codereview.chromium.org/19775006/diff/8001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/19775006/diff/8001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:21: SkImageFilter::SkImageFilter(int inputCount,
SkImageFilter** inputs, const SkIRect* cropRect)
line-wrap

https://codereview.chromium.org/19775006/diff/8001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:142: void SkImageFilter::applyCropRect(SkIRect* rect)
const {
How is this documented to behave if rect and fCropRect do not intersect? Should
this method return a bool, as the rect-method does?

https://codereview.chromium.org/19775006/diff/8001/src/effects/SkBlurImageFil...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/19775006/diff/8001/src/effects/SkBlurImageFil...
src/effects/SkBlurImageFilter.cpp:24:
SkBlurImageFilter::SkBlurImageFilter(SkScalar sigmaX, SkScalar sigmaY,
SkImageFilter* input, const SkIRect* cropRect)
line-wrap

https://codereview.chromium.org/19775006/diff/8001/src/effects/SkColorFilterI...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/19775006/diff/8001/src/effects/SkColorFilterI...
src/effects/SkColorFilterImageFilter.cpp:78:
SkColorFilterImageFilter::SkColorFilterImageFilter(SkColorFilter* cf,
SkImageFilter* input, const SkIRect* cropRect) : INHERITED(input, cropRect),
fColorFilter(cf) {
line-wrap

https://codereview.chromium.org/19775006/diff/8001/src/effects/SkColorFilterI...
src/effects/SkColorFilterImageFilter.cpp:108: this->applyCropRect(&bounds);
what will bounds be if it and the croprect didn't overlap?

[Stephen White, 2013-07-19 22:11:50]

https://codereview.chromium.org/19775006/diff/8001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/8001/include/core/SkImageFilter...
include/core/SkImageFilter.h:157: explicit SkImageFilter(SkImageFilter* input,
const SkIRect* cropRect = NULL);
On 2013/07/19 21:34:04, reed1 wrote:
> is explicit correct w/ more than one parameter?

When you have default arguments, I think it is, since otherwise you could still
get implicit conversions when calling it with one arg.

https://codereview.chromium.org/19775006/diff/8001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/19775006/diff/8001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:142: void SkImageFilter::applyCropRect(SkIRect* rect)
const {
On 2013/07/19 21:34:04, reed1 wrote:
> How is this documented to behave if rect and fCropRect do not intersect?
Should
> this method return a bool, as the rect-method does?

That's a good point!

The SVG spec doesn't call out empty intersections explicitly, but just says "All
intermediate offscreens are defined to not exceed the intersection of ‘x’, ‘y’,
‘width’ and ‘height’ with the filter region." So it seems we have to return a
0x0 bitmap, at least semantically.

It looks like the GPU backend doesn't like 0x0 textures, so I think we should
leave the result bitmap untouched and early-out (maybe even before processing
inputs). Does that make sense?

[Stephen White, 2013-07-22 17:40:36]

OK, I've made applyCropRect() return a bool, and have the filtering code abort
on zero intersection. This required making the GPU path abort on image filtering
failure (rather than drawing the unfiltered source), which it appears the raster
path was already doing.

I've also transposed and simplified the GM, and added a zero-intersection case.
This should leave room for additional test cases once the other filters handle
cropping.

[reed1, 2013-07-22 19:11:23]

lgtm w/ request for dox

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilte...
include/core/SkImageFilter.h:157: explicit SkImageFilter(SkImageFilter* input,
const SkIRect* cropRect = NULL);
Why do we have explict on 1 of these, and not all (or none)?

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilte...
include/core/SkImageFilter.h:174: bool applyCropRect(SkIRect* rect) const;
// returns intersection of rect w/ filter's croprect, or false if they don't
overlap?

[Stephen White, 2013-07-22 19:46:27]

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilte...
include/core/SkImageFilter.h:157: explicit SkImageFilter(SkImageFilter* input,
const SkIRect* cropRect = NULL);
On 2013/07/22 19:11:23, reed1 wrote:
> Why do we have explict on 1 of these, and not all (or none)?

There's only one version which can be called with a single argument, and thus
could be mistakenly used as an implicit type conversion. The rest have two or
more required arguments.

https://codereview.chromium.org/19775006/diff/20001/include/core/SkImageFilte...
include/core/SkImageFilter.h:174: bool applyCropRect(SkIRect* rect) const;
On 2013/07/22 19:11:23, reed1 wrote:
> // returns intersection of rect w/ filter's croprect, or false if they don't
> overlap?

Done.

[Stephen White, 2013-07-22 20:03:30]

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

[robertphillips, 2013-07-24 13:18:55]

This was reverted in r10304 due to some Chromium-side unit test failures
(https://codereview.chromium.org/19664010/)

[Stephen White, 2013-07-24 20:52:34]

reed@, bsalomon@: PTAL. This version fixes the webkit_unit_tests failures from
the last landing (fixed SkColorFilterImageFilter::asColorFilter). It should also
take care of http://skbug.com/1440, by only invoking the new cropping/decal code
for image filters. (Blur mask filters don't need it, since we always have a
boundary of transparent black around the source path we can clamp to.)

Also added a new unit test to check the color filter optimizations, and that the
crop rect precludes asColorFilter().

(The patch set previous landed was #7, if you want to diff against that.)

[reed1, 2013-07-24 20:58:50]

lgtm

[robertphillips, 2013-07-24 20:59:22]

Will this still require the rebaseline for the other unit test?

[robertphillips, 2013-07-24 21:00:29]

... and, if so, do you have the new image lying around somewhere?

[Stephen White, 2013-07-24 21:02:15]

> Will this still require the rebaseline for the other unit test?

Yes, I think it will.

On 2013/07/24 21:00:29, robertphillips wrote:
> ... and, if so, do you have the new image lying around somewhere?

I'll try to generate it before landing here.

[Stephen White, 2013-07-24 21:36:41]

On 2013/07/24 21:02:15, Stephen White wrote:
> > Will this still require the rebaseline for the other unit test?
> 
> Yes, I think it will.
> 
> On 2013/07/24 21:00:29, robertphillips wrote:
> > ... and, if so, do you have the new image lying around somewhere?
> 
> I'll try to generate it before landing here.

Generated and uploaded here: https://codereview.chromium.org/20148002/

If you plan to dcommit the roll, you should be able to just grab that PNG and
include it with the roll. Otherwise, we can land that change (which temporarily
fuzzifies the test). Let me know which you prefer.

[Stephen White, 2013-07-24 22:19:33]

Committed patchset #11 manually as r10338 (presubmit successful).