Implement crop rect support for SkRectShaderImageFilter

Implement crop rect support for SkRectShaderImageFilter: remove fRect and use the cropRect from SkImageFilter in its place.

R=reed@google.com

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

[reed1, 2013-10-07 18:14:23]

https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
File include/effects/SkRectShaderImageFilter.h (right):

https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
include/effects/SkRectShaderImageFilter.h:24: static SkRectShaderImageFilter*
Create(SkShader* s, const SkIRect& rect);
General question:

What does the rect really mean? Is it

     "the portion of the src to which the filter is applied"?

For some drawings, the src geometry can be very small, but we end up scaling the
results to be very large (e.g. a global zoom on the outer canvas). For this
reason nearly all of our APIs take floats when talking about coordinates, so the
caller can be very precise, regardless of scale.

[Stephen White, 2013-10-07 19:04:28]

https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
File include/effects/SkRectShaderImageFilter.h (right):

https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
include/effects/SkRectShaderImageFilter.h:24: static SkRectShaderImageFilter*
Create(SkShader* s, const SkIRect& rect);
On 2013/10/07 18:14:24, reed1 wrote:
> General question:
> 
> What does the rect really mean? Is it
> 
>      "the portion of the src to which the filter is applied"?

Exactly. It also determines the size of the result bitmap, although I suppose
this is an implementation detail.

> For some drawings, the src geometry can be very small, but we end up scaling
the
> results to be very large (e.g. a global zoom on the outer canvas). For this
> reason nearly all of our APIs take floats when talking about coordinates, so
the
> caller can be very precise, regardless of scale.

In general, the crop rect represents the sub-region of the primitive to filter.
I made it an SkIRect simply to match the SkImageFilter::filterBounds() API which
you had written. It originally represented device-space pixels, so using integer
pixels made since. But it's now scaled by the CTM, so it may end up being
partial pixels anyway (not currently done by Chrome, but may in the future). So
we should probably change it to an SkRect at some point.

[reed1, 2013-10-07 19:33:09]

On 2013/10/07 19:04:28, Stephen White wrote:
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> File include/effects/SkRectShaderImageFilter.h (right):
> 
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> include/effects/SkRectShaderImageFilter.h:24: static SkRectShaderImageFilter*
> Create(SkShader* s, const SkIRect& rect);
> On 2013/10/07 18:14:24, reed1 wrote:
> > General question:
> > 
> > What does the rect really mean? Is it
> > 
> >      "the portion of the src to which the filter is applied"?
> 
> Exactly. It also determines the size of the result bitmap, although I suppose
> this is an implementation detail.
> 
> > For some drawings, the src geometry can be very small, but we end up scaling
> the
> > results to be very large (e.g. a global zoom on the outer canvas). For this
> > reason nearly all of our APIs take floats when talking about coordinates, so
> the
> > caller can be very precise, regardless of scale.
> 
> In general, the crop rect represents the sub-region of the primitive to
filter.
> I made it an SkIRect simply to match the SkImageFilter::filterBounds() API
which
> you had written. It originally represented device-space pixels, so using
integer
> pixels made since. But it's now scaled by the CTM, so it may end up being
> partial pixels anyway (not currently done by Chrome, but may in the future).
So
> we should probably change it to an SkRect at some point.

    bool filterBounds(const SkIRect& src, const SkMatrix& ctm, SkIRect* dst);

Ah, you are saying the src-rect here should really be fraction, even though its
in device-space? That's fine w/ me (we changed drawBitmapRectToRect to allow
something similar).

Since the output of filterBounds is device-space, an IRect still makes sense
here I presume.

[Stephen White, 2013-10-07 20:36:25]

