Factory methods for heap-allocated SkImageFilter objects.

Factory methods for heap-allocated SkImageFilter objects.

This is part of an effort to ensure that all SkPaint effects can only be
allocated on the heap.

This patch makes the constructors of SkImageFilter and its subclasses non-public
and instead provides factory methods for creating these objects on the heap. We
temporarily keep constructor of publicly visible classes public behind a flag.

BUG=skia:2187
Committed: http://code.google.com/p/skia/source/detail?r=13718

[Dominik Grewe, 2014-03-03 17:17:55]

This is, I believe, the last of the "make paint effects only allocatable on
heap" patches. PTAL.

Apart from SkPictureImageFilter all subclasses are immutable.
SkPictureImageFilter takes a pointer to an SkPicture, which is not immutable and
possibly never will be (because it holds an SkPictureRecord which isn't
immutable).

[reed1, 2014-03-03 18:23:19]

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBlurIm...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBlurIm...
include/effects/SkBlurImageFilter.h:21: }
nit: LF after }

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkDropSh...
File include/effects/SkDropShadowImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkDropSh...
include/effects/SkDropShadowImageFilter.h:16: return
SkNEW_ARGS(SkDropShadowImageFilter, (dx, dy, sigma, color, input));
Perhaps not needed in this CL, but the filter team should be encouraged to
document (now that we're using factories) that the factory might return NULL for
mal-formed parameters (not just dropshadow, but in general).

Factories also enable us to use specialized/alternate impls based on input,
rather than forcing us to create the actual subclass the client thought they
were asking for.

[scroggo, 2014-03-03 19:11:53]

The CL lgtm. Adding Stephen in case he has any thoughts, but it appears to be
pretty mechanical.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkMorpho...
File include/effects/SkMorphologyImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkMorpho...
include/effects/SkMorphologyImageFilter.h:32: SkMorphologyImageFilter(int
radiusX, int radiusY, SkImageFilter* input, const CropRect* cropRect);
nit: line too long

[Stephen White, 2014-03-03 19:48:04]

LGTM

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBitmap...
File include/effects/SkBitmapSource.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBitmap...
include/effects/SkBitmapSource.h:34: #ifdef
SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
Might want to give Robert a heads-up that this will need to be set in Chrome's
skia_common.gypi when rolling Skia DEPS.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkDropSh...
File include/effects/SkDropShadowImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkDropSh...
include/effects/SkDropShadowImageFilter.h:16: return
SkNEW_ARGS(SkDropShadowImageFilter, (dx, dy, sigma, color, input));
On 2014/03/03 18:23:19, reed1 wrote:
> Perhaps not needed in this CL, but the filter team should be encouraged to
> document (now that we're using factories) that the factory might return NULL
for
> mal-formed parameters (not just dropshadow, but in general).
> 
> Factories also enable us to use specialized/alternate impls based on input,
> rather than forcing us to create the actual subclass the client thought they
> were asking for.

Yep! It would be great to move to factories for deserialization (SkReadBuffer
construction) too, so that we can fail gracefully without having to early-out
and an invalid object.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkResize...
File include/effects/SkResizeImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkResize...
include/effects/SkResizeImageFilter.h:55: SkImageFilter* input = NULL);
Note for the future: when we remove the #idef, we should probably remove the
default arguments as well (since the factory fn will always be passing all
args).

[Dominik Grewe, 2014-03-04 10:29:00]

Thanks. Just need a review from reed@ for changes to the public API.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBitmap...
File include/effects/SkBitmapSource.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBitmap...
include/effects/SkBitmapSource.h:34: #ifdef
SK_SUPPORT_LEGACY_PUBLICEFFECTCONSTRUCTORS
On 2014/03/03 19:48:04, Stephen White wrote:
> Might want to give Robert a heads-up that this will need to be set in Chrome's
> skia_common.gypi when rolling Skia DEPS.

This is already included in that file (via the new
skia_for_chromium_defines.gypi in Skia)

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBlurIm...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkBlurIm...
include/effects/SkBlurImageFilter.h:21: }
On 2014/03/03 18:23:19, reed1 wrote:
> nit: LF after }

