Change filterImageGPU() to use SkBitmap

This changes the signature of SkImageFilter::filterImageGPU() to use SkBitmaps for input and output, and removes the rect param.  This allows us to return textures which are larger than the actual result, such as when GrAutoScratchTextures are used.  The SkBitmap's size represents the active region, while the GrTexture's size is the full texture size.  

This fixes the bicubic image filter GM on the GPU, which otherwise draws garbage outside the filtered region.  It also moves us closer to unifying the signatures of SkImageFilter::onFilterImage() and SkImageFilter::filterImageGPU().

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

[Stephen White, 2013-01-22 23:20:52]

Alternate version.  Take a look if you like, but I think I've convinced myself
this way is worse.

[bsalomon, 2013-01-23 15:09:43]

On 2013/01/22 23:20:52, Stephen White wrote:
> Alternate version.  Take a look if you like, but I think I've convinced myself
> this way is worse.

The bitmap version does seem pretty nice. A few comments:

The comment in SkImageFilter.h needs updating

To match the non-GPU version perhaps the result bitmap should be a SkBitmap*
param rather than a return value. I don't think we usually return structs.

SkGrTexturePixelRef.h, SkGrTexturePixelRef, and SkGrRenderTargetPixelRef are
deprecated. Use SkGrPixelRef in SkGrPixelRef.h

Maybe two helpers, possibly could be declared in SkGrPixelRef

GrMakeTextureBitmap(const SkBitmap& rasterBmp, GrContext* context, SkBitmap*
result);

GrMakeTextureBitmap(GrTexture* texture, const SkIRect& subrect, GrContext*
context, SkBitmap* result);

(not necessarily attached to those names and sigs may not be quite right)

[Stephen White, 2013-01-23 16:14:14]

On 2013/01/23 15:09:43, bsalomon wrote:
> The bitmap version does seem pretty nice. A few comments:
> 
> The comment in SkImageFilter.h needs updating

Done.

> 
> To match the non-GPU version perhaps the result bitmap should be a SkBitmap*
> param rather than a return value. I don't think we usually return structs.

Please, let's allow returning a struct in this case.  It's lightweight due to
the pixelref, it's much cleaner at the call sites, and all modern compilers
implement the return value optimization, so there's no performance advantage to
passing a pointer.

> SkGrTexturePixelRef.h, SkGrTexturePixelRef, and SkGrRenderTargetPixelRef are
> deprecated. Use SkGrPixelRef in SkGrPixelRef.h
> 
> Maybe two helpers, possibly could be declared in SkGrPixelRef
> 
> GrMakeTextureBitmap(const SkBitmap& rasterBmp, GrContext* context, SkBitmap*
> result);
> 
> GrMakeTextureBitmap(GrTexture* texture, const SkIRect& subrect, GrContext*
> context, SkBitmap* result);
> 
> (not necessarily attached to those names and sigs may not be quite right)

I've put the necessary function in SkImageFilterUtils for now, as well as
refactoring the various versions of getInputResultAsTexture (now
GetInputResultGPU).  I may actually be able to move them into a base class for
GPU-capable filters, but I'd prefer to do that in another patch.

[reed1, 2013-01-23 16:36:15]

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

https://codereview.appspot.com/7180048/diff/26001/include/core/SkImageFilter....
include/core/SkImageFilter.h:110: */
Lets just take a dst-bitmap as an argument, since that is Skia's API pattern.

e.g.     virtual bool onFilterImage(Proxy*, const SkBitmap& src, const
SkMatrix&,
                               SkBitmap* result, SkIPoint* offset);

If we want to measure and debate return-value-optimizations, that's fine (sort
of), but doing it piece-meal in one CL is probably not the right place (or
time).

[bsalomon, 2013-01-23 16:39:43]

On 2013/01/23 16:14:14, Stephen White wrote:
> On 2013/01/23 15:09:43, bsalomon wrote:
> > The bitmap version does seem pretty nice. A few comments:
> > 
> > The comment in SkImageFilter.h needs updating
> 
> Done.
> 
> > 
> > To match the non-GPU version perhaps the result bitmap should be a SkBitmap*
> > param rather than a return value. I don't think we usually return structs.
> 
> Please, let's allow returning a struct in this case.  It's lightweight due to
> the pixelref, it's much cleaner at the call sites, and all modern compilers
> implement the return value optimization, so there's no performance advantage
to
> passing a pointer.
> 
> > SkGrTexturePixelRef.h, SkGrTexturePixelRef, and SkGrRenderTargetPixelRef are
> > deprecated. Use SkGrPixelRef in SkGrPixelRef.h
> > 
> > Maybe two helpers, possibly could be declared in SkGrPixelRef
> > 
> > GrMakeTextureBitmap(const SkBitmap& rasterBmp, GrContext* context, SkBitmap*
> > result);
> > 
> > GrMakeTextureBitmap(GrTexture* texture, const SkIRect& subrect, GrContext*
> > context, SkBitmap* result);
> > 
> > (not necessarily attached to those names and sigs may not be quite right)
> 
> I've put the necessary function in SkImageFilterUtils for now, as well as
> refactoring the various versions of getInputResultAsTexture (now
> GetInputResultGPU).  I may actually be able to move them into a base class for
> GPU-capable filters, but I'd prefer to do that in another patch.

