Allow single-pass filters (which use asNewEffect()) to participate in the image filter DAG. This w…

Allow single-pass filters (which use asNewEffect()) to participate in the image filter DAG.  This was done by implementing support for asNewEffect() in the default implementations of SkImageFilter::canFilterImageGPU() and SkImageFilter::filteImageGPU().   The derived class still only needs to asNewEffect().  This also allows us to remove any knowledge of single-pass image filters from SkGpuDevice.

BUG=

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

[Stephen White, 2013-04-07 20:30:03]

PTAL

[bsalomon, 2013-04-08 15:09:55]

On 2013/04/07 20:30:03, Stephen White wrote:
> PTAL

Is it possible to do this without the new base class? What if asNewEffect was a
private virtual and the default implementation of filterImageGPU() in
SkImageFilter was roughly what you have in SkSinglePassImageFilter (with a NULL
check for asNewEffect())? Then effects could just override either asNewEffect()
or filterImageGpu() like they do today. And we could still remove the calling
code for asNewEffect from SkGpuDevice.

[Stephen White, 2013-04-08 15:13:51]

On 2013/04/08 15:09:55, bsalomon wrote:
> On 2013/04/07 20:30:03, Stephen White wrote:
> > PTAL
> 
> Is it possible to do this without the new base class? What if asNewEffect was
a
> private virtual and the default implementation of filterImageGPU() in
> SkImageFilter was roughly what you have in SkSinglePassImageFilter (with a
NULL
> check for asNewEffect())? Then effects could just override either
asNewEffect()
> or filterImageGpu() like they do today. And we could still remove the calling
> code for asNewEffect from SkGpuDevice.

It's a good idea, and I tried it before, but it required putting circular
dependencies between gpu/ and core/.  Now that (I think) those dependencies have
been added anyway, I'll try it again.

[Stephen White, 2013-04-08 15:18:27]

On 2013/04/08 15:13:51, Stephen White wrote:
> On 2013/04/08 15:09:55, bsalomon wrote:
> > On 2013/04/07 20:30:03, Stephen White wrote:
> > > PTAL
> > 
> > Is it possible to do this without the new base class? What if asNewEffect
was
> a
> > private virtual and the default implementation of filterImageGPU() in
> > SkImageFilter was roughly what you have in SkSinglePassImageFilter (with a
> NULL
> > check for asNewEffect())? Then effects could just override either
> asNewEffect()
> > or filterImageGpu() like they do today. And we could still remove the
calling
> > code for asNewEffect from SkGpuDevice.
> 
> It's a good idea, and I tried it before, but it required putting circular
> dependencies between gpu/ and core/.  Now that (I think) those dependencies
have
> been added anyway, I'll try it again.

Oh, one other thing:  I'll either have to restore the old version of
asNewEffect() in order to allow canFilterImageGPU() to work automatically for
effect-based filters, or leave it up to the effect-based subclasses to remember
to implement canFilterImageGPU() themselves.  WDYT?

[bsalomon, 2013-04-08 15:28:02]

On 2013/04/08 15:18:27, Stephen White wrote:
> On 2013/04/08 15:13:51, Stephen White wrote:
> > On 2013/04/08 15:09:55, bsalomon wrote:
> > > On 2013/04/07 20:30:03, Stephen White wrote:
> > > > PTAL
> > > 
> > > Is it possible to do this without the new base class? What if asNewEffect
> was
> > a
> > > private virtual and the default implementation of filterImageGPU() in
> > > SkImageFilter was roughly what you have in SkSinglePassImageFilter (with a
> > NULL
> > > check for asNewEffect())? Then effects could just override either
> > asNewEffect()
> > > or filterImageGpu() like they do today. And we could still remove the
> calling
> > > code for asNewEffect from SkGpuDevice.
> > 
> > It's a good idea, and I tried it before, but it required putting circular
> > dependencies between gpu/ and core/.  Now that (I think) those dependencies
> have
> > been added anyway, I'll try it again.
> 
> Oh, one other thing:  I'll either have to restore the old version of
> asNewEffect() in order to allow canFilterImageGPU() to work automatically for
> effect-based filters, or leave it up to the effect-based subclasses to
remember
> to implement canFilterImageGPU() themselves.  WDYT?

When you say the old version, do you mean the old sig? That seems OK to me.

While we're at it, what do you think of eliminating canFilterImageGPU() and
making filterImageGPU() work like asNewEffect()? I mean that NULL inputs would
indicate a query for support. I only ask because it seems like it would make it
more difficult for a subclass to accidentally wind up with inconsistent virtual
function implementations.

[Stephen White, 2013-04-08 15:38:45]

