Generalize DAG connectivity for SkImageFilter

This patch implements generalized DAG connectivity for SkImageFilter.  SkImageFilter maintains a list of inputs, which can be constructed either from a SkImageFilter** or zero or more SkImageFilter* arguments (varargs).

Existing filters which maintained their own filter connectivity were refactored to use the new constructors and flattening/unflattening code.  Modifying the remaining filters which are not yet DAG-friendly is left for future work; they are considered to have zero inputs for now.

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

[reed1, 2012-10-04 15:41:34]

I think the question of how/when to specify other parameters (inputs) is a
healthy one.

This model fixes the number of inputs on creation, but does not necessarily
require the inputs to be specified then.

A yet more strick model, the might allow ImageFilters to be "immutable" would be
that the actual inputs must be sent to the base SkImageFilter construct, with no
setter available. This may give some advantages for caching and cloning.

e.g.

SkImageFIlter(int count, SkImageFIlter* [])

A very different model would be to take this out of the constructor completely.
And allow the list to grow/shrink/change afterwards (e.g. like SkTArray). This
may make caching/cloning harder, but does allow the client to re-purpose a given
instance.

e.g.

imagefilter.setInput(A);
draw(..., imagefilter);
imagefilter.setInput(B);
draw(..., imagefilter);

I am not concerned at all (for the most part) about the micro efficiency of when
to allocate that array internally. I think performance and API clarity are more
interesting.

ImageFilters are a new beast for skia, so I'm not sure how much it should be
guided by past API experience. Certainly, things are easier w/ immutable objects
(e.g. xfermodes, typeface, bitmap), but we do survive with mutable ones (e.g.
shaders).

+ some of the other picture/pipe/caching folks for input.

https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:101: SkASSERT(i < fNumInputs);
SkRefCnt_SafeAssign(fInputs[i], filter);

This guy also handles the case when the two are the same

[Leon, 2012-10-04 17:54:58]

+1 for the effort of moving common functionality into the base class.

I have not looked through all of the filters' APIs, but it looks like we
typically do not have a way to change a filter after it is created. If that is
the way they will work, I do agree that it makes sense to make them immutable.

