Gaussian blurs for images

Implement optional Gaussian image blurs on saveLayer()/restore(). The caller creates an an SkBlurImageFilter, sets it on an SkPaint, passes that paint to saveLayer(), draws the primitives which are to be blurred, then restore(), which applies the blur.  The blurs have separate sizes in the horizontal and vertical direction.  This feature is GPU-only for now.

The original soft shadows code was refactored to allow it to be applied to an input GrTexture, and to apply X and Y sigma separately.  In the soft shadows implementation, clipping is now done in the pre-downsampled coordinate space.  The temporary textures used for downsampling and blurring are now allocated without stencil, which should save on VRAM.  When doing one of the special blur modes in soft shadows, we also preserve the originally-drawn texture, saving a copy to a temporary.

At Brian's suggestion, this change also turns on GR_AGGRESSIVE_SHADER_OPTS in SampleApp, since it's also on in Chrome.

NB:  Due to the clipping change, there are slight pixel differences on the blurs_gpu and shadows_gpu tests, so those will require rebaselining on all platforms, as will some of the WebKit layout tests (TBD).

[reed1, 2011-11-02 19:42:08]

Why do we need to add fields to SkPaint? Can this be triggered via a blurmask
filter, or (the future) imagefilter?

[Stephen White, 2011-11-02 20:47:56]

On 2011/11/02 19:42:08, reed1 wrote:
> Why do we need to add fields to SkPaint? Can this be triggered via a blurmask
> filter, or (the future) imagefilter?

After chatting offline, we've decided Mike will implement SkImageFilter on the
SkPaint, and the blur params will be put in a subclass of SkImageFilter and
extracted by SkGpuDevice.

[Stephen White, 2011-11-03 20:31:02]

Update to new SkPaint/SkImageFilter API

[reed1, 2011-11-03 20:40:52]

Defer to brian for the bulk of the changes. Lets be sure you can iterate on this
w/o causing any real distraction for Brian.

I presume we will see a CPU impl at some point, but I'm fine for that to be done
later.

http://codereview.appspot.com/5322068/diff/8001/include/effects/SkBlurImageFi...
File include/effects/SkBlurImageFilter.h (right):

http://codereview.appspot.com/5322068/diff/8001/include/effects/SkBlurImageFi...
include/effects/SkBlurImageFilter.h:15: public:
nit: pass const-ref instead of copy of SkSize

    SkBlurImageFilter(const SkSize& sigma)

Or possibly (SkScalar sigmaX, SkScalar sigmaY) to we don't force the caller to
const up a struct. Not a big deal.

[Stephen White, 2011-11-03 21:02:27]

Change SkBlurImageFilter constructor as per review

[bsalomon, 2011-11-03 21:46:30]

It's great that the existing blur shader code can be used to implement the image
filter blur as well!

Do you think we should move some of the blur code out of SkGpuDevice.cpp to its
own file?

GrPaint doesn't feel like the right place for the blurX and blurY data since
none of the GrContext draw methods look at them. Can those values be confined to
SkGpuDevice? 

drawBitmap is already a pretty complicated code path. Interactions with tiling
of large bitmaps could get ugly. Could we do the intercept in drawDevice? I
believe SkCanvas does a saveLayer / restore when it sees an imageFilter. The
restore calls drawDevice so I think we could catch it there. I don't think we
need to check for imageFilter in the general skPaint->grPaint conversion if only
drawDevice needs to be aware of it. IIUC canvas guarantees we only see it in
drawDevice.

Would it be too much work to push the blur code into into GrContext as a
drawBlurredTexture or some such? It could take a texture, blur parameters, and
draw the blurred image to the current render target using private convolveInX/Y
functions. Then SkGpuDevice could invoke it as needed to implement image or mask
filters. I don't think its necessary for this change but I'd like to move in
that direction.

http://codereview.appspot.com/5322068/diff/8001/samplecode/SampleBitmapBlur.cpp
File samplecode/SampleBitmapBlur.cpp (right):

http://codereview.appspot.com/5322068/diff/8001/samplecode/SampleBitmapBlur.c...
samplecode/SampleBitmapBlur.cpp:1: /*
Now that SampleApp is pulling GMs in we should probably just make this a GM.

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

http://codereview.appspot.com/5322068/diff/8001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1327: if (grPaint->fBlurX > 0.0f || grPaint->fBlurY >
0.0f)
Nit: { on same line as "if"

[Stephen White, 2011-11-04 14:54:53]

Thanks for the review.

On 2011/11/03 21:46:30, bsalomon wrote:
> It's great that the existing blur shader code can be used to implement the
image
> filter blur as well!
> 
> Do you think we should move some of the blur code out of SkGpuDevice.cpp to
its
> own file?

Definitely.  The idea was that it would end up in GrContext eventually, as you
suggest below.

> GrPaint doesn't feel like the right place for the blurX and blurY data since
> none of the GrContext draw methods look at them. Can those values be confined
to
> SkGpuDevice? 

I think if we move this code to drawDevice(), it can be done.

> drawBitmap is already a pretty complicated code path. Interactions with tiling
> of large bitmaps could get ugly. Could we do the intercept in drawDevice? I
> believe SkCanvas does a saveLayer / restore when it sees an imageFilter.

It doesn't.. yet.  My idea was that imagefilters would simply be like the
colorfilters, applied during bitmap draws, and that the callers would do their
own canvas allocations as necessary, and make immediate-mode drawing calls.

About the saveLayer() approach:  is the idea that the image filters are applied
during restore(), and (confined to the clipbounds, if given), then copied over
the original canvas?  My worry was that if the restore() step was automatic, it
might not provide enough control over the allocation and processing that might
be needed for the full Filters DAG.  My idea was that we'd simply allocate a new
SkCanvas (in WebKit) if necessary, and do the restore() step manually, by
drawing over the source image.  But after working through a few examples, I
can't think of one that would be problematic.

Let me give the blurs a shot using saveLayer() and see how it looks.

> Would it be too much work to push the blur code into into GrContext as a
> drawBlurredTexture or some such? It could take a texture, blur parameters, and
> draw the blurred image to the current render target using private
convolveInX/Y
> functions. Then SkGpuDevice could invoke it as needed to implement image or
mask
> filters. I don't think its necessary for this change but I'd like to move in
> that direction.

Absolutely!  My idea was that the gaussianBlur() function should be that entry
point, and it should now be portable (along with its support functions) into
GrContext.

>
http://codereview.appspot.com/5322068/diff/8001/samplecode/SampleBitmapBlur.cpp
> File samplecode/SampleBitmapBlur.cpp (right):
> 
>
http://codereview.appspot.com/5322068/diff/8001/samplecode/SampleBitmapBlur.c...
> samplecode/SampleBitmapBlur.cpp:1: /*
> Now that SampleApp is pulling GMs in we should probably just make this a GM.

Will do.

> http://codereview.appspot.com/5322068/diff/8001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/5322068/diff/8001/src/gpu/SkGpuDevice.cpp#newco...
> src/gpu/SkGpuDevice.cpp:1327: if (grPaint->fBlurX > 0.0f || grPaint->fBlurY >
> 0.0f)
> Nit: { on same line as "if"

Will fix.

[Stephen White, 2011-11-04 19:08:58]

Moved drawing to drawDevice() to use save/restore; changed sample to GM

[Stephen White, 2011-11-04 19:10:50]

Remove some commented-out includes

[bsalomon, 2011-11-07 17:42:22]

LGTM

[Stephen White, 2012-01-03 22:54:31]

On 2011/11/07 17:42:22, bsalomon wrote:
> LGTM

This landed way back in the mists of time, aka r2643.  Closing.