On 2013/04/08 15:28:02, bsalomon wrote:
> On 2013/04/08 15:18:27, Stephen White wrote:
> > On 2013/04/08 15:13:51, Stephen White wrote:
> > > On 2013/04/08 15:09:55, bsalomon wrote:
> > > > On 2013/04/07 20:30:03, Stephen White wrote:
> > > > > PTAL
> > > > 
> > > > Is it possible to do this without the new base class? What if
asNewEffect
> > was
> > > a
> > > > private virtual and the default implementation of filterImageGPU() in
> > > > SkImageFilter was roughly what you have in SkSinglePassImageFilter (with
a
> > > NULL
> > > > check for asNewEffect())? Then effects could just override either
> > > asNewEffect()
> > > > or filterImageGpu() like they do today. And we could still remove the
> > calling
> > > > code for asNewEffect from SkGpuDevice.
> > > 
> > > It's a good idea, and I tried it before, but it required putting circular
> > > dependencies between gpu/ and core/.  Now that (I think) those
dependencies
> > have
> > > been added anyway, I'll try it again.
> > 
> > Oh, one other thing:  I'll either have to restore the old version of
> > asNewEffect() in order to allow canFilterImageGPU() to work automatically
for
> > effect-based filters, or leave it up to the effect-based subclasses to
> remember
> > to implement canFilterImageGPU() themselves.  WDYT?
> 
> When you say the old version, do you mean the old sig? That seems OK to me.

Yeah, the old signature.  I kind of dislike the ** outparam, and the fact that
it's different from other implementations of asNewEffect (e.g., SkColorFilter).

> While we're at it, what do you think of eliminating canFilterImageGPU() and
> making filterImageGPU() work like asNewEffect()? I mean that NULL inputs would
> indicate a query for support. I only ask because it seems like it would make
it
> more difficult for a subclass to accidentally wind up with inconsistent
virtual
> function implementations.

I don't think that's a huge issue -- I've never run into it. And that ugly
outparam (and the conditionals in every implementation of filterImageGPU()
required to check it) are a high price to pay, IMHO.

[Stephen White, 2013-04-08 16:32:02]

OK, I've removed SkSinglePassImageFilter, and moved its guts to the
SkImageFilter::filterImageGPU() default impl.  I've left asNewEffect() with the
new signature, so subclasses should return true for canFilterImageGPU() if they
implement either asNewEffect() or filterImageGPU().

[bsalomon, 2013-04-08 17:52:17]

Personally I think the old signature is easier on a subclass developer because
they only one virtual must be overridden and there isn't a consistency issue.
But I don't feel very strongly about it. The new xfer mode virtual follows the
pointer=NULL is a query pattern.

https://codereview.chromium.org/13602013/diff/11001/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/13602013/diff/11001/include/core/SkImageFilte...
include/core/SkImageFilter.h:101: *  rendered to textures.  For single-pass
effects, use asNewEffect().
It seems like this comment should be updated to say that the caller must
override this if they override asNewEffect().

https://codereview.chromium.org/13602013/diff/11001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/13602013/diff/11001/src/core/SkImageFilter.cp...
src/core/SkImageFilter.cpp:138: SkAutoTUnref<GrEffectRef>
effect(asNewEffect(srcTexture));
this->asNewEffect()

[Stephen White, 2013-04-08 18:53:35]

On 2013/04/08 17:52:17, bsalomon wrote:
> Personally I think the old signature is easier on a subclass developer because
> they only one virtual must be overridden and there isn't a consistency issue.
> But I don't feel very strongly about it. The new xfer mode virtual follows the
> pointer=NULL is a query pattern.

OK, since I won't be alone in my ugliness, I've put it back to the old
signature.  :)

https://codereview.chromium.org/13602013/diff/11001/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/13602013/diff/11001/include/core/SkImageFilte...
> include/core/SkImageFilter.h:101: *  rendered to textures.  For single-pass
> effects, use asNewEffect().
> It seems like this comment should be updated to say that the caller must
> override this if they override asNewEffect().
> 
> https://codereview.chromium.org/13602013/diff/11001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/13602013/diff/11001/src/core/SkImageFilter.cp...
> src/core/SkImageFilter.cpp:138: SkAutoTUnref<GrEffectRef>
> effect(asNewEffect(srcTexture));
> this->asNewEffect()

[bsalomon, 2013-04-08 19:38:29]

lgtm with a some trivial nits

https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
src/core/SkImageFilter.cpp:112: return asNewEffect(NULL, NULL);
this->

https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
src/core/SkImageFilter.cpp:119: if
(!SkImageFilterUtils::GetInputResultGPU(getInput(0), proxy, src, &input)) {
this->

https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
src/core/SkImageFilter.cpp:139: asNewEffect(&effect, srcTexture);
this->

[Stephen White, 2013-04-08 19:43:33]

Committed patchset #10 manually as r8563 (presubmit successful).

[Stephen White, 2013-04-08 19:43:52]

On 2013/04/08 19:38:29, bsalomon wrote:
> lgtm with a some trivial nits

Thanks for the review!  Nits fixed on landing.

https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
> src/core/SkImageFilter.cpp:112: return asNewEffect(NULL, NULL);
> this->
> 
>
https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
> src/core/SkImageFilter.cpp:119: if
> (!SkImageFilterUtils::GetInputResultGPU(getInput(0), proxy, src, &input)) {
> this->
> 
>
https://codereview.chromium.org/13602013/diff/17003/src/core/SkImageFilter.cp...
> src/core/SkImageFilter.cpp:139: asNewEffect(&effect, srcTexture);
> this->