Implement blur types for GPU-based blur

This patch implements the remaining blur types (inner, outer, solid) in the GPU-based Gaussian blur.  They are implemented using a post-upsampling blending pass.

[bsalomon, 2011-07-11 19:47:22]

On 2011/07/11 18:43:52, Stephen White wrote:

I'll defer to Mike on this one as I'm not familiar with the blur types. That
said it looks like it does what it says.

[reed1, 2011-07-11 20:25:20]

If drawWithGPUMaskFilter returns false, seems like we should call
drawWithMaskFilter.

When you have a non-normal blur, we are losing antialiasing, and the resulting
edge (between the blur and the original) is pixelated.

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (left):

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode812
src/gpu/SkGpuDevice.cpp:812: {
lets cache 1/(2*sigma*sigma) and pass it in, so we don't perform the repeated
divide. Otherwise the comment about sigma being const is a little funny.

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode814
src/gpu/SkGpuDevice.cpp:814: // since we renormalize the kernel after generation
anyway.
sk_float_exp() is declared to take a float and return a float. This is better
than calling exp(), which is declared to take and return a double.

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode833
src/gpu/SkGpuDevice.cpp:833: {
SkTSwap() should do the trick. Shouldn't need this function at all.

[reed1, 2011-07-11 20:27:29]

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

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#newcode848
src/gpu/SkGpuDevice.cpp:848: SkMaskFilter::BlurType blurType =
filter->asABlur(&info);
possible we can move USE_GPU_BLUR here, since we'll want to exercise the case
when we don't know the maskfilter type anyway...

#ifdef USE_GPU_BLUR
    blurType = filter->asABlur(&info);
#else
    blurType = SkMaskFilter::kNone_BlurType;
#endif

[reed1, 2011-07-11 20:34:44]

I will write a bench for blur, so we can make that part of the evaluation.

[Stephen White, 2011-07-11 20:59:25]

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (left):

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode812
src/gpu/SkGpuDevice.cpp:812: {
On 2011/07/11 20:25:20, reed1 wrote:
> lets cache 1/(2*sigma*sigma) and pass it in, so we don't perform the repeated
> divide. Otherwise the comment about sigma being const is a little funny.

Done.  (Although note that this is a max. of 25 CPU divides per *frame*. 
Probably not the bottleneck when the GPU is going to be doing 25x2 texture
samples and ALU per *pixel*).

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode814
src/gpu/SkGpuDevice.cpp:814: // since we renormalize the kernel after generation
anyway.
On 2011/07/11 20:25:20, reed1 wrote:
> sk_float_exp() is declared to take a float and return a float. This is better
> than calling exp(), which is declared to take and return a double.

Done.

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#oldcode833
src/gpu/SkGpuDevice.cpp:833: {
On 2011/07/11 20:25:20, reed1 wrote:
> SkTSwap() should do the trick. Shouldn't need this function at all.

Done.

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

http://codereview.appspot.com/4664075/diff/1/src/gpu/SkGpuDevice.cpp#newcode848
src/gpu/SkGpuDevice.cpp:848: SkMaskFilter::BlurType blurType =
filter->asABlur(&info);
On 2011/07/11 20:27:29, reed1 wrote:
> possible we can move USE_GPU_BLUR here, since we'll want to exercise the case
> when we don't know the maskfilter type anyway...
> 
> #ifdef USE_GPU_BLUR
>     blurType = filter->asABlur(&info);
> #else
>     blurType = SkMaskFilter::kNone_BlurType;
> #endif

Done.

[Stephen White, 2011-07-11 21:00:23]

Responding to review comments

[Stephen White, 2011-07-11 21:07:33]

On 2011/07/11 20:25:20, reed1 wrote:
> If drawWithGPUMaskFilter returns false, seems like we should call
> drawWithMaskFilter.

Done.

> When you have a non-normal blur, we are losing antialiasing, and the resulting
> edge (between the blur and the original) is pixelated.

I've been meaning to ask Brian about this:  this seems to be because the "hard
shadow" path is drawn without AA, even if I set fAntiAlias true on the mask
paint.  (It actually happens w/kNormal_BlurType too, you just don't see it 'cos
it's blurred -- try a radius of 1 to see it).  Is there something else I need to
do to get AA working?

Also, are there other settings I should be copying from the passed-in GrPaint to
the one used for the mask draw?  I wasn't sure what the semantics are supposed
to be here.

[bsalomon, 2011-07-11 21:12:44]

On 2011/07/11 21:07:33, Stephen White wrote:
> On 2011/07/11 20:25:20, reed1 wrote:
> > If drawWithGPUMaskFilter returns false, seems like we should call
> > drawWithMaskFilter.
> 
> Done.
> 
> > When you have a non-normal blur, we are losing antialiasing, and the
resulting
> > edge (between the blur and the original) is pixelated.
> 
> I've been meaning to ask Brian about this:  this seems to be because the "hard
> shadow" path is drawn without AA, even if I set fAntiAlias true on the mask
> paint.  (It actually happens w/kNormal_BlurType too, you just don't see it
'cos
> it's blurred -- try a radius of 1 to see it).  Is there something else I need
to
> do to get AA working?

You mean even when you do tempPaint.fAntiAlias = true it draws non-AA?  That
seems like a bug. I'll take a look.

> 
> Also, are there other settings I should be copying from the passed-in GrPaint
to
> the one used for the mask draw?  I wasn't sure what the semantics are supposed
> to be here.

Just the AA flag should be enough. Like this:

tempPaint.fAntiAlias = grp->fAntiAlias;

[Stephen White, 2011-07-11 21:26:56]

Copy antialiasing state from the passed-in paint

[Stephen White, 2011-07-11 21:45:07]

Fix antialiasing on the hard shadow mask

[reed1, 2011-07-12 14:20:31]

I like the new narrower use of USE_GPU_BLUR

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

http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:910: tempPaint.fAntiAlias = grp->fAntiAlias;
Add a comment why we're forcing the blendcoeff here. Is this a work-around for
some other future fix?

http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1035: if (origEntry) {
Given the distance between this unlock and where origEntry was created, can we
creates some auto-stack helper to perform the unlock, to guard against an early
exit being introduced later.

[Stephen White, 2011-07-12 14:50:44]

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

http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:910: tempPaint.fAntiAlias = grp->fAntiAlias;
On 2011/07/12 14:20:31, reed1 wrote:
> Add a comment why we're forcing the blendcoeff here.

Done.

> Is this a work-around for
> some other future fix?

Ask Brian?  It sort of makes sense for it to be done externally, since you might
want some other blend mode with your antialiased primitives, but some kind of
blending has to be enabled for it to work.

http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1035: if (origEntry) {
On 2011/07/12 14:20:31, reed1 wrote:
> Given the distance between this unlock and where origEntry was created, can we
> creates some auto-stack helper to perform the unlock, to guard against an
early
> exit being introduced later.

Followup patch?

[Stephen White, 2011-07-12 14:52:05]

Respond to review comments on patch set 4

[reed1, 2011-07-12 15:11:34]

On 2011/07/12 14:50:44, Stephen White wrote:
> http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:910: tempPaint.fAntiAlias = grp->fAntiAlias;
> On 2011/07/12 14:20:31, reed1 wrote:
> > Add a comment why we're forcing the blendcoeff here.
> 
> Done.
> 
> > Is this a work-around for
> > some other future fix?
> 
> Ask Brian?  It sort of makes sense for it to be done externally, since you
might
> want some other blend mode with your antialiased primitives, but some kind of
> blending has to be enabled for it to work.

As long as there's a comment in the code, I'm fine if Brian gives it to you.


> 
>
http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:1035: if (origEntry) {
> On 2011/07/12 14:20:31, reed1 wrote:
> > Given the distance between this unlock and where origEntry was created, can
we
> > creates some auto-stack helper to perform the unlock, to guard against an
> early
> > exit being introduced later.
> 
> Followup patch?

Sure, lets just put a comment on the unlockTexture call, saying that we need
such a aut-stack helper.

[bsalomon, 2011-07-12 15:16:13]

The comment can be:

AA uses the "coverage" stages on GrDrawTarget. Coverage with a dst blend
coeff of zero requires
dual source blending support in order to properly blend partially covered
pixels. This means the AA
code path may not be taken. So we use a dst blend coeff of ISA. We could
special case AA
draws to a dst surface with  known alpha=0 to use a zero dst coeff when dual
source blending
isn't available.

On Tue, Jul 12, 2011 at 10:50 AM, <senorblanco@chromium.org> wrote:

>
> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp>
> File src/gpu/SkGpuDevice.cpp (right):
>
> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp#newcode910<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newcode910>
> src/gpu/SkGpuDevice.cpp:910: tempPaint.fAntiAlias = grp->fAntiAlias;
> On 2011/07/12 14:20:31, reed1 wrote:
>
>> Add a comment why we're forcing the blendcoeff here.
>>
>
> Done.
>
>
>  Is this a work-around for
>> some other future fix?
>>
>
> Ask Brian?  It sort of makes sense for it to be done externally, since
> you might want some other blend mode with your antialiased primitives,
> but some kind of blending has to be enabled for it to work.
>
>
> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp#newcode1035<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newcode1035>
> src/gpu/SkGpuDevice.cpp:1035: if (origEntry) {
> On 2011/07/12 14:20:31, reed1 wrote:
>
>> Given the distance between this unlock and where origEntry was
>>
> created, can we
>
>> creates some auto-stack helper to perform the unlock, to guard against
>>
> an early
>
>> exit being introduced later.
>>
>
> Followup patch?
>
>
>
http://codereview.appspot.com/**4664075/<http://codereview.appspot.com/4664075/>
>

[Stephen White, 2011-07-12 15:32:29]

And comments per review

[Stephen White, 2011-07-12 15:39:08]

On Tue, Jul 12, 2011 at 11:11 AM, <reed@google.com> wrote:

> On 2011/07/12 14:50:44, Stephen White wrote:
>
> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp>
>
>> File src/gpu/SkGpuDevice.cpp (right):
>>
>
>
> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp#newcode910<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newcode910>
>
>> src/gpu/SkGpuDevice.cpp:910: tempPaint.fAntiAlias = grp->fAntiAlias;
>> On 2011/07/12 14:20:31, reed1 wrote:
>> > Add a comment why we're forcing the blendcoeff here.
>>
>
>  Done.
>>
>
>  > Is this a work-around for
>> > some other future fix?
>>
>
>  Ask Brian?  It sort of makes sense for it to be done externally, since
>>
> you might
>
>> want some other blend mode with your antialiased primitives, but some
>>
> kind of
>
>> blending has to be enabled for it to work.
>>
>
> As long as there's a comment in the code, I'm fine if Brian gives it to
> you.


Done.


> http://codereview.appspot.com/**4664075/diff/13001/src/gpu/**
>
SkGpuDevice.cpp#newcode1035<http://codereview.appspot.com/4664075/diff/13001/src/gpu/SkGpuDevice.cpp#newcode1035>
>
>> src/gpu/SkGpuDevice.cpp:1035: if (origEntry) {
>> On 2011/07/12 14:20:31, reed1 wrote:
>> > Given the distance between this unlock and where origEntry was
>>
> created, can we
>
>> > creates some auto-stack helper to perform the unlock, to guard
>>
> against an
>
>> early
>> > exit being introduced later.
>>
>
>  Followup patch?
>>
>
> Sure, lets just put a comment on the unlockTexture call, saying that we
> need such a aut-stack helper.


Done.

>
>
>
http://codereview.appspot.com/**4664075/<http://codereview.appspot.com/4664075/>
>

[reed1, 2011-07-12 15:42:56]

LGTM -- deferring to brian

[bsalomon, 2011-07-12 16:48:43]

On 2011/07/12 15:42:56, reed1 wrote:
> LGTM -- deferring to brian

LGTM

[Stephen White, 2011-07-12 16:56:22]

On 2011/07/12 16:48:43, bsalomon wrote:
> On 2011/07/12 15:42:56, reed1 wrote:
> > LGTM -- deferring to brian
> 
> LGTM

Landed as r1839; closing.