On 2013/10/07 19:33:09, reed1 wrote:
> On 2013/10/07 19:04:28, Stephen White wrote:
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > File include/effects/SkRectShaderImageFilter.h (right):
> > 
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > include/effects/SkRectShaderImageFilter.h:24: static
SkRectShaderImageFilter*
> > Create(SkShader* s, const SkIRect& rect);
> > On 2013/10/07 18:14:24, reed1 wrote:
> > > General question:
> > > 
> > > What does the rect really mean? Is it
> > > 
> > >      "the portion of the src to which the filter is applied"?
> > 
> > Exactly. It also determines the size of the result bitmap, although I
suppose
> > this is an implementation detail.
> > 
> > > For some drawings, the src geometry can be very small, but we end up
scaling
> > the
> > > results to be very large (e.g. a global zoom on the outer canvas). For
this
> > > reason nearly all of our APIs take floats when talking about coordinates,
so
> > the
> > > caller can be very precise, regardless of scale.
> > 
> > In general, the crop rect represents the sub-region of the primitive to
> filter.
> > I made it an SkIRect simply to match the SkImageFilter::filterBounds() API
> which
> > you had written. It originally represented device-space pixels, so using
> integer
> > pixels made since. But it's now scaled by the CTM, so it may end up being
> > partial pixels anyway (not currently done by Chrome, but may in the future).
> So
> > we should probably change it to an SkRect at some point.
> 
>     bool filterBounds(const SkIRect& src, const SkMatrix& ctm, SkIRect* dst);
> 
> Ah, you are saying the src-rect here should really be fraction, even though
its
> in device-space? That's fine w/ me (we changed drawBitmapRectToRect to allow
> something similar).
> 
> Since the output of filterBounds is device-space, an IRect still makes sense
> here I presume.

Yes, I think that one could stay an IRect. Unfortunately, the one we do need to
change (the cropRect) will require some #ifdef and/or deprecation fun, since
it's already being called from Blink.

[reed1, 2013-10-07 20:42:25]

On 2013/10/07 20:36:25, Stephen White wrote:
> On 2013/10/07 19:33:09, reed1 wrote:
> > On 2013/10/07 19:04:28, Stephen White wrote:
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > File include/effects/SkRectShaderImageFilter.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > include/effects/SkRectShaderImageFilter.h:24: static
> SkRectShaderImageFilter*
> > > Create(SkShader* s, const SkIRect& rect);
> > > On 2013/10/07 18:14:24, reed1 wrote:
> > > > General question:
> > > > 
> > > > What does the rect really mean? Is it
> > > > 
> > > >      "the portion of the src to which the filter is applied"?
> > > 
> > > Exactly. It also determines the size of the result bitmap, although I
> suppose
> > > this is an implementation detail.
> > > 
> > > > For some drawings, the src geometry can be very small, but we end up
> scaling
> > > the
> > > > results to be very large (e.g. a global zoom on the outer canvas). For
> this
> > > > reason nearly all of our APIs take floats when talking about
coordinates,
> so
> > > the
> > > > caller can be very precise, regardless of scale.
> > > 
> > > In general, the crop rect represents the sub-region of the primitive to
> > filter.
> > > I made it an SkIRect simply to match the SkImageFilter::filterBounds() API
> > which
> > > you had written. It originally represented device-space pixels, so using
> > integer
> > > pixels made since. But it's now scaled by the CTM, so it may end up being
> > > partial pixels anyway (not currently done by Chrome, but may in the
future).
> > So
> > > we should probably change it to an SkRect at some point.
> > 
> >     bool filterBounds(const SkIRect& src, const SkMatrix& ctm, SkIRect*
dst);
> > 
> > Ah, you are saying the src-rect here should really be fraction, even though
> its
> > in device-space? That's fine w/ me (we changed drawBitmapRectToRect to allow
> > something similar).
> > 
> > Since the output of filterBounds is device-space, an IRect still makes sense
> > here I presume.
> 
> Yes, I think that one could stay an IRect. Unfortunately, the one we do need
to
> change (the cropRect) will require some #ifdef and/or deprecation fun, since
> it's already being called from Blink.

For drawBitmapRect -> drawBitmapRectToRect, we just made the IRect version be a
non-virtual wrapper for the new API. Can you do that here to start the
migration? Seems like we should not hold up the "correct/future" good API for
skia to test just because a few bad-blink-apples are using the older version :)

[reed1, 2013-10-07 20:43:15]

Regardless of the filterBounds api, I presume we should (now) keep the
float-rect on the constructor here, since that is more future-oriented...?

