Fast path for blurred round rects -- blur a small 9patch rect on the CPU

Fast path for blurred round rects -- blur a small 9patch rect on the CPU
And nonlinearly stretch the resulting texture across proxy geometry.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=14392

Committed: http://code.google.com/p/skia/source/detail?r=14404

Committed: http://code.google.com/p/skia/source/detail?r=14420

[humper, 2014-04-23 19:23:16]

PTAL

[bsalomon, 2014-04-23 19:50:29]

mostly looking good

https://codereview.chromium.org/248613004/diff/1/gm/blurroundrect.cpp
File gm/blurroundrect.cpp (right):

https://codereview.chromium.org/248613004/diff/1/gm/blurroundrect.cpp#newcode100
gm/blurroundrect.cpp:100: : fName("simpleblurroundrect") {
do we need to add this test to ignored-tests.txt?

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMask.h
File src/effects/SkBlurMask.h (right):

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMask.h#new...
src/effects/SkBlurMask.h:45: SkIPoint* margin = NULL, bool force_quality=false);
comment about force_quality? Also should be forceQuality and spaces around =

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:264: SK_CONF_DECLARE( bool,
c_analyticBlurRRect, "mask.filter.blur.analytibrrect", false, "Use the faster
analytic blur approach for ninepatch rects" );
analytibrrect?

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:828: // The flags are used to indicate which
corners are circluar (unflagged corners are assumed to
Delete comment?

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:907: smallRect.setWH(
smallRectSide,smallRectSide );

smallRect.setWH(smallRectSide, smallRectSide); // whitespace change

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:949: return fRRect == rrbe.fRRect && fSigma ==
rrbe.fSigma;
maybe it doesn't really matter much, but it seems like the rrects don't need to
be the same, just the radius. Maybe comparing the texture is better since it is
keyed off both the radii?

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:962: SkScalar r = random->nextRangeF(0, 9.f);
I think if r was actually 0 it would cause ::Create below to return NULL which
would cause the GLPrograms unit test to assert.

[humper, 2014-04-23 20:11:49]

https://codereview.chromium.org/248613004/diff/1/gm/blurroundrect.cpp
File gm/blurroundrect.cpp (right):

https://codereview.chromium.org/248613004/diff/1/gm/blurroundrect.cpp#newcode100
gm/blurroundrect.cpp:100: : fName("simpleblurroundrect") {
On 2014/04/23 19:50:30, bsalomon wrote:
> do we need to add this test to ignored-tests.txt?

Done.

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMask.h
File src/effects/SkBlurMask.h (right):

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMask.h#new...
src/effects/SkBlurMask.h:45: SkIPoint* margin = NULL, bool force_quality=false);
On 2014/04/23 19:50:30, bsalomon wrote:
> comment about force_quality? Also should be forceQuality and spaces around =

Done.

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:828: // The flags are used to indicate which
corners are circluar (unflagged corners are assumed to
On 2014/04/23 19:50:30, bsalomon wrote:
> Delete comment?

Done.

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:907: smallRect.setWH(
smallRectSide,smallRectSide );
On 2014/04/23 19:50:30, bsalomon wrote:
> 
> smallRect.setWH(smallRectSide, smallRectSide); // whitespace change

Done.

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:949: return fRRect == rrbe.fRRect && fSigma ==
rrbe.fSigma;
On 2014/04/23 19:50:30, bsalomon wrote:
> maybe it doesn't really matter much, but it seems like the rrects don't need
to
> be the same, just the radius. Maybe comparing the texture is better since it
is
> keyed off both the radii?

This was a bug -- changed to just look at the corner radius.  The texture might
not be computed yet, I think.

https://codereview.chromium.org/248613004/diff/1/src/effects/SkBlurMaskFilter...
src/effects/SkBlurMaskFilter.cpp:962: SkScalar r = random->nextRangeF(0, 9.f);
On 2014/04/23 19:50:30, bsalomon wrote:
> I think if r was actually 0 it would cause ::Create below to return NULL which
> would cause the GLPrograms unit test to assert.

Done.

[bsalomon, 2014-04-23 20:40:46]

lgtm if you fix the try bot build errors from PS 1

https://codereview.chromium.org/248613004/diff/40001/expectations/gm/ignored-...
File expectations/gm/ignored-tests.txt (right):

https://codereview.chromium.org/248613004/diff/40001/expectations/gm/ignored-...
expectations/gm/ignored-tests.txt:80: simpleblurrrect
If you haven't done it already, it's probably worth doing a local gm --config
gpu run both before and after your CL and skidff'ing the results in case there
are other GMs that will be changed.

[reed1, 2014-04-23 21:48:00]

https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h
File src/effects/SkBlurMask.h (right):

https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h...
src/effects/SkBlurMask.h:52: SkIPoint* margin = NULL, bool forceQuality =
false);
What is the impact on the raster side if we just always do this (i.e.
always_force)?

[humper, 2014-04-25 00:30:28]

On 2014/04/23 21:48:00, reed1 wrote:
> https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h
> File src/effects/SkBlurMask.h (right):
> 
>
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h...
> src/effects/SkBlurMask.h:52: SkIPoint* margin = NULL, bool forceQuality =
> false);
> What is the impact on the raster side if we just always do this (i.e.
> always_force)?

Extremely small blur radii applied to large images will take about 3x as long
with no significant visual quality improvement.

[reed1, 2014-04-25 15:02:32]

On 2014/04/25 00:30:28, humper wrote:
> On 2014/04/23 21:48:00, reed1 wrote:
> >
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h
> > File src/effects/SkBlurMask.h (right):
> > 
> >
>
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h...
> > src/effects/SkBlurMask.h:52: SkIPoint* margin = NULL, bool forceQuality =
> > false);
> > What is the impact on the raster side if we just always do this (i.e.
> > always_force)?
> 
> Extremely small blur radii applied to large images will take about 3x as long
> with no significant visual quality improvement.