Nice, that makes the code a lot more readable.

[bsalomon, 2013-01-23 16:49:54]

https://codereview.appspot.com/7180048/diff/14001/src/effects/SkImageFilterUt...
File src/effects/SkImageFilterUtils.cpp (right):

https://codereview.appspot.com/7180048/diff/14001/src/effects/SkImageFilterUt...
src/effects/SkImageFilterUtils.cpp:35: SkSafeRef(resultTex);
Is there a leak here? The pixel ref takes a ref to the texture right?

[Stephen White, 2013-01-23 16:51:11]

On 2013/01/23 16:36:15, reed1 wrote:
> https://codereview.appspot.com/7180048/diff/26001/include/core/SkImageFilter.h
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.appspot.com/7180048/diff/26001/include/core/SkImageFilter....
> include/core/SkImageFilter.h:110: */
> Lets just take a dst-bitmap as an argument, since that is Skia's API pattern.
> 
> e.g.     virtual bool onFilterImage(Proxy*, const SkBitmap& src, const
> SkMatrix&,
>                                SkBitmap* result, SkIPoint* offset);
> 
> If we want to measure and debate return-value-optimizations, that's fine (sort
> of), but doing it piece-meal in one CL is probably not the right place (or
> time).

You're right that this CL is the wrong place to debate it.  I'm going to bring
it up on skia-dev and see what people think.

[reed1, 2013-01-23 16:54:59]

sounds good.

I think for this API, losing the bool-return is a loss too, since on other
"effects" apis, we regularly expect them to possibly fail, and the caller can
use that return-bool to then decide what to do (e.g. draw nothing, draw the
"plain" version, call again with a smaller clip/tile, etc.).

[Stephen White, 2013-01-23 16:57:09]

On 2013/01/23 16:54:59, reed1 wrote:
> sounds good.
> 
> I think for this API, losing the bool-return is a loss too, since on other
> "effects" apis, we regularly expect them to possibly fail, and the caller can
> use that return-bool to then decide what to do (e.g. draw nothing, draw the
> "plain" version, call again with a smaller clip/tile, etc.).

I've been using a SkBitmap::isEmpty() for that case.

[reed1, 2013-01-23 16:58:19]

Understood, but that isn't as clear to the caller, and it (possibly) conflates a
failure with a possibly-legal empty result.

[Stephen White, 2013-01-23 22:42:12]

On 2013/01/23 16:49:54, bsalomon wrote:
>
https://codereview.appspot.com/7180048/diff/14001/src/effects/SkImageFilterUt...
> File src/effects/SkImageFilterUtils.cpp (right):
> 
>
https://codereview.appspot.com/7180048/diff/14001/src/effects/SkImageFilterUt...
> src/effects/SkImageFilterUtils.cpp:35: SkSafeRef(resultTex);
> Is there a leak here? The pixel ref takes a ref to the texture right?

Thanks, fixed (here and elsewhere).

[Stephen White, 2013-01-24 22:38:16]

On 2013/01/23 16:58:19, reed1 wrote:
> Understood, but that isn't as clear to the caller, and it (possibly) conflates
a
> failure with a possibly-legal empty result.

What can the filter infrastructure do with a failure?  The only things I can
think of are draw nothing, or draw the unfiltered source, which are both equally
well handled by the callee (return SkBitmap or return src).

[reed1, 2013-01-25 13:25:48]

Lets not remove the choice from the caller. Please return a bool and let the
caller decide. Skia behaves this way already, so lets be consistent (and
flexible, since we are a system-level API).

[Stephen White, 2013-01-30 03:19:07]

On 2013/01/25 13:25:48, reed1 wrote:
> Lets not remove the choice from the caller. Please return a bool and let the
> caller decide. Skia behaves this way already, so lets be consistent (and
> flexible, since we are a system-level API).

Done.

[bsalomon, 2013-01-30 13:28:24]

lgtm

[reed1, 2013-01-30 13:52:34]

lgtm w/ comment nit

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

https://codereview.appspot.com/7180048/diff/33005/include/core/SkImageFilter....
include/core/SkImageFilter.h:109: *  implementation returns an empty SkBitmap.
Comment should read: returns false and ignores result parameter.