Add canFilterMaskGPU & filterMaskGPU to SkMaskFilter

Clean up the SkMaskFilter's GPU interface so we can talk about it without being embarrassed.

[robertphillips, 2013-06-28 17:46:12]

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter.h
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:73: 
The next two calls mimic the ImageFilter. What do you think about passing in raw
GrTexture*'s rather then SkBitmaps?

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:98: */
What do you think about this signature actually being:

(SkBitmap* src, const SkRect& maskRect, SkBitmap* result, bool okayToRecycleSrc)

?

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp#ne...
src/gpu/SkGpuDevice.cpp:890: 
Just moved 'wrap_texture' up from below.

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp#ne...
src/gpu/SkGpuDevice.cpp:957: pathPtr = &tmpPath;
This omission didn't cause any problems but I think it is clearer this way.

[robertphillips, 2013-06-28 17:58:01]

Also, I thought this was big enough as is so I will move the Gaussian blur code
out of GrContext in a separate CL.

[bsalomon, 2013-06-28 18:32:42]

Overall looks a like a nice step towards having a clearer story for how we allow
arbitrary Sk*'s to interact with the gpu backend.

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter.h
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:73: 
On 2013/06/28 17:46:12, robertphillips wrote:
> The next two calls mimic the ImageFilter. What do you think about passing in
raw
> GrTexture*'s rather then SkBitmaps?

Is it useful to be able to filter CPU bitmaps? Seems like it could be.

Do we save something if it takes a GrTexture?

https://codereview.chromium.org/18110012/diff/2001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:98: */
On 2013/06/28 17:46:12, robertphillips wrote:
> What do you think about this signature actually being:
> 
> (SkBitmap* src, const SkRect& maskRect, SkBitmap* result, bool
okayToRecycleSrc)
> 
> ?

oh dear... why?

https://codereview.chromium.org/18110012/diff/2001/src/device/xps/SkXPSDevice...
File src/device/xps/SkXPSDevice.cpp (left):