But can we always compute the bounds *as if* we were going to do the
high-quality version? If so, would that allow you to reliably use it?

[humper, 2014-04-25 16:47:29]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-04-25 16:47:50]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/humper@google.com/248613004/120001

[humper, 2014-04-25 16:48:10]

On 2014/04/25 15:02:32, reed1 wrote:
> On 2014/04/25 00:30:28, humper wrote:
> > On 2014/04/23 21:48:00, reed1 wrote:
> > >
> https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h
> > > File src/effects/SkBlurMask.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h...
> > > src/effects/SkBlurMask.h:52: SkIPoint* margin = NULL, bool forceQuality =
> > > false);
> > > What is the impact on the raster side if we just always do this (i.e.
> > > always_force)?
> > 
> > Extremely small blur radii applied to large images will take about 3x as
long
> > with no significant visual quality improvement.
> 
> But can we always compute the bounds *as if* we were going to do the
> high-quality version? If so, would that allow you to reliably use it?

No, the problem is that when you only blur once, the boundary between blurry and
non-blurry isn't where you think it is, so the texture coordinate warping is
wrong.

[reed1, 2014-04-25 17:04:02]

On 2014/04/25 16:48:10, humper wrote:
> On 2014/04/25 15:02:32, reed1 wrote:
> > On 2014/04/25 00:30:28, humper wrote:
> > > On 2014/04/23 21:48:00, reed1 wrote:
> > > >
> >
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h
> > > > File src/effects/SkBlurMask.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/248613004/diff/60001/src/effects/SkBlurMask.h...
> > > > src/effects/SkBlurMask.h:52: SkIPoint* margin = NULL, bool forceQuality
=
> > > > false);
> > > > What is the impact on the raster side if we just always do this (i.e.
> > > > always_force)?
> > > 
> > > Extremely small blur radii applied to large images will take about 3x as
> long
> > > with no significant visual quality improvement.
> > 
> > But can we always compute the bounds *as if* we were going to do the
> > high-quality version? If so, would that allow you to reliably use it?
> 
> No, the problem is that when you only blur once, the boundary between blurry
and
> non-blurry isn't where you think it is, so the texture coordinate warping is
> wrong.

Hmmm, meaning where to place the nine-patch boundaries? Maybe drawing a picture
(live) would facilitate this. Are you in NC soon, or should we try a VC?

[commit-bot: I haz the power, 2014-04-28 13:19:30]

Change committed as 14392

[caryclark, 2014-04-28 14:44:33]

A revert of this CL has been created in
https://codereview.chromium.org/258893010/ by caryclark@google.com.

The reason for reverting is: Looks like this causes unit tests to fail with a
zero constructor like this one:
http://108.170.220.120:10117/builders/Test-Win7-ShuttleA-HD2000-x86-Release/b....

[humper, 2014-04-28 14:49:51]

On 2014/04/28 14:44:33, caryclark wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/258893010/ by mailto:caryclark@google.com.
> 
> The reason for reverting is: Looks like this causes unit tests to fail with a
> zero constructor like this one:
>
http://108.170.220.120:10117/builders/Test-Win7-ShuttleA-HD2000-x86-Release/b....

Fixed test creator to make sure test rectangles are wide enough to not return
NULL.  PTAL again.

[bsalomon, 2014-04-28 14:51:10]

lgtm

[humper, 2014-04-28 14:52:40]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-04-28 14:52:54]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/humper@google.com/248613004/140001

[commit-bot: I haz the power, 2014-04-28 15:09:19]

Change committed as 14404

[humper, 2014-04-28 18:57:27]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-04-28 18:57:51]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/humper@google.com/248613004/180001

[commit-bot: I haz the power, 2014-04-28 19:04:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:04:11]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildGm
http://skia-tree-status.appspot.com/redirect/compile-buildbots/buildstatus?bu...

[humper, 2014-04-28 19:09:13]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-04-28 19:09:30]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/humper@google.com/248613004/190001

[commit-bot: I haz the power, 2014-04-28 19:49:39]

Change committed as 14420