Cleanup patch to move all of SkImageFilterUtils into SkImageFilter.

Cleanup patch to move all of SkImageFilterUtils into SkImageFilter.

This was a utility class that dates from before GPU code was allowed
in core. Now that it is, there's no reason not to have this
functionality in SkImageFilter.

Covered by existing tests.

R=reed@google.com

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

[Stephen White, 2014-03-03 21:58:54]

PTAL

[reed1, 2014-03-03 22:07:04]

lgtm w/ nit

https://codereview.chromium.org/185973003/diff/160001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/185973003/diff/160001/include/core/SkImageFil...
include/core/SkImageFilter.h:153: static void wrapTexture(GrTexture* texture,
int width, int height, SkBitmap* result);
nit: Capitalization: WrapTexture

[Stephen White, 2014-03-03 22:09:50]

https://codereview.chromium.org/185973003/diff/160001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/185973003/diff/160001/include/core/SkImageFil...
include/core/SkImageFilter.h:153: static void wrapTexture(GrTexture* texture,
int width, int height, SkBitmap* result);
On 2014/03/03 22:07:04, reed1 wrote:
> nit: Capitalization: WrapTexture

Done.

[Stephen White, 2014-03-03 22:14:04]

Committed patchset #12 manually as r13646 (presubmit successful).

[reed1, 2014-03-04 21:03:26]

Part of this was to move WrapTexture into SkImageFilter.h

Two comments:

1. Can we make it protected instead of public, to be defensive (as it appears to
me that only subclasses need it..?)

2. How can we evolve ImageFilters in general off of the notion that SkBitmap may
in-fact be wrapping a GrTexture? Our long-term plan is to break that dualism,
and it might be instructive to explore the question just in the context of
ImageFilters...

[Stephen White, 2014-03-04 21:17:02]

On 2014/03/04 21:03:26, reed1 wrote:
> Part of this was to move WrapTexture into SkImageFilter.h
> 
> Two comments:
> 
> 1. Can we make it protected instead of public, to be defensive (as it appears
to
> me that only subclasses need it..?)

SkMorphology needed it to be public, since it calls it from a file-static
function.
I could make that function part of the SkMorphologyImageFilter, but that exposes
a 
bunch of Ganesh stuff into SkMorphologyImageFilter.h.

(Note that there's nothing SkImageFilter-specific about WrapTexture(); it could
go 
anywhere, really.)

> 2. How can we evolve ImageFilters in general off of the notion that SkBitmap
may
> in-fact be wrapping a GrTexture? Our long-term plan is to break that dualism,
> and it might be instructive to explore the question just in the context of
> ImageFilters...

Possibly doable, but the GPU implementations are always going to need some way
to
get at the textures, in order to read from and render to. I don't have a strong
preference
about how that happens. Note that filterImageGPU() used to deal directly with
GrTextures, but 
this became unwieldy with Skia lack of real smart pointers.

One thing I was thinking about was putting the guts of getInputResultGPU() into
the base 
class implementation of filterImageGPU(), and just getting rid of
canFilterImageGPU() and 
getInputResultGPU() entirely. Then if you don't have a GPU implementation, we
just call 
onFilterImage() and upload it to a texture. This is essentially what happens
anyway, just in a 
more roundabout way.

[bsalomon, 2014-03-04 21:24:22]

