Remove asAFoo() functions in SkImageFilter

Replace the asAFoo() functions in SkImageFilter with canFilterImageGPU() and onFilterImageGPU() virtuals.  This allows each filter to implement its own GPU processing code, even for multi-pass filters.

This is the first stage; the next step will be to move the actual processing code from GrContext into the filter subclasses.

[bsalomon, 2012-08-01 15:41:39]

I love the direction this is going in.

http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp#newcode1513
src/gpu/SkGpuDevice.cpp:1513: if (filter->canFilterImageGPU()) {
Which should we prefer, asNewCustomStage or canFilterImageGPU? Are there any
cases where we'd prefer the former because we can tack on extra stuff to the
draw to save on intermediate draws?

[TomH, 2012-08-01 15:42:56]

On 2012/08/01 15:41:39, bsalomon wrote:
> I love the direction this is going in.
> 
> http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp#newcode1513
> src/gpu/SkGpuDevice.cpp:1513: if (filter->canFilterImageGPU()) {
> Which should we prefer, asNewCustomStage or canFilterImageGPU? Are there any
> cases where we'd prefer the former because we can tack on extra stuff to the
> draw to save on intermediate draws?

I like the idea of having the separate bool, but this is certainly worth
discussing since it flies in the face of the current convention. Brian, can you
point us again to the place where combining them is a significant savings?

[Stephen White, 2012-08-01 15:51:50]

On 2012/08/01 15:41:39, bsalomon wrote:
> I love the direction this is going in.
> 
> http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp#newcode1513
> src/gpu/SkGpuDevice.cpp:1513: if (filter->canFilterImageGPU()) {
> Which should we prefer, asNewCustomStage or canFilterImageGPU? Are there any
> cases where we'd prefer the former because we can tack on extra stuff to the
> draw to save on intermediate draws?

If you only have a single pass, doing asNewCustomStage is less work for the
implementor, since it means you don't have to write any texture-handling code
(just the shaders).

Another idea I had was to make the base class implementation of
onFilterImageGPU() call asNewCustomStage(), and if it has one, do that directly.
 That would make the SkGpuDevice code cleaner, since it only has one case to
worry about.  It might preclude combining multiple stages later, but we could
revisit it at that time.

[bsalomon, 2012-08-01 15:52:13]

On 2012/08/01 15:42:56, TomH wrote:
> On 2012/08/01 15:41:39, bsalomon wrote:
> > I love the direction this is going in.
> > 
> > http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp
> > File src/gpu/SkGpuDevice.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6449075/diff/1/src/gpu/SkGpuDevice.cpp#newcode1513
> > src/gpu/SkGpuDevice.cpp:1513: if (filter->canFilterImageGPU()) {
> > Which should we prefer, asNewCustomStage or canFilterImageGPU? Are there any
> > cases where we'd prefer the former because we can tack on extra stuff to the
> > draw to save on intermediate draws?
> 
> I like the idea of having the separate bool, but this is certainly worth
> discussing since it flies in the face of the current convention. Brian, can
you
> point us again to the place where combining them is a significant savings?

I dunno if such a case exists, that's why I asked the question :)  The
high-level application of image filters by SkCanvas and WK is a bit murky to me.

[Stephen White, 2012-08-01 16:21:09]

On 2012/08/01 15:52:13, bsalomon wrote:
> On 2012/08/01 15:42:56, TomH wrote:
> > I like the idea of having the separate bool, but this is certainly worth
> > discussing since it flies in the face of the current convention. Brian, can
> you
> > point us again to the place where combining them is a significant savings?
> 
> I dunno if such a case exists, that's why I asked the question :)  The
> high-level application of image filters by SkCanvas and WK is a bit murky to
me.

We currently don't combine individual custom filter stages anywhere in WebKit or
SkCanvas.  They're always applied one at a time, on an SkBitmap.  We actually
can't do this in many places, at least naively, due to colour clipping semantics
(e.g., color matrix) or image clipping.

[Stephen White, 2012-08-01 17:29:33]

On 2012/08/01 15:51:50, Stephen White wrote:
> Another idea I had was to make the base class implementation of
> onFilterImageGPU() call asNewCustomStage(), and if it has one, do that
directly.

OK, so I tried this out, but since it needs to use GrContext and friends, it
required moving SkImageFilter.cpp to src/effects, while 
leaving SkImageFilter.h in include/core.  This seems pretty wrong.

Since it didn't allow me to do the cleanup I'd hoped to do anyway, I think I'll
go back to this version of the patch.

[bsalomon, 2012-08-01 17:37:40]

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

http://codereview.appspot.com/6449075/diff/1/include/core/SkImageFilter.h#new...
include/core/SkImageFilter.h:95: // Default impl returns NULL
Probably should have some commenton what rect is and how it relates to the input
/ output textures. Must the output texture have rect w/h dimensions?

[Stephen White, 2012-08-01 17:42:18]

Fix comments, remove unused var

[Stephen White, 2012-08-01 17:43:02]

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

http://codereview.appspot.com/6449075/diff/1/include/core/SkImageFilter.h#new...
include/core/SkImageFilter.h:95: // Default impl returns NULL
On 2012/08/01 17:37:41, bsalomon wrote:
> Probably should have some commenton what rect is and how it relates to the
input
> / output textures. Must the output texture have rect w/h dimensions?

Done.

[TomH, 2012-08-01 17:55:15]

http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
File include/effects/SkMorphologyImageFilter.h (right):

http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
include/effects/SkMorphologyImageFilter.h:36: virtual GrTexture*
onFilterImageGPU(GrTexture* src, const SkRect& rect);
+SK_OVERRIDE throughout this file

[Stephen White, 2012-08-01 17:59:18]

Add SK_OVERRIDE where possible

[Stephen White, 2012-08-01 17:59:38]

http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
File include/effects/SkMorphologyImageFilter.h (right):

http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
include/effects/SkMorphologyImageFilter.h:36: virtual GrTexture*
onFilterImageGPU(GrTexture* src, const SkRect& rect);
On 2012/08/01 17:55:15, TomH wrote:
> +SK_OVERRIDE throughout this file

Done.

[bsalomon, 2012-08-01 18:02:31]

On 2012/08/01 17:59:38, Stephen White wrote:
>
http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
> File include/effects/SkMorphologyImageFilter.h (right):
> 
>
http://codereview.appspot.com/6449075/diff/10001/include/effects/SkMorphology...
> include/effects/SkMorphologyImageFilter.h:36: virtual GrTexture*
> onFilterImageGPU(GrTexture* src, const SkRect& rect);
> On 2012/08/01 17:55:15, TomH wrote:
> > +SK_OVERRIDE throughout this file
> 
> Done.

LGTM

[reed1, 2012-08-01 19:29:38]

I can't speak for the long-term vision, but I sure like removing virtuals that
begin with asA...

lgtm

[Stephen White, 2012-08-01 20:16:52]

Landed with r4900.  Closing.