https://codereview.chromium.org/18110012/diff/2001/src/device/xps/SkXPSDevice...
src/device/xps/SkXPSDevice.cpp:1545: //TODO: currently ignoring the ppm if blur
ignoring transform.
better check with bungeman

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp#ne...
src/gpu/SkGpuDevice.cpp:792: bool drawWithMaskFilter(GrContext* context, const
SkPath& devPath,
if_were_moving_to_names_that_look_like_this_should_we_rename_this_function()?

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp#ne...
src/gpu/SkGpuDevice.cpp:872: // AA uses the "coverage" stages on GrDrawTarget.
Coverage with a dst
Ohh boy... this sucks. (I realize this has been around for a long time, just
couldn't help myself). We have the "coverage drawing" flag, but only on draw
state...

[robertphillips, 2013-07-01 19:53:57]

This patch changes the signatures of the filterMaskGPU functions.

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/2001/src/gpu/SkGpuDevice.cpp#ne...
src/gpu/SkGpuDevice.cpp:792: bool drawWithMaskFilter(GrContext* context, const
SkPath& devPath,
On 2013/06/28 18:32:42, bsalomon wrote:
> if_were_moving_to_names_that_look_like_this_should_we_rename_this_function()?

Done.

[bsalomon, 2013-07-01 20:01:57]

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter.h
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:97: *  Note: the texture backing 'src' may be
recycled for 'result'
really? That seems dangerous. I was thinking we'd pass false if src is
texture-backed.

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:104: *  This flavor of 'filterMaskGPU' provides a
more direct means of accessing
Document canOverwriteSrc?

https://codereview.chromium.org/18110012/diff/7001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/18110012/diff/7001/src/core/SkMaskFilter.cpp#...
src/core/SkMaskFilter.cpp:286: GrTexture* src = (GrTexture*) srcBM.getTexture();
Shouldn't we create a texture for the bitmap if it doesn't have one?

[robertphillips, 2013-07-02 13:11:58]

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter.h
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:97: *  Note: the texture backing 'src' may be
recycled for 'result'
Done - comment was left over.

https://codereview.chromium.org/18110012/diff/7001/include/core/SkMaskFilter....
include/core/SkMaskFilter.h:104: *  This flavor of 'filterMaskGPU' provides a
more direct means of accessing
On 2013/07/01 20:01:57, bsalomon wrote:
> Document canOverwriteSrc?

Done.

https://codereview.chromium.org/18110012/diff/7001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/18110012/diff/7001/src/core/SkMaskFilter.cpp#...
src/core/SkMaskFilter.cpp:286: GrTexture* src = (GrTexture*) srcBM.getTexture();
Done-ish - I added a TODO. Is that sufficient for now?

[robertphillips, 2013-07-02 19:52:09]

Patch 4 adds alters the interface to the static filterMaskGPU entry point and
adds the dubious capability to filter a CPU-side texture on the GPU.

[Stephen White, 2013-07-02 20:02:05]

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:388: srcRect.inset(SkFloatToScalar(-sigma3),
SkFloatToScalar(-sigma3));
Nit:  could useSkRect::outset() here.

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:389: clipRect.inset(SkFloatToScalar(-sigma3),
SkFloatToScalar(-sigma3));
And here.

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp#n...
src/gpu/SkGpuDevice.cpp:849: // We actually only need A8, but it often isn't
supported as a
This comment is probably stale, since it looks like we try Alpha_8 below where
available.

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp#n...
src/gpu/SkGpuDevice.cpp:891: static SkBitmap wrap_texture(GrTexture* texture) {
Nit: maybe get rid of "static" now that it's in the unnamed namespace?

[Stephen White, 2013-07-02 20:02:52]

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp#n...
src/gpu/SkGpuDevice.cpp:849: // We actually only need A8, but it often isn't
supported as a
On 2013/07/02 20:02:05, Stephen White wrote:
> This comment is probably stale, since it looks like we try Alpha_8 below where
> available.

Never mind; I mis-parsed "often". :)

[robertphillips, 2013-07-03 12:51:16]

This version updates the static filterMaskGPU to use a scratch texture and
addresses the other code review issues.

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:388: srcRect.inset(SkFloatToScalar(-sigma3),
SkFloatToScalar(-sigma3));
On 2013/07/02 20:02:05, Stephen White wrote:
> Nit:  could useSkRect::outset() here.

Done.

https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
src/effects/SkBlurMaskFilter.cpp:389: clipRect.inset(SkFloatToScalar(-sigma3),
SkFloatToScalar(-sigma3));
On 2013/07/02 20:02:05, Stephen White wrote:
> And here.

Done.

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp#n...
src/gpu/SkGpuDevice.cpp:891: static SkBitmap wrap_texture(GrTexture* texture) {
On 2013/07/02 20:02:05, Stephen White wrote:
> Nit: maybe get rid of "static" now that it's in the unnamed namespace?

Done.

[bsalomon, 2013-07-03 13:21:16]

On 2013/07/03 12:51:16, robertphillips wrote:
> This version updates the static filterMaskGPU to use a scratch texture and
> addresses the other code review issues.
> 
>
https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
> File src/effects/SkBlurMaskFilter.cpp (right):
> 
>
https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
> src/effects/SkBlurMaskFilter.cpp:388: srcRect.inset(SkFloatToScalar(-sigma3),
> SkFloatToScalar(-sigma3));
> On 2013/07/02 20:02:05, Stephen White wrote:
> > Nit:  could useSkRect::outset() here.
> 
> Done.
> 
>
https://codereview.chromium.org/18110012/diff/36001/src/effects/SkBlurMaskFil...
> src/effects/SkBlurMaskFilter.cpp:389: clipRect.inset(SkFloatToScalar(-sigma3),
> SkFloatToScalar(-sigma3));
> On 2013/07/02 20:02:05, Stephen White wrote:
> > And here.
> 
> Done.
> 
> https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.chromium.org/18110012/diff/36001/src/gpu/SkGpuDevice.cpp#n...
> src/gpu/SkGpuDevice.cpp:891: static SkBitmap wrap_texture(GrTexture* texture)
{
> On 2013/07/02 20:02:05, Stephen White wrote:
> > Nit: maybe get rid of "static" now that it's in the unnamed namespace?
> 
> Done.

lgtm

[Stephen White, 2013-07-03 14:32:56]

LGTM

https://codereview.chromium.org/18110012/diff/46002/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/18110012/diff/46002/src/core/SkMaskFilter.cpp...
src/core/SkMaskFilter.cpp:327: dst->unref();
FYI, one of the reasons I changed the SkImageFilter GPU API to use
texture-backed SkBitmaps rather than GrTextures directly was to avoid manual
ref/unref like this. It's a slight bit more boilerplate, but I find it less
error-prone, since the pixelref will take care of your refcounts. Just a
thought.

[robertphillips, 2013-07-03 15:34:51]

committed as r9888