On 2014/03/04 21:17:02, Stephen White wrote:
> On 2014/03/04 21:03:26, reed1 wrote:
> > Part of this was to move WrapTexture into SkImageFilter.h
> > 
> > Two comments:
> > 
> > 1. Can we make it protected instead of public, to be defensive (as it
appears
> to
> > me that only subclasses need it..?)
> 
> SkMorphology needed it to be public, since it calls it from a file-static
> function.
> I could make that function part of the SkMorphologyImageFilter, but that
exposes
> a 
> bunch of Ganesh stuff into SkMorphologyImageFilter.h.
> 
> (Note that there's nothing SkImageFilter-specific about WrapTexture(); it
could
> go 
> anywhere, really.)
> 
> > 2. How can we evolve ImageFilters in general off of the notion that SkBitmap
> may
> > in-fact be wrapping a GrTexture? Our long-term plan is to break that
dualism,
> > and it might be instructive to explore the question just in the context of
> > ImageFilters...
> 
> Possibly doable, but the GPU implementations are always going to need some way
> to
> get at the textures, in order to read from and render to. I don't have a
strong
> preference
> about how that happens. Note that filterImageGPU() used to deal directly with
> GrTextures, but 
> this became unwieldy with Skia lack of real smart pointers.
> 
> One thing I was thinking about was putting the guts of getInputResultGPU()
into
> the base 
> class implementation of filterImageGPU(), and just getting rid of
> canFilterImageGPU() and 
> getInputResultGPU() entirely. Then if you don't have a GPU implementation, we
> just call 
> onFilterImage() and upload it to a texture. This is essentially what happens
> anyway, just in a 
> more roundabout way.

Would it help to have image filters operate on... get ready for it... images?
(As in SkImage.) The problem with SkBitmap-backed textures is that we bend over
backwards to make the SkBitmap API work for textures, but even then there are
still behavior differences and ambiguities. The bitmap API was really designed
for pixels in cpu-accessible memory and we'd like to roll back the
generalization of SkBitmap to other backing stores. The limited API of SkImage
is much more acceptable.

[Stephen White, 2014-03-04 21:32:13]

On 2014/03/04 21:24:22, bsalomon wrote:
> On 2014/03/04 21:17:02, Stephen White wrote:
> > On 2014/03/04 21:03:26, reed1 wrote:
> > > Part of this was to move WrapTexture into SkImageFilter.h
> > > 
> > > Two comments:
> > > 
> > > 1. Can we make it protected instead of public, to be defensive (as it
> appears
> > to
> > > me that only subclasses need it..?)
> > 
> > SkMorphology needed it to be public, since it calls it from a file-static
> > function.
> > I could make that function part of the SkMorphologyImageFilter, but that
> exposes
> > a 
> > bunch of Ganesh stuff into SkMorphologyImageFilter.h.
> > 
> > (Note that there's nothing SkImageFilter-specific about WrapTexture(); it
> could
> > go 
> > anywhere, really.)
> > 
> > > 2. How can we evolve ImageFilters in general off of the notion that
SkBitmap
> > may
> > > in-fact be wrapping a GrTexture? Our long-term plan is to break that
> dualism,
> > > and it might be instructive to explore the question just in the context of
> > > ImageFilters...
> > 
> > Possibly doable, but the GPU implementations are always going to need some
way
> > to
> > get at the textures, in order to read from and render to. I don't have a
> strong
> > preference
> > about how that happens. Note that filterImageGPU() used to deal directly
with
> > GrTextures, but 
> > this became unwieldy with Skia lack of real smart pointers.
> > 
> > One thing I was thinking about was putting the guts of getInputResultGPU()
> into
> > the base 
> > class implementation of filterImageGPU(), and just getting rid of
> > canFilterImageGPU() and 
> > getInputResultGPU() entirely. Then if you don't have a GPU implementation,
we
> > just call 
> > onFilterImage() and upload it to a texture. This is essentially what happens
> > anyway, just in a 
> > more roundabout way.
> 
> Would it help to have image filters operate on... get ready for it... images?
> (As in SkImage.) The problem with SkBitmap-backed textures is that we bend
over
> backwards to make the SkBitmap API work for textures, but even then there are
> still behavior differences and ambiguities. The bitmap API was really designed
> for pixels in cpu-accessible memory and we'd like to roll back the
> generalization of SkBitmap to other backing stores. The limited API of SkImage
> is much more acceptable.

Yeah, Mike was asking for this before. I'm a little leery of the work of 
switching at this point, just as we're hoping to turn on the Skia DAG in Blink,
but it's definitely something we should look at for the future.