Done.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkMorpho...
File include/effects/SkMorphologyImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkMorpho...
include/effects/SkMorphologyImageFilter.h:32: SkMorphologyImageFilter(int
radiusX, int radiusY, SkImageFilter* input, const CropRect* cropRect);
On 2014/03/03 19:11:54, scroggo wrote:
> nit: line too long

Done.

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkResize...
File include/effects/SkResizeImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/40001/include/effects/SkResize...
include/effects/SkResizeImageFilter.h:55: SkImageFilter* input = NULL);
On 2014/03/03 19:48:04, Stephen White wrote:
> Note for the future: when we remove the #idef, we should probably remove the
> default arguments as well (since the factory fn will always be passing all
> args).

Yes, will do that.

[reed1, 2014-03-04 16:56:42]

Before we rev. chrome to use the new factories, perhaps we can take time to do
any housekeeping/cleanup to them, ... so we don't have to rev. chrome twice.

For example, many of these have several default parameter arguments. This has
almost always come back to bite us in other places in Skia's public API. Can we
reduce this to 0, or at most 1 for the factories. Alternatively, offering
multiple entry-points (as factories) seems a fine way to accommodate simple -vs-
complex variations.

As I stated before, we *really* should begin the process of having the factories
perform some sanity checks (returning NULL on failure), and so we may need to
update Chrome's callsites immediately to know to check for NULL (or at least to
not crash accidentally).

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
File include/effects/SkDisplacementMapEffect.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
include/effects/SkDisplacementMapEffect.h:22: kKeyBits = 3 // Max value is 4, so
3 bits are required at most
I know its not part of this CL, but why is this enum value here, since it is a
dup of kB_ChannelSelectorType?
Does it have to be public?

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
File include/effects/SkMagnifierImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
include/effects/SkMagnifierImageFilter.h:17: static SkMagnifierImageFilter*
Create(SkRect srcRect, SkScalar inset) {
Shouldn't this be (const SkRect& srcRect, ...) ?

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
File include/effects/SkMatrixConvolutionImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
include/effects/SkMatrixConvolutionImageFilter.h:58: const SkIPoint& target,
Nit: Seems funny to separate kernelSize and kernalTarget (my label) by 4
parameters.

Also, this seems like a good place for the factory to sanity check the inputs...
if the kernelTarget is outside of [0..kernelSize]...

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

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:27: static SkPictureImageFilter*
Create(SkPicture* picture, const SkRect& rect) {
'rect' is a pretty unmeaningful parameter name. Does it become the 'bounds' of
the src image? When creating a raster of the picture (internally) is the picture
translated by -rect.top/left and then clipped by rect?

[Stephen White, 2014-03-04 18:18:00]

+sugoi, +zork

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
File include/effects/SkDisplacementMapEffect.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
include/effects/SkDisplacementMapEffect.h:22: kKeyBits = 3 // Max value is 4, so
3 bits are required at most
On 2014/03/04 16:56:42, reed1 wrote:
> I know its not part of this CL, but why is this enum value here, since it is a
> dup of kB_ChannelSelectorType?
> Does it have to be public?

It's not a dupe of that value; it's the number of bits required to represent the
enum. I agree that it shouldn't be public; should probably just be in the .cpp.

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
File include/effects/SkMagnifierImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
include/effects/SkMagnifierImageFilter.h:17: static SkMagnifierImageFilter*
Create(SkRect srcRect, SkScalar inset) {
On 2014/03/04 16:56:42, reed1 wrote:
> Shouldn't this be (const SkRect& srcRect, ...) ?

Probably. (zork@?)

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
File include/effects/SkMatrixConvolutionImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
include/effects/SkMatrixConvolutionImageFilter.h:58: const SkIPoint& target,
On 2014/03/04 16:56:42, reed1 wrote:
> Nit: Seems funny to separate kernelSize and kernalTarget (my label) by 4
> parameters.

I think that was just the order they appeared in the SVG spec. Moving it up
SGTM. Blink calls target "targetOffset". If we're going to go with something
different than those two, I'd prefer "kernelOffset".

> Also, this seems like a good place for the factory to sanity check the
inputs...
> if the kernelTarget is outside of [0..kernelSize]...

Sounds good.

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

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:27: static SkPictureImageFilter*
Create(SkPicture* picture, const SkRect& rect) {
On 2014/03/04 16:56:42, reed1 wrote:
> 'rect' is a pretty unmeaningful parameter name. Does it become the 'bounds' of
> the src image? When creating a raster of the picture (internally) is the
picture
> translated by -rect.top/left and then clipped by rect?

Yep, it's the bounds in world space (it gets transformed by the CTM). We could
change it to cropRect like the other filters, but all its edges are mandatory
(unlike the cropRect), so that might be misleading.

[Dominik Grewe, 2014-03-07 13:40:28]

When do you want to make all those changes?
How about I land this one now as it is and then someone with more knowledge of
these classes lands the other changes in a follow-up patch?

[reed1, 2014-03-07 13:55:59]

On 2014/03/07 13:40:28, Dominik Grewe wrote:
> When do you want to make all those changes?
> How about I land this one now as it is and then someone with more knowledge of
> these classes lands the other changes in a follow-up patch?

My nits can be addressed in a separate CL, but should be soon, before we make
any substantial changes to chrome...

api lgtm

[Dominik Grewe, 2014-03-07 14:06:32]

On 2014/03/07 13:55:59, reed1 wrote:
> On 2014/03/07 13:40:28, Dominik Grewe wrote:
> > When do you want to make all those changes?
> > How about I land this one now as it is and then someone with more knowledge
of
> > these classes lands the other changes in a follow-up patch?
> 
> My nits can be addressed in a separate CL, but should be soon, before we make
> any substantial changes to chrome...
> 
> api lgtm

Actually, looking at it again, I'm happy to address most of the nits here. It's
just things like sanity checking the inputs that I'd rather leave to someone
that knows more about what these classes are actually doing.

[reed1, 2014-03-07 14:07:38]

On 2014/03/07 14:06:32, Dominik Grewe wrote:
> On 2014/03/07 13:55:59, reed1 wrote:
> > On 2014/03/07 13:40:28, Dominik Grewe wrote:
> > > When do you want to make all those changes?
> > > How about I land this one now as it is and then someone with more
knowledge
> of
> > > these classes lands the other changes in a follow-up patch?
> > 
> > My nits can be addressed in a separate CL, but should be soon, before we
make
> > any substantial changes to chrome...
> > 
> > api lgtm
> 
> Actually, looking at it again, I'm happy to address most of the nits here.
It's
> just things like sanity checking the inputs that I'd rather leave to someone
> that knows more about what these classes are actually doing.

Completely agree that the sanity checks should be done separately (with
corresponding tests). I will file a bug.

[reed1, 2014-03-07 14:10:18]

On 2014/03/07 14:07:38, reed1 wrote:
> On 2014/03/07 14:06:32, Dominik Grewe wrote:
> > On 2014/03/07 13:55:59, reed1 wrote:
> > > On 2014/03/07 13:40:28, Dominik Grewe wrote:
> > > > When do you want to make all those changes?
> > > > How about I land this one now as it is and then someone with more
> knowledge
> > of
> > > > these classes lands the other changes in a follow-up patch?
> > > 
> > > My nits can be addressed in a separate CL, but should be soon, before we
> make
> > > any substantial changes to chrome...
> > > 
> > > api lgtm
> > 
> > Actually, looking at it again, I'm happy to address most of the nits here.
> It's
> > just things like sanity checking the inputs that I'd rather leave to someone
> > that knows more about what these classes are actually doing.
> 
> Completely agree that the sanity checks should be done separately (with
> corresponding tests). I will file a bug.

https://code.google.com/p/skia/issues/detail?id=2269

[Dominik Grewe, 2014-03-07 16:43:37]

Uploaded PS5. PTAL.

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
File include/effects/SkDisplacementMapEffect.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkDispla...
include/effects/SkDisplacementMapEffect.h:22: kKeyBits = 3 // Max value is 4, so
3 bits are required at most
On 2014/03/04 18:18:00, Stephen White wrote:
> On 2014/03/04 16:56:42, reed1 wrote:
> > I know its not part of this CL, but why is this enum value here, since it is
a
> > dup of kB_ChannelSelectorType?
> > Does it have to be public?
> 
> It's not a dupe of that value; it's the number of bits required to represent
the
> enum. I agree that it shouldn't be public; should probably just be in the
.cpp.

I've replaced it with a #define inside the .cpp file.

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
File include/effects/SkMagnifierImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMagnif...
include/effects/SkMagnifierImageFilter.h:17: static SkMagnifierImageFilter*
Create(SkRect srcRect, SkScalar inset) {
On 2014/03/04 18:18:00, Stephen White wrote:
> On 2014/03/04 16:56:42, reed1 wrote:
> > Shouldn't this be (const SkRect& srcRect, ...) ?
> 
> Probably. (zork@?)

Done (and for constructor below).

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
File include/effects/SkMatrixConvolutionImageFilter.h (right):

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkMatrix...
include/effects/SkMatrixConvolutionImageFilter.h:58: const SkIPoint& target,
On 2014/03/04 18:18:00, Stephen White wrote:
> On 2014/03/04 16:56:42, reed1 wrote:
> > Nit: Seems funny to separate kernelSize and kernalTarget (my label) by 4
> > parameters.
> 
> I think that was just the order they appeared in the SVG spec. Moving it up
> SGTM. Blink calls target "targetOffset". If we're going to go with something
> different than those two, I'd prefer "kernelOffset".
> 
> > Also, this seems like a good place for the factory to sanity check the
> inputs...
> > if the kernelTarget is outside of [0..kernelSize]...
> 
> Sounds good.

There didn't seem to be a clear consensus here so I just went ahead a renamed
everything to kernelOffset for now.

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

https://codereview.chromium.org/182983003/diff/50001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:27: static SkPictureImageFilter*
Create(SkPicture* picture, const SkRect& rect) {
On 2014/03/04 18:18:00, Stephen White wrote:
> On 2014/03/04 16:56:42, reed1 wrote:
> > 'rect' is a pretty unmeaningful parameter name. Does it become the 'bounds'
of
> > the src image? When creating a raster of the picture (internally) is the
> picture
> > translated by -rect.top/left and then clipped by rect?
> 
> Yep, it's the bounds in world space (it gets transformed by the CTM). We could
> change it to cropRect like the other filters, but all its edges are mandatory
> (unlike the cropRect), so that might be misleading.

Changed it to cropRect.

[reed1, 2014-03-07 17:41:50]

api lgtm

[Dominik Grewe, 2014-03-10 10:36:23]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-03-10 10:36:33]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/182983003/110001

[commit-bot: I haz the power, 2014-03-10 10:52:15]

Change committed as 13718

[Stephen White, 2014-03-10 20:58:47]

https://codereview.chromium.org/182983003/diff/70001/bench/MatrixConvolutionB...
File bench/MatrixConvolutionBench.cpp (right):

https://codereview.chromium.org/182983003/diff/70001/bench/MatrixConvolutionB...
bench/MatrixConvolutionBench.cpp:25: SkIPoint target = SkIPoint::Make(1, 1);
Nit: should probably rename this to kernelOffset too.

[Dominik Grewe, 2014-03-12 10:05:56]

https://codereview.chromium.org/182983003/diff/70001/bench/MatrixConvolutionB...
File bench/MatrixConvolutionBench.cpp (right):

https://codereview.chromium.org/182983003/diff/70001/bench/MatrixConvolutionB...
bench/MatrixConvolutionBench.cpp:25: SkIPoint target = SkIPoint::Make(1, 1);
On 2014/03/10 20:58:48, Stephen White wrote:
> Nit: should probably rename this to kernelOffset too.

Thanks for spotting this! I've fixed it (and two other places) in a follow-up
patch: https://codereview.chromium.org/197013003