Added a new function to directly generate a blurred rectangle analytically.

Added a new function to directly generate a blurred rectangle analytically.

Added two new microbenchmarks to demonstrate speedup over existing BlurSeparable approach.

Added new GM tests for blurred rectangles. 
Committed: https://code.google.com/p/skia/source/detail?r=7034

[bsalomon, 2013-01-04 14:04:50]

Mostly LGTM. I have some pesky style nits below.

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp
File bench/BlurMaskBench.cpp (right):

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp#newcode3
bench/BlurMaskBench.cpp:3: * Copyright 2011 Google Inc.
2013

https://codereview.appspot.com/7037050/diff/1/bench/BlurRectBench.cpp
File bench/BlurRectBench.cpp (right):

https://codereview.appspot.com/7037050/diff/1/bench/BlurRectBench.cpp#newcode16
bench/BlurRectBench.cpp:16: #define SMALL   SkIntToScalar(2)
Maybe this test should be in the same cpp as BlurMaskBench. They could even be
subclasses of a common base or a single class with a param that selects mask vs
rect.

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp
File gm/blurrect.cpp (right):

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp#newcode159
gm/blurrect.cpp:159: fName(name)
style-nit

: fThing(thing)
, fOtherThing(otherThing)
, fThirdThing(thirdThing)
...

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp#newcode165
gm/blurrect.cpp:165: unsigned int width() const { return fRectWidth; }
skia/google style is to use ints for these values unless a value > int_max is
actually sensible.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1044: static float gaussian_integral( float x )
style-nit: no newline before {

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1046: if ( x > 1.5f ) return 0.0f;
style-nit: if () always has {}

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1101: int profile_size;
style-nit: 4 space indentations (which is at least 2 too many IMO).

[Stephen White, 2013-01-04 15:16:58]

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp
File bench/BlurMaskBench.cpp (right):

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp#newcode21
bench/BlurMaskBench.cpp:21: class BlurMaskBench: public SkBenchmark {
I'd prefer that we just add new cases (and possibly flags) to the existing
BlurBench, rather than adding two new, separate tests that could more easily go
out of sync.

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp#newcode60
bench/BlurMaskBench.cpp:60: if (fRadius > 50) {
Technically, this should be SkIntToScalar(50) to make SK_SCALAR_IS_FIXED work,
but I don't know if we care (ask Mike).

https://codereview.appspot.com/7037050/diff/1/bench/BlurMaskBench.cpp#newcode62
bench/BlurMaskBench.cpp:62: } else if (fRadius > 5) {
Same here.

https://codereview.appspot.com/7037050/diff/1/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/core/SkMaskFilter.cpp#newco...
src/core/SkMaskFilter.cpp:201: 
Is the change to this file intentional?

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1072: int size = radius * 3 + 1;
Skia style is 4-space indents.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1110: float radius = provided_radius * .57735;
Since this fudge factor is in the same file as the one for SkBlurMask::Blur(),
perhaps we could at least factor them out into a constant at the start of the
file?

This calculation also won't work for SK_SCALAR_IS_FIXED, although I'm not sure
we care.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1114: int pad = radius * 1.5 + 1;
1.5 is a double constant; probably personal style, but I prefer 1.5f to avoid
casting up to double for the *.  You might also want to explicitly cast the
result to int to avoid compiler warnings (but that's also personal style).

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1118: dst->fBounds.set(src.fLeft - pad, src.fTop -
pad, src.fRight + pad, src.fBottom + pad);
If you like, I think you could use:

dst->fBounds.set(src);
dst->fBounds.outset(pad);

[Humper, 2013-01-04 15:59:35]

https://codereview.appspot.com/7037050/diff/1/bench/BlurRectBench.cpp
File bench/BlurRectBench.cpp (right):

https://codereview.appspot.com/7037050/diff/1/bench/BlurRectBench.cpp#newcode16
bench/BlurRectBench.cpp:16: #define SMALL   SkIntToScalar(2)
On 2013/01/04 14:04:50, bsalomon wrote:
> Maybe this test should be in the same cpp as BlurMaskBench. They could even be
> subclasses of a common base or a single class with a param that selects mask
vs
> rect. 

Done.

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp
File gm/blurrect.cpp (right):

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp#newcode159
gm/blurrect.cpp:159: fName(name)
On 2013/01/04 14:04:50, bsalomon wrote:
> style-nit
> 
> : fThing(thing)
> , fOtherThing(otherThing)
> , fThirdThing(thirdThing)
> ...

Done.

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp#newcode159
gm/blurrect.cpp:159: fName(name)
On 2013/01/04 14:04:50, bsalomon wrote:
> style-nit
> 
> : fThing(thing)
> , fOtherThing(otherThing)
> , fThirdThing(thirdThing)
> ...

Done.

https://codereview.appspot.com/7037050/diff/1/gm/blurrect.cpp#newcode165
gm/blurrect.cpp:165: unsigned int width() const { return fRectWidth; }
On 2013/01/04 14:04:50, bsalomon wrote:
> skia/google style is to use ints for these values unless a value > int_max is
> actually sensible.

Done.

https://codereview.appspot.com/7037050/diff/1/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/core/SkMaskFilter.cpp#newco...
src/core/SkMaskFilter.cpp:201: 
On 2013/01/04 15:16:58, Stephen White wrote:
> Is the change to this file intentional?

Nope :)

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1044: static float gaussian_integral( float x )
On 2013/01/04 14:04:50, bsalomon wrote:
> style-nit: no newline before {

Done.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1046: if ( x > 1.5f ) return 0.0f;
On 2013/01/04 14:04:50, bsalomon wrote:
> style-nit: if () always has {}

Done.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1072: int size = radius * 3 + 1;
On 2013/01/04 15:16:58, Stephen White wrote:
> Skia style is 4-space indents.

Done.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1110: float radius = provided_radius * .57735;
On 2013/01/04 15:16:58, Stephen White wrote:
> Since this fudge factor is in the same file as the one for SkBlurMask::Blur(),
> perhaps we could at least factor them out into a constant at the start of the
> file?
> 
> This calculation also won't work for SK_SCALAR_IS_FIXED, although I'm not sure
> we care.

In this case, it's not exactly the same because the Blur() function has this low
quality mode, so the fudge factor there is the square root of the number of
passes.  If we're *certain* that the number of passes for high quality blurring
will always be 3, then that's fine.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1114: int pad = radius * 1.5 + 1;
On 2013/01/04 15:16:58, Stephen White wrote:
> 1.5 is a double constant; probably personal style, but I prefer 1.5f to avoid
> casting up to double for the *.  You might also want to explicitly cast the
> result to int to avoid compiler warnings (but that's also personal style).

Done.

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1118: dst->fBounds.set(src.fLeft - pad, src.fTop -
pad, src.fRight + pad, src.fBottom + pad);
On 2013/01/04 15:16:58, Stephen White wrote:
> If you like, I think you could use:
> 
> dst->fBounds.set(src);
> dst->fBounds.outset(pad);

Done, although fBounds is an SkIRect, so I had to do:

dst->fBounds = SkIRect::makeWH( src.width(), src.height() );
dst->fBounds.outset( pad, pad );

[Stephen White, 2013-01-04 16:22:14]

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1110: float radius = provided_radius * .57735;
On 2013/01/04 15:59:35, Humper wrote:
> On 2013/01/04 15:16:58, Stephen White wrote:
> > Since this fudge factor is in the same file as the one for
SkBlurMask::Blur(),
> > perhaps we could at least factor them out into a constant at the start of
the
> > file?
> > 
> > This calculation also won't work for SK_SCALAR_IS_FIXED, although I'm not
sure
> > we care.
> 
> In this case, it's not exactly the same because the Blur() function has this
low
> quality mode, so the fudge factor there is the square root of the number of
> passes.  If we're *certain* that the number of passes for high quality
blurring
> will always be 3, then that's fine.

Yeah, it's guaranteed.  Feel free to rewrite the code in SkBlurMask::Blur() to
be more explicit about this; something like:

SkScalar passRadius = kHigh_Quality == quality ? SkScalarMul(radius,
kFudgeFactor) : radius;

rather than using the passCount.

https://codereview.appspot.com/7037050/diff/11001/gyp/SampleApp.gyp
File gyp/SampleApp.gyp (right):

https://codereview.appspot.com/7037050/diff/11001/gyp/SampleApp.gyp#newcode9
gyp/SampleApp.gyp:9: '../src/effects', #needed for BlurMask.h
Hmm.  I see why you need this, but it seems unfortunate.  If at the end of the
day we're going to need to call SkBlurMask::BlurRect() from other parts of skia
(which I assume we're going to need to do, since SkBlurMaskFilter can't know
that it's blurring a rect and we don't need to rasterize the source rect
anyway), SkBlurMask.h should probably be moved to include/effects.

[reed1, 2013-01-04 16:29:55]

Not so sure we need to move the header. SkBlurMask is only called from within
src/effects today. I vote to be conservative about publicizing headers.

[reed1, 2013-01-04 16:30:27]

modulo one caller in SampleStrokePath, but I don't count our hacking demo code
:)

[Stephen White, 2013-01-04 16:31:14]

On 2013/01/04 16:29:55, reed1 wrote:
> Not so sure we need to move the header. SkBlurMask is only called from within
> src/effects today. I vote to be conservative about publicizing headers.

Sounds good.  We can move it if/when we need to.

[Humper, 2013-01-04 17:04:56]

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/1/src/effects/SkBlurMask.cpp#newc...
src/effects/SkBlurMask.cpp:1110: float radius = provided_radius * .57735;
On 2013/01/04 16:22:14, Stephen White wrote:
> On 2013/01/04 15:59:35, Humper wrote:
> > On 2013/01/04 15:16:58, Stephen White wrote:
> > > Since this fudge factor is in the same file as the one for
> SkBlurMask::Blur(),
> > > perhaps we could at least factor them out into a constant at the start of
> the
> > > file?
> > > 
> > > This calculation also won't work for SK_SCALAR_IS_FIXED, although I'm not
> sure
> > > we care.
> > 
> > In this case, it's not exactly the same because the Blur() function has this
> low
> > quality mode, so the fudge factor there is the square root of the number of
> > passes.  If we're *certain* that the number of passes for high quality
> blurring
> > will always be 3, then that's fine.
> 
> Yeah, it's guaranteed.  Feel free to rewrite the code in SkBlurMask::Blur() to
> be more explicit about this; something like:
> 
> SkScalar passRadius = kHigh_Quality == quality ? SkScalarMul(radius,
> kFudgeFactor) : radius;
> 
> rather than using the passCount.

Okay, this is done.

[Stephen White, 2013-01-04 17:19:06]

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:21: #define kBlurRadiusFudgeFactor .57735f
This isn't an SkScalar.  I think you need SkFloatToScalar() here (granted, that
means it probably isn't actually a constant in SK_SCALAR_IS_FIXED, but I can
live with that).  Does it give pixel-exact GM results for the old blur
implementation?

Alternatively, you could use SkScalarInverse(SkScalarSqrt(SkIntToScalar(3))),
which would work for SK_SCALAR_IS_DOUBLE too, although it wouldn't be a constant
at all.  I can live with that too.

[Humper, 2013-01-04 18:46:21]

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:21: #define kBlurRadiusFudgeFactor .57735f
On 2013/01/04 17:19:06, Stephen White wrote:
> This isn't an SkScalar.  I think you need SkFloatToScalar() here (granted,
that
> means it probably isn't actually a constant in SK_SCALAR_IS_FIXED, but I can
> live with that).  Does it give pixel-exact GM results for the old blur
> implementation?
> 
> Alternatively, you could use SkScalarInverse(SkScalarSqrt(SkIntToScalar(3))),
> which would work for SK_SCALAR_IS_DOUBLE too, although it wouldn't be a
constant
> at all.  I can live with that too.

I think this is finally fixed -- Mike held my hand through the SkScalar zen.

Also, I did a full GM comparison and found that refactoring the fudge factor as
done here affects zero pixels in the existing master images.

[Stephen White, 2013-01-04 19:14:53]

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/16003/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:21: #define kBlurRadiusFudgeFactor .57735f
On 2013/01/04 18:46:21, Humper wrote:
> On 2013/01/04 17:19:06, Stephen White wrote:
> > This isn't an SkScalar.  I think you need SkFloatToScalar() here (granted,
> that
> > means it probably isn't actually a constant in SK_SCALAR_IS_FIXED, but I can
> > live with that).  Does it give pixel-exact GM results for the old blur
> > implementation?
> > 
> > Alternatively, you could use
SkScalarInverse(SkScalarSqrt(SkIntToScalar(3))),
> > which would work for SK_SCALAR_IS_DOUBLE too, although it wouldn't be a
> constant
> > at all.  I can live with that too.
> 
> I think this is finally fixed -- Mike held my hand through the SkScalar zen.

Hmm, I'm still seeing some mismatched types (see below).  Note that this is
still only an issue if we care about SK_SCALAR_IS_FIXED; any of the variants
you've posted so far will work fine for the other flavours.
> 
> Also, I did a full GM comparison and found that refactoring the fudge factor
as
> done here affects zero pixels in the existing master images.

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:877: SkScalar passRadius = (kHigh_Quality == quality)
? radius * kBlurRadiusFudgeFactor: radius;
This is a mul between an SkScalar and a float, resulting in a float stored into
an SkScalar.

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:1118: float radius = SkScalarToFloat( provided_radius
* kBlurRadiusFudgeFactor );
This is a mul between an SkScalar and a float.

[reed1, 2013-01-04 19:17:05]

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:877: SkScalar passRadius = (kHigh_Quality == quality)
? radius * kBlurRadiusFudgeFactor: radius;
On 2013/01/04 19:14:53, Stephen White wrote:
> This is a mul between an SkScalar and a float, resulting in a float stored
into
> an SkScalar.

My bad. Numerically the code is fine (the result of fixed * float is a fixed,
but may need a cast to avoid a warning.

[Stephen White, 2013-01-04 19:45:56]

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:877: SkScalar passRadius = (kHigh_Quality == quality)
? radius * kBlurRadiusFudgeFactor: radius;
On 2013/01/04 19:17:05, reed1 wrote:
> On 2013/01/04 19:14:53, Stephen White wrote:
> > This is a mul between an SkScalar and a float, resulting in a float stored
> into
> > an SkScalar.
> 
> My bad. Numerically the code is fine (the result of fixed * float is a fixed,
> but may need a cast to avoid a warning.

Eeesh, yuck, ok.  I've just always used the SkScalar macros, just to be safe.

[Humper, 2013-01-04 19:47:42]

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.appspot.com/7037050/diff/20001/src/effects/SkBlurMask.cpp#...
src/effects/SkBlurMask.cpp:877: SkScalar passRadius = (kHigh_Quality == quality)
? radius * kBlurRadiusFudgeFactor: radius;
On 2013/01/04 19:17:05, reed1 wrote:
> On 2013/01/04 19:14:53, Stephen White wrote:
> > This is a mul between an SkScalar and a float, resulting in a float stored
> into
> > an SkScalar.
> 
> My bad. Numerically the code is fine (the result of fixed * float is a fixed,
> but may need a cast to avoid a warning.

Ok after a truly silly  amount of discussion about how to multiply two numbers,
I think I've finally uploaded a version that gets the zen of this right.

[Stephen White, 2013-01-04 19:55:46]

LGTM.  Thanks for going the extra .57735 miles.

[bsalomon, 2013-01-04 20:16:21]

On 2013/01/04 19:55:46, Stephen White wrote:
> LGTM.  Thanks for going the extra .57735 miles.

lgtm