Optimize color matrix filters

When two or more color matrix image filters are connected together, and the non-leaf matrices do not require clamping, we can concatenate their matrices and apply them together.

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

[reed1, 2012-08-30 11:52:22]

Thank you, I think this is more defensive at the API level.

If we can avoid the cast, that seems cleaner (see inline comment).

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

https://codereview.appspot.com/6489054/diff/1/include/core/SkImageFilter.h#ne...
include/core/SkImageFilter.h:113: */
Can this also work as:

virtual SkColorFilter* asAColorFilter() const;

This way no need for a cast later on, and conceivably some other imagefilter
subclass could also return this.

[schenney, 2012-08-30 18:49:30]

What happens if a single color matrix requires clamping?

Otherwise no complaints from me.

https://codereview.appspot.com/6489054/diff/1/src/effects/SkColorFilterImageF...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/1/src/effects/SkColorFilterImageF...
src/effects/SkColorFilterImageFilter.cpp:87: while (parent &&
parent->isColorFilter()) {
Are you testing this? It's not clear to me.

[Stephen White, 2012-09-10 14:34:17]

> What happens if a single color matrix requires clamping?

A single color matrix clamps implicitly when it writes to the framebuffer. 
That's the semantic that this code preserves:  only concatenate the parent
matrix if it wouldn't have caused clamping if it had been written to the
framebuffer.

https://codereview.appspot.com/6489054/diff/1/src/effects/SkColorFilterImageF...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/1/src/effects/SkColorFilterImageF...
src/effects/SkColorFilterImageFilter.cpp:87: while (parent &&
parent->isColorFilter()) {
On 2012/08/30 18:49:30, schenney wrote:
> Are you testing this? It's not clear to me.

Good point.  Many tests exercise parent being non-NULL, but it looks like there
isn't a test that exercises isColorFilter() being false here.  I'll add one.

[Stephen White, 2012-09-10 14:45:36]

On 2012/08/30 11:52:22, reed1 wrote:
> Thank you, I think this is more defensive at the API level.
> 
> If we can avoid the cast, that seems cleaner (see inline comment).
> 
> https://codereview.appspot.com/6489054/diff/1/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.appspot.com/6489054/diff/1/include/core/SkImageFilter.h#ne...
> include/core/SkImageFilter.h:113: */
> Can this also work as:
> 
> virtual SkColorFilter* asAColorFilter() const;
> 
> This way no need for a cast later on, and conceivably some other imagefilter
> subclass could also return this.

This doesn't work, since I still need to cast the parent to
SkColorFilterImageFilter (or at least SkSingleInputImageFilter) in order to get
its input (line 96).  As in the other patch, we need two things out of the
parent:  its matrix, and its input (the filter we need to apply the matrix to).

[Stephen White, 2012-09-10 21:34:35]

With another test, and using asAColorFilterImageFilter

[Stephen White, 2012-09-10 21:35:26]

On 2012/09/10 21:34:35, Stephen White wrote:
> With another test, and using asAColorFilterImageFilter

This version gets rid of the cast by using a virtual
asAColorFilterImageFilter().  It does mean SkImageFilter references its
SkColorFilterImageFilter subclass, if only by forward reference.  Let me know if
you prefer it this way.

[reed1, 2012-09-11 12:20:09]

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

https://codereview.appspot.com/6489054/diff/10001/include/core/SkImageFilter....
include/core/SkImageFilter.h:111: 
SkColorFilter* asAColorFilter() seems a more general query. Can that suffice? If
so, it means we don't necessarily have to perform a cast, since we've been
handed the actual colorfilter object.

[Stephen White, 2012-09-11 14:32:33]

On 2012/09/11 12:20:09, reed1 wrote:
> https://codereview.appspot.com/6489054/diff/10001/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.appspot.com/6489054/diff/10001/include/core/SkImageFilter....
> include/core/SkImageFilter.h:111: 
> SkColorFilter* asAColorFilter() seems a more general query. Can that suffice?
If
> so, it means we don't necessarily have to perform a cast, since we've been
> handed the actual colorfilter object.

See previous comment:

"This doesn't work, since I still need to cast the parent to
SkColorFilterImageFilter (or at least SkSingleInputImageFilter) in order to get
its input (line 96).  As in the other patch, we need two things out of the
parent:  its matrix, and its input (the filter we need to apply the matrix to)."

Another way to get the input would be to implement a fully-generalized DAG API,
as in https://codereview.appspot.com/6443119/.

So far, I think I like the previous version of this patch the best, since it
just returns a bool, not adding knowledge of the SkColorFilterImageFilter
subclass to the base class (albeit just a forward declaration).  I'm open to
other suggestions, though.

[Stephen White, 2012-09-11 14:36:41]

On 2012/09/11 14:32:33, Stephen White wrote:
> On 2012/09/11 12:20:09, reed1 wrote:
> >
https://codereview.appspot.com/6489054/diff/10001/include/core/SkImageFilter.h
> > File include/core/SkImageFilter.h (right):
> > 
> >
>
https://codereview.appspot.com/6489054/diff/10001/include/core/SkImageFilter....
> > include/core/SkImageFilter.h:111: 
> > SkColorFilter* asAColorFilter() seems a more general query. Can that
suffice?
> If
> > so, it means we don't necessarily have to perform a cast, since we've been
> > handed the actual colorfilter object.
> 
> See previous comment:
> 
> "This doesn't work, since I still need to cast the parent to
> SkColorFilterImageFilter (or at least SkSingleInputImageFilter) in order to
get
> its input (line 96).  As in the other patch, we need two things out of the
> parent:  its matrix, and its input (the filter we need to apply the matrix
to)."
> 
> Another way to get the input would be to implement a fully-generalized DAG
API,
> as in https://codereview.appspot.com/6443119/.
> 
> So far, I think I like the previous version of this patch the best, since it
> just returns a bool, not adding knowledge of the SkColorFilterImageFilter
> subclass to the base class (albeit just a forward declaration).  I'm open to
> other suggestions, though.

If you don't like the fully-generalized DAG, we could just make input retrieval
an abstract API on SkImageFilter:

virtual SkImageFilter* input(int index) const { return NULL; }

To the base class.  Then SkSingleInputImageFilter could override it:

virtual SkImageFilter* input(int index) const { SkASSERT(index == 0); return
fInput; }

(The index is not to tie it to single-input filters).

[reed1, 2012-09-11 15:00:45]

I actually have been thinking and drawing pictures about the DAG problem, and I
can imagine a world where we have no DAG knowledge built into SkImageFIlter
itself. Hence my earlier plea for a convo in person in NC to talk about this.

I thought the earlier bool API had the downside of basically giving you
permission to perform a cast later on. I was suggesting/hoping that this
proposal (returning a colorfilter) would not necessitate a cast, and it has the
added benefit of enabling a performance boost on the CPU side as well (if an
imagefilter is really just a colorfilter, I can skip the offscreen all
together).

[Stephen White, 2012-09-11 15:24:25]

On 2012/09/11 15:00:45, reed1 wrote:
> I actually have been thinking and drawing pictures about the DAG problem, and
I
> can imagine a world where we have no DAG knowledge built into SkImageFIlter
> itself. Hence my earlier plea for a convo in person in NC to talk about this.

I'll be there next week, and we can chat about it then.

> I thought the earlier bool API had the downside of basically giving you
> permission to perform a cast later on. I was suggesting/hoping that this
> proposal (returning a colorfilter) would not necessitate a cast, and it has
the
> added benefit of enabling a performance boost on the CPU side as well (if an
> imagefilter is really just a colorfilter, I can skip the offscreen all
> together).

It's a good goal.  I'm just not sure how we can achieve it without being able to
process the upstream input filter as well.  E.g.,  blur -> colorfilter ->
colorfilter.  If you want to collapse the two colorfilters, you need to be able
to grab and process the input blur to apply the concatenated matrix to it.

As far as the generalized DAG, we're eventually going to want to traverse the
graph for other reasons, such as to invalidate temporary results.  If we don't
generalize the DAG, each subclass which maintains its own inputs (currently
SkSingleInputImageFilter, SkBlendImageFilter, SkComposeImageFilter,
SkMergeImageFilter) will have to have specialized versions of those traversals. 
Not a huge deal, but it will mean more code.  Note that the WebKit
implementation of SVG FilterEffects has a generalized DAG: 
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/...

(see m_inputEffects, inputEffect(), numberOfEffectInputs()).

[Stephen White, 2012-10-11 17:12:04]

This version uses the new DAG infrastructure (which unfortunately I had to make
public within SkImageFilter -- oh well).  This allows us to use an
asColorFilter() virtual on the base class, as Mike had wanted.

[reed1, 2012-10-11 18:04:18]

https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.cpp
File gm/colorfilterimagefilter.cpp (right):

https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.c...
gm/colorfilterimagefilter.cpp:62: void drawClippedRect(SkCanvas* canvas, const
SkRect& r, const SkPaint& paint) {
ugh. do we really have to call kReplace here? This will make it less useful for
other tests (including clip testing).

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

https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter....
include/core/SkImageFilter.h:117: 
If these have to be public, lets add some dox, and rename a little to better
match skia.

countInputFilters()
getInputFilter() (or inputFilterAt()?)

or, if your only public need is sequential, can we recast this as an iterator?
(ala SkPath)? Just a thought.

https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
src/effects/SkColorFilterImageFilter.cpp:111:
paint.setXfermodeMode(SkXfermode::kSrc_Mode);
setColorFilter(cf)->unref(); instead of the unref later on?

or SkAutoUnref aur(cf); above on line 106?

[Stephen White, 2012-10-12 15:30:49]

Thanks for your review.

https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.cpp
File gm/colorfilterimagefilter.cpp (right):

https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.c...
gm/colorfilterimagefilter.cpp:62: void drawClippedRect(SkCanvas* canvas, const
SkRect& r, const SkPaint& paint) {
On 2012/10/11 18:04:18, reed1 wrote:
> ugh. do we really have to call kReplace here? This will make it less useful
for
> other tests (including clip testing). 

OK, will change to save()/clipRect()/restore().

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

https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter....
include/core/SkImageFilter.h:117: 
On 2012/10/11 18:04:18, reed1 wrote:
> If these have to be public, lets add some dox, and rename a little to better
> match skia.

Done.
> 
> countInputFilters()
> getInputFilter() (or inputFilterAt()?)
> 
> or, if your only public need is sequential, can we recast this as an iterator?
> (ala SkPath)? Just a thought.

https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
src/effects/SkColorFilterImageFilter.cpp:111:
paint.setXfermodeMode(SkXfermode::kSrc_Mode);
On 2012/10/11 18:04:18, reed1 wrote:
> setColorFilter(cf)->unref(); instead of the unref later on?

Done.

> or SkAutoUnref aur(cf); above on line 106?

Can't do that, since it may be newly-allocated (on line 100) and already have a
ref, in which case we want it to go away when the paint does.

[Stephen White, 2012-10-12 15:38:59]

On 2012/10/12 15:30:49, Stephen White wrote:
> Thanks for your review.
> 
>
https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.cpp
> File gm/colorfilterimagefilter.cpp (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.c...
> gm/colorfilterimagefilter.cpp:62: void drawClippedRect(SkCanvas* canvas, const
> SkRect& r, const SkPaint& paint) {
> On 2012/10/11 18:04:18, reed1 wrote:
> > ugh. do we really have to call kReplace here? This will make it less useful
> for
> > other tests (including clip testing). 
> 
> OK, will change to save()/clipRect()/restore().
> 
> https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter....
> include/core/SkImageFilter.h:117: 
> On 2012/10/11 18:04:18, reed1 wrote:
> > If these have to be public, lets add some dox, and rename a little to better
> > match skia.
> 
> Done.
> > 
> > countInputFilters()
> > getInputFilter() (or inputFilterAt()?)
> > 
> > or, if your only public need is sequential, can we recast this as an
iterator?
> > (ala SkPath)? Just a thought.
> 
>
https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
> File src/effects/SkColorFilterImageFilter.cpp (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
> src/effects/SkColorFilterImageFilter.cpp:111:
> paint.setXfermodeMode(SkXfermode::kSrc_Mode);
> On 2012/10/11 18:04:18, reed1 wrote:
> > setColorFilter(cf)->unref(); instead of the unref later on?
> 
> Done.
> 
> > or SkAutoUnref aur(cf); above on line 106?
> 
> Can't do that, since it may be newly-allocated (on line 100) and already have
a
> ref, in which case we want it to go away when the paint does.

Never mind, that does work (I forgot the semantics for a minute there).

[Stephen White, 2012-10-12 15:39:00]

On 2012/10/12 15:30:49, Stephen White wrote:
> Thanks for your review.
> 
>
https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.cpp
> File gm/colorfilterimagefilter.cpp (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/gm/colorfilterimagefilter.c...
> gm/colorfilterimagefilter.cpp:62: void drawClippedRect(SkCanvas* canvas, const
> SkRect& r, const SkPaint& paint) {
> On 2012/10/11 18:04:18, reed1 wrote:
> > ugh. do we really have to call kReplace here? This will make it less useful
> for
> > other tests (including clip testing). 
> 
> OK, will change to save()/clipRect()/restore().
> 
> https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/include/core/SkImageFilter....
> include/core/SkImageFilter.h:117: 
> On 2012/10/11 18:04:18, reed1 wrote:
> > If these have to be public, lets add some dox, and rename a little to better
> > match skia.
> 
> Done.
> > 
> > countInputFilters()
> > getInputFilter() (or inputFilterAt()?)
> > 
> > or, if your only public need is sequential, can we recast this as an
iterator?
> > (ala SkPath)? Just a thought.
> 
>
https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
> File src/effects/SkColorFilterImageFilter.cpp (right):
> 
>
https://codereview.appspot.com/6489054/diff/19002/src/effects/SkColorFilterIm...
> src/effects/SkColorFilterImageFilter.cpp:111:
> paint.setXfermodeMode(SkXfermode::kSrc_Mode);
> On 2012/10/11 18:04:18, reed1 wrote:
> > setColorFilter(cf)->unref(); instead of the unref later on?
> 
> Done.
> 
> > or SkAutoUnref aur(cf); above on line 106?
> 
> Can't do that, since it may be newly-allocated (on line 100) and already have
a
> ref, in which case we want it to go away when the paint does.

Never mind, that does work (I forgot the semantics for a minute there).

[reed1, 2012-10-12 16:17:10]

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

https://codereview.appspot.com/6489054/diff/13008/include/core/SkImageFilter....
include/core/SkImageFilter.h:127: */
trivial nit: often in skia we will make the inline have CRs if there is more
than one statement, just for readability.

e.g.

getInput() {
    assert
    return ...
}

Not critical.

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkColorFilterIm...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkColorFilterIm...
src/effects/SkColorFilterImageFilter.cpp:99: }
SkNEW_ARGS(SkColorMatrixFilter, (colorMatrix))

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkTestImageFilt...
File src/effects/SkTestImageFilters.cpp (right):

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkTestImageFilt...
src/effects/SkTestImageFilters.cpp:101:
///////////////////////////////////////////////////////////////////////////////
minor, and not specific to this CL, but its icky to see a function call in the
middle of the for-loop (even tho you and I know its a trivial getter). Clearer
might be to pull the count outside of the loop.

[Stephen White, 2012-10-12 17:13:13]

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

https://codereview.appspot.com/6489054/diff/13008/include/core/SkImageFilter....
include/core/SkImageFilter.h:127: */
On 2012/10/12 16:17:10, reed1 wrote:
> trivial nit: often in skia we will make the inline have CRs if there is more
> than one statement, just for readability.
> 
> e.g.
> 
> getInput() {
>     assert
>     return ...
> }
> 
> Not critical.

Done.

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkColorFilterIm...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkColorFilterIm...
src/effects/SkColorFilterImageFilter.cpp:99: }
On 2012/10/12 16:17:10, reed1 wrote:
> SkNEW_ARGS(SkColorMatrixFilter, (colorMatrix))

Done.

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkTestImageFilt...
File src/effects/SkTestImageFilters.cpp (right):

https://codereview.appspot.com/6489054/diff/13008/src/effects/SkTestImageFilt...
src/effects/SkTestImageFilters.cpp:101:
///////////////////////////////////////////////////////////////////////////////
On 2012/10/12 16:17:10, reed1 wrote:
> minor, and not specific to this CL, but its icky to see a function call in the
> middle of the for-loop (even tho you and I know its a trivial getter). Clearer
> might be to pull the count outside of the loop.

Done.

[reed1, 2012-10-12 17:19:30]

lgtm