[Stephen White, 2013-10-07 21:06:37]

On 2013/10/07 20:42:25, reed1 wrote:
> On 2013/10/07 20:36:25, Stephen White wrote:
> > On 2013/10/07 19:33:09, reed1 wrote:
> > > On 2013/10/07 19:04:28, Stephen White wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > > File include/effects/SkRectShaderImageFilter.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > > include/effects/SkRectShaderImageFilter.h:24: static
> > SkRectShaderImageFilter*
> > > > Create(SkShader* s, const SkIRect& rect);
> > > > On 2013/10/07 18:14:24, reed1 wrote:
> > > > > General question:
> > > > > 
> > > > > What does the rect really mean? Is it
> > > > > 
> > > > >      "the portion of the src to which the filter is applied"?
> > > > 
> > > > Exactly. It also determines the size of the result bitmap, although I
> > suppose
> > > > this is an implementation detail.
> > > > 
> > > > > For some drawings, the src geometry can be very small, but we end up
> > scaling
> > > > the
> > > > > results to be very large (e.g. a global zoom on the outer canvas). For
> > this
> > > > > reason nearly all of our APIs take floats when talking about
> coordinates,
> > so
> > > > the
> > > > > caller can be very precise, regardless of scale.
> > > > 
> > > > In general, the crop rect represents the sub-region of the primitive to
> > > filter.
> > > > I made it an SkIRect simply to match the SkImageFilter::filterBounds()
API
> > > which
> > > > you had written. It originally represented device-space pixels, so using
> > > integer
> > > > pixels made since. But it's now scaled by the CTM, so it may end up
being
> > > > partial pixels anyway (not currently done by Chrome, but may in the
> future).
> > > So
> > > > we should probably change it to an SkRect at some point.
> > > 
> > >     bool filterBounds(const SkIRect& src, const SkMatrix& ctm, SkIRect*
> dst);
> > > 
> > > Ah, you are saying the src-rect here should really be fraction, even
though
> > its
> > > in device-space? That's fine w/ me (we changed drawBitmapRectToRect to
allow
> > > something similar).
> > > 
> > > Since the output of filterBounds is device-space, an IRect still makes
sense
> > > here I presume.
> > 
> > Yes, I think that one could stay an IRect. Unfortunately, the one we do need
> to
> > change (the cropRect) will require some #ifdef and/or deprecation fun, since
> > it's already being called from Blink.
> 
> For drawBitmapRect -> drawBitmapRectToRect, we just made the IRect version be
a
> non-virtual wrapper for the new API. Can you do that here to start the
> migration? Seems like we should not hold up the "correct/future" good API for
> skia to test just because a few bad-blink-apples are using the older version
:)

One wrinkle is that I need out-of-band values to indicate that there's no limit
on that dimension of the crop rect, and that the original primitives's bounds
should be used. SVG allows you to specify any subset of x, y, width, height. For
SkIRect, I'm using SK_[Min|Max]S32. When switching to SkRect, I suppose I could
use NaN or inf or something. Any suggestions?

The Blink code actually keeps a separate bool for each boundary. I was hoping to
avoid that, since it'll probably be ugly for SkImageFilter's
immutable-after-construction semantics.

[Stephen White, 2013-10-07 21:09:21]

