Separable mask blurs: Add compile-time flag. Fix reversed offsets in asymmetrical blurs (this bug…

Separable mask blurs:  Add compile-time flag.  Fix reversed offsets in asymmetrical blurs (this bug cancels itself out, but I thought it might be confusing for future readers).  Use correct stride in asymmetrical blurs (this is a real bug).

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

[Stephen White, 2012-11-19 20:26:59]

PTAL

[reed1, 2012-11-19 21:12:35]

lgtm

What do you think the default for skia should be? the separable version? Is your
thinking that Chrome should not use this in our next DEPS roll?

[Stephen White, 2012-11-19 21:22:04]

On 2012/11/19 21:12:35, reed1 wrote:
> lgtm
> 
> What do you think the default for skia should be? the separable version? Is
your
> thinking that Chrome should not use this in our next DEPS roll?

The thinking is just to provide another lever to tweak in the performance vs
quality tradeoff.  It would benefit all platforms in terms of performance, at
the cost of the lack of subpixel radii.  Personally, I don't think the latter is
a huge loss -- I haven't seen a website yet with animated subpixel shadow blurs
-- but that's just my opinion.

[Stephen White, 2012-11-19 22:34:21]

OK, I've changed the #ifdef to a runtime flag.  PTAL.

[reed1, 2012-11-20 15:25:57]

I like the name

https://codereview.appspot.com/6782089/diff/4001/include/effects/SkBlurDrawLo...
File include/effects/SkBlurDrawLooper.h (right):

https://codereview.appspot.com/6782089/diff/4001/include/effects/SkBlurDrawLo...
include/effects/SkBlurDrawLooper.h:33: kHighQuality_BlurFlag       = 0x04,
might add a simple dox:

// faster, but may discretize the radius

https://codereview.appspot.com/6782089/diff/4001/src/effects/SkBlurDrawLooper...
File src/effects/SkBlurDrawLooper.cpp (right):

https://codereview.appspot.com/6782089/diff/4001/src/effects/SkBlurDrawLooper...
src/effects/SkBlurDrawLooper.cpp:31: SkBlurMaskFilter::kCoarseRadius_BlurFlag :
do you mean 0 for this line?

[Stephen White, 2012-11-20 15:33:13]

Also added the comment as requested.

https://codereview.appspot.com/6782089/diff/4001/src/effects/SkBlurDrawLooper...
File src/effects/SkBlurDrawLooper.cpp (right):

https://codereview.appspot.com/6782089/diff/4001/src/effects/SkBlurDrawLooper...
src/effects/SkBlurDrawLooper.cpp:31: SkBlurMaskFilter::kCoarseRadius_BlurFlag :
On 2012/11/20 15:25:57, reed1 wrote:
> do you mean 0 for this line?

Thanks, yeah, I noticed that too.  Fixed.

[reed1, 2012-11-20 15:38:33]

seems like we should add a gm and bench, to exercise the new flag.

https://codereview.appspot.com/6782089/diff/11/include/effects/SkBlurDrawLoop...
File include/effects/SkBlurDrawLooper.h (right):

https://codereview.appspot.com/6782089/diff/11/include/effects/SkBlurDrawLoop...
include/effects/SkBlurDrawLooper.h:36: /** mask for all blur flags */
0xF instead of 15

or perhaps we should write

kAll_BlurFlags = kIgnore... | kOverride... | ...;

[reed1, 2012-11-20 16:03:22]

btw -- in general I think this sort of change should come with tests, but in
this case I know you have been locally timing it, so we can add the gm/bench as
a 2nd CL if you wish.