That said, for the way that pipe and picture (Derek, correct me if I'm wrong)
currently work, it does not matter whether the filters are immutable or not. We
flatten the filter, then compare the flattened version to other flattened
objects to determine whether they are the same. If they were immutable, we could
potentially speed up the comparison, but I'm not sure what the overall impact
would be on performance.

However we go about it, we do need to bump the picture version.

[Stephen White, 2012-10-04 18:12:55]

Thanks for your comments.  Just for background, here was my original comment to
Mike:

> One thing I'm not entirely happy with is having the SkImageFilter constructor
take the number of inputs.  It saves me from having to reallocate the input
array on the fly, but maybe that's a premature optimization.  Let me know what
you think.

On 2012/10/04 15:41:34, reed1 wrote:
> I think the question of how/when to specify other parameters (inputs) is a
> healthy one.
> 
> This model fixes the number of inputs on creation, but does not necessarily
> require the inputs to be specified then.
> 
> A yet more strick model, the might allow ImageFilters to be "immutable" would
be
> that the actual inputs must be sent to the base SkImageFilter construct, with
no
> setter available. This may give some advantages for caching and cloning.
> 
> e.g.
> 
> SkImageFIlter(int count, SkImageFIlter* [])

It's a good idea, and I wanted to do it that way.  The problem I ran into
(probably just me having a mental C++ block) are the filters which already take
separate parameters, where I couldn't figure out a way to cons up an array from
them within the base class constructor call, since it needs to be an expression.
 

e.g., SkComposeImageFilter(SkImageFilter* outer, SkImageFilter* inner) :
INHERITED(2, ???)

Jeff suggests maybe a make_array() function that returns a dynamic array, maybe
an auto_ptr or SkTArray, that we return to the initializer and get() from.

> A very different model would be to take this out of the constructor
completely.
> And allow the list to grow/shrink/change afterwards (e.g. like SkTArray). This
> may make caching/cloning harder, but does allow the client to re-purpose a
given
> instance.
> 
> e.g.
> 
> imagefilter.setInput(A);
> draw(..., imagefilter);
> imagefilter.setInput(B);
> draw(..., imagefilter);
> 
> I am not concerned at all (for the most part) about the micro efficiency of
when
> to allocate that array internally. I think performance and API clarity are
more
> interesting.

Since the rest of the parameters of image filters are immutable, I think it kind
of makes sense to make the inputs immutable too.

> ImageFilters are a new beast for skia, so I'm not sure how much it should be
> guided by past API experience. Certainly, things are easier w/ immutable
objects
> (e.g. xfermodes, typeface, bitmap), but we do survive with mutable ones (e.g.
> shaders).
> 
> + some of the other picture/pipe/caching folks for input.
> 
> https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp#n...
> src/core/SkImageFilter.cpp:101: SkASSERT(i < fNumInputs);
> SkRefCnt_SafeAssign(fInputs[i], filter);
> 
> This guy also handles the case when the two are the same

Thanks, will fix.

[bsalomon, 2012-10-04 18:18:00]

On 2012/10/04 18:12:55, Stephen White wrote:
> Thanks for your comments.  Just for background, here was my original comment
to
> Mike:
> 
> > One thing I'm not entirely happy with is having the SkImageFilter
constructor
> take the number of inputs.  It saves me from having to reallocate the input
> array on the fly, but maybe that's a premature optimization.  Let me know what
> you think.
> 
> On 2012/10/04 15:41:34, reed1 wrote:
> > I think the question of how/when to specify other parameters (inputs) is a
> > healthy one.
> > 
> > This model fixes the number of inputs on creation, but does not necessarily
> > require the inputs to be specified then.
> > 
> > A yet more strick model, the might allow ImageFilters to be "immutable"
would
> be
> > that the actual inputs must be sent to the base SkImageFilter construct,
with
> no
> > setter available. This may give some advantages for caching and cloning.
> > 
> > e.g.
> > 
> > SkImageFIlter(int count, SkImageFIlter* [])
> 
> It's a good idea, and I wanted to do it that way.  The problem I ran into
> (probably just me having a mental C++ block) are the filters which already
take
> separate parameters, where I couldn't figure out a way to cons up an array
from
> them within the base class constructor call, since it needs to be an
expression.
>  
> 
> e.g., SkComposeImageFilter(SkImageFilter* outer, SkImageFilter* inner) :
> INHERITED(2, ???)

var_args, baby. I'm gonna at least try typing it for custom effect textures.


> 
> Jeff suggests maybe a make_array() function that returns a dynamic array,
maybe
> an auto_ptr or SkTArray, that we return to the initializer and get() from.
> 
> > A very different model would be to take this out of the constructor
> completely.
> > And allow the list to grow/shrink/change afterwards (e.g. like SkTArray).
This
> > may make caching/cloning harder, but does allow the client to re-purpose a
> given
> > instance.
> > 
> > e.g.
> > 
> > imagefilter.setInput(A);
> > draw(..., imagefilter);
> > imagefilter.setInput(B);
> > draw(..., imagefilter);
> > 
> > I am not concerned at all (for the most part) about the micro efficiency of
> when
> > to allocate that array internally. I think performance and API clarity are
> more
> > interesting.
> 
> Since the rest of the parameters of image filters are immutable, I think it
kind
> of makes sense to make the inputs immutable too.
> 
> > ImageFilters are a new beast for skia, so I'm not sure how much it should be
> > guided by past API experience. Certainly, things are easier w/ immutable
> objects
> > (e.g. xfermodes, typeface, bitmap), but we do survive with mutable ones
(e.g.
> > shaders).
> > 
> > + some of the other picture/pipe/caching folks for input.
> > 
> > https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp
> > File src/core/SkImageFilter.cpp (right):
> > 
> >
>
https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp#n...
> > src/core/SkImageFilter.cpp:101: SkASSERT(i < fNumInputs);
> > SkRefCnt_SafeAssign(fInputs[i], filter);
> > 
> > This guy also handles the case when the two are the same
> 
> Thanks, will fix.

[Stephen White, 2012-10-09 17:02:41]

On 2012/10/04 18:18:00, bsalomon wrote:
> On 2012/10/04 18:12:55, Stephen White wrote:
> > It's a good idea, and I wanted to do it that way.  The problem I ran into
> > (probably just me having a mental C++ block) are the filters which already
> take
> > separate parameters, where I couldn't figure out a way to cons up an array
> from
> > them within the base class constructor call, since it needs to be an
> expression.
> >  
> > 
> > e.g., SkComposeImageFilter(SkImageFilter* outer, SkImageFilter* inner) :
> > INHERITED(2, ???)
> 
> var_args, baby. I'm gonna at least try typing it for custom effect textures.

Good idea.  One thing that concerns me is the lack of type safety, but at least
it should only be confined to the implementation (ie., protected constructor). 
I'll give it a shot.

> 
> 
> > 
> > Jeff suggests maybe a make_array() function that returns a dynamic array,
> maybe
> > an auto_ptr or SkTArray, that we return to the initializer and get() from.
> > 
> > > A very different model would be to take this out of the constructor
> > completely.
> > > And allow the list to grow/shrink/change afterwards (e.g. like SkTArray).
> This
> > > may make caching/cloning harder, but does allow the client to re-purpose a
> > given
> > > instance.
> > > 
> > > e.g.
> > > 
> > > imagefilter.setInput(A);
> > > draw(..., imagefilter);
> > > imagefilter.setInput(B);
> > > draw(..., imagefilter);
> > > 
> > > I am not concerned at all (for the most part) about the micro efficiency
of
> > when
> > > to allocate that array internally. I think performance and API clarity are
> > more
> > > interesting.
> > 
> > Since the rest of the parameters of image filters are immutable, I think it
> kind
> > of makes sense to make the inputs immutable too.
> > 
> > > ImageFilters are a new beast for skia, so I'm not sure how much it should
be
> > > guided by past API experience. Certainly, things are easier w/ immutable
> > objects
> > > (e.g. xfermodes, typeface, bitmap), but we do survive with mutable ones
> (e.g.
> > > shaders).
> > > 
> > > + some of the other picture/pipe/caching folks for input.
> > > 
> > >
https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp
> > > File src/core/SkImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/6443119/diff/6001/src/core/SkImageFilter.cpp#n...
> > > src/core/SkImageFilter.cpp:101: SkASSERT(i < fNumInputs);
> > > SkRefCnt_SafeAssign(fInputs[i], filter);
> > > 
> > > This guy also handles the case when the two are the same
> > 
> > Thanks, will fix.

[reed1, 2012-10-10 15:29:41]

I like the simplification of requiring all inputs up front. LGTM w/ request for
some dox and internal-dox (either enum or getters) to clarify what is meant by
getInput(0) or getInput(1).

https://codereview.appspot.com/6443119/diff/33012/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.appspot.com/6443119/diff/33012/include/core/SkImageFilter....
include/core/SkImageFilter.h:112: SkImageFilter(int numInputs, SkImageFilter**
inputs);
// the ... represents numInputs SkImageFilter pointers, upon which this
// constructor will call SkSafeRef(). This is the same behavior as the
// SkImageFilter(int, SkImageFilter**) constructor.

https://codereview.appspot.com/6443119/diff/33012/src/effects/SkBlendImageFil...
File src/effects/SkBlendImageFilter.cpp (right):

https://codereview.appspot.com/6443119/diff/33012/src/effects/SkBlendImageFil...
src/effects/SkBlendImageFilter.cpp:78: SkBitmap background, foreground = src;
Might want to consider helper getters or a private enum to document in a single
place which input is background and which is foreground. Not sure if this would
help other subclasses as well or not.

[Stephen White, 2012-10-10 19:11:54]

https://codereview.appspot.com/6443119/diff/33012/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.appspot.com/6443119/diff/33012/include/core/SkImageFilter....
include/core/SkImageFilter.h:112: SkImageFilter(int numInputs, SkImageFilter**
inputs);
On 2012/10/10 15:29:41, reed1 wrote:
> // the ... represents numInputs SkImageFilter pointers, upon which this
> // constructor will call SkSafeRef(). This is the same behavior as the
> // SkImageFilter(int, SkImageFilter**) constructor.

Done.

https://codereview.appspot.com/6443119/diff/33012/src/effects/SkBlendImageFil...
File src/effects/SkBlendImageFilter.cpp (right):

https://codereview.appspot.com/6443119/diff/33012/src/effects/SkBlendImageFil...
src/effects/SkBlendImageFilter.cpp:78: SkBitmap background, foreground = src;
On 2012/10/10 15:29:41, reed1 wrote:
> Might want to consider helper getters or a private enum to document in a
single
> place which input is background and which is foreground. Not sure if this
would
> help other subclasses as well or not.

Done.