On 2013/10/07 21:06:37, Stephen White wrote:
> On 2013/10/07 20:42:25, reed1 wrote:
> > On 2013/10/07 20:36:25, Stephen White wrote:
> > > On 2013/10/07 19:33:09, reed1 wrote:
> > > > On 2013/10/07 19:04:28, Stephen White wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > > > File include/effects/SkRectShaderImageFilter.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/26009004/diff/1/include/effects/SkRectShaderI...
> > > > > include/effects/SkRectShaderImageFilter.h:24: static
> > > SkRectShaderImageFilter*
> > > > > Create(SkShader* s, const SkIRect& rect);
> > > > > On 2013/10/07 18:14:24, reed1 wrote:
> > > > > > General question:
> > > > > > 
> > > > > > What does the rect really mean? Is it
> > > > > > 
> > > > > >      "the portion of the src to which the filter is applied"?
> > > > > 
> > > > > Exactly. It also determines the size of the result bitmap, although I
> > > suppose
> > > > > this is an implementation detail.
> > > > > 
> > > > > > For some drawings, the src geometry can be very small, but we end up
> > > scaling
> > > > > the
> > > > > > results to be very large (e.g. a global zoom on the outer canvas).
For
> > > this
> > > > > > reason nearly all of our APIs take floats when talking about
> > coordinates,
> > > so
> > > > > the
> > > > > > caller can be very precise, regardless of scale.
> > > > > 
> > > > > In general, the crop rect represents the sub-region of the primitive
to
> > > > filter.
> > > > > I made it an SkIRect simply to match the SkImageFilter::filterBounds()
> API
> > > > which
> > > > > you had written. It originally represented device-space pixels, so
using
> > > > integer
> > > > > pixels made since. But it's now scaled by the CTM, so it may end up
> being
> > > > > partial pixels anyway (not currently done by Chrome, but may in the
> > future).
> > > > So
> > > > > we should probably change it to an SkRect at some point.
> > > > 
> > > >     bool filterBounds(const SkIRect& src, const SkMatrix& ctm, SkIRect*
> > dst);
> > > > 
> > > > Ah, you are saying the src-rect here should really be fraction, even
> though
> > > its
> > > > in device-space? That's fine w/ me (we changed drawBitmapRectToRect to
> allow
> > > > something similar).
> > > > 
> > > > Since the output of filterBounds is device-space, an IRect still makes
> sense
> > > > here I presume.
> > > 
> > > Yes, I think that one could stay an IRect. Unfortunately, the one we do
need
> > to
> > > change (the cropRect) will require some #ifdef and/or deprecation fun,
since
> > > it's already being called from Blink.
> > 
> > For drawBitmapRect -> drawBitmapRectToRect, we just made the IRect version
be
> a
> > non-virtual wrapper for the new API. Can you do that here to start the
> > migration? Seems like we should not hold up the "correct/future" good API
for
> > skia to test just because a few bad-blink-apples are using the older version
> :)
> 
> One wrinkle is that I need out-of-band values to indicate that there's no
limit
> on that dimension of the crop rect, and that the original primitives's bounds
> should be used. SVG allows you to specify any subset of x, y, width, height.
For
> SkIRect, I'm using SK_[Min|Max]S32. When switching to SkRect, I suppose I
could
> use NaN or inf or something. Any suggestions?

(I see now I could probably use SK_ScalarMax or SK_ScalarInfinity.)

> The Blink code actually keeps a separate bool for each boundary. I was hoping
to
> avoid that, since it'll probably be ugly for SkImageFilter's
> immutable-after-construction semantics.

[Stephen White, 2013-10-08 14:39:45]

On 2013/10/07 20:43:15, reed1 wrote:
> Regardless of the filterBounds api, I presume we should (now) keep the
> float-rect on the constructor here, since that is more future-oriented...?

OK, I've uploaded https://codereview.chromium.org/26364003/ to convert fCropRect
to an SkRect. I've reverted the external Create() function here to take an
SkRect. If we land the former first, I can convert the (private) constructor
here as well.

[Stephen White, 2013-10-08 14:42:45]

On 2013/10/08 14:39:45, Stephen White wrote:
> On 2013/10/07 20:43:15, reed1 wrote:
> > Regardless of the filterBounds api, I presume we should (now) keep the
> > float-rect on the constructor here, since that is more future-oriented...?
> 
> OK, I've uploaded https://codereview.chromium.org/26364003/ 

(that should be https://codereview.chromium.org/26371002/; rietveld upload went
south on that version.)

> to convert fCropRect
> to an SkRect. I've reverted the external Create() function here to take an
> SkRect. If we land the former first, I can convert the (private) constructor
> here as well.

[Stephen White, 2013-10-10 18:16:08]

Patch set 3: Updated to ToT, and CropRect API.

[reed1, 2013-10-10 18:46:13]

lgtm

[Stephen White, 2013-10-10 21:36:51]

Committed patchset #3 manually as r11720 (presubmit successful).