Fast blurred rectangles on GPU

Perform the same analytic blur calculation on the GPU that we do on the CPU.  Results in significant performance gains when using Ganesh to render drop shadows in Chrome.

BUG=

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

[humper, 2013-12-21 01:38:53]

PTAL

[bsalomon, 2013-12-21 02:50:09]

The perf improvement looks real real nice!

https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:96: virtual bool directFilterMaskGPU(GrContext
*context,
minor style nit: type* var 

Also, is there a reason to make the path mutable? I guess the current calling
code always has a mutable path but is that a contract we want to commit to? 

Also for future implementors of this virtual it might be nice to provide
SkStrokeRec rather than doFill. That way the stroking parameters are available
to do a stroke fast-path. (It also can communicate that the path is filled
rather than stroked).

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:612: builder->fsCodeAppendf("\t%s =
final*%s;\n", outputColor, inputColor);
There is a weird case where inputColor may be NULL and the effect is supposed to
take that to mean solid white (yeah, very user-friendly).

There is a helper GrGLSLModulatef that skips the multiply when the input color
is NULL. There's an example here:
https://chromium.googlesource.com/external/skia/src/+/4ba644d7d0ae1a6eb097739...


There is a plan to fix this to make it less burdensome, but it will involve
changing the signature of emitCode unfortunately :(

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:662: key.fData32[1] = 1;
This guy doesn't also need the profile size?

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:694: fTrivialTransform(kLocal_GrCoordSet,
SkMatrix()) {
I thought SkMatrix didn't have a default cons and we use SkMatrix::I() for the
identity, but maybe I'm wrong. Or does any matrix work here?

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:715: // FIXME:  Perhaps we can do better.
I think this comment can be removed. It's not obvious to me that we can promise
anything useful about the output values from this effect. (This is used to help
optimize effects out of the shader or notice that blending can be disabled for a
program).

[humper, 2013-12-23 13:26:58]

https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:96: virtual bool directFilterMaskGPU(GrContext
*context,
On 2013/12/21 02:50:09, bsalomon wrote:
> minor style nit: type* var 
> 
> Also, is there a reason to make the path mutable? I guess the current calling
> code always has a mutable path but is that a contract we want to commit to? 
> 
> Also for future implementors of this virtual it might be nice to provide
> SkStrokeRec rather than doFill. That way the stroking parameters are available
> to do a stroke fast-path. (It also can communicate that the path is filled
> rather than stroked).

Done.

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:662: key.fData32[1] = 1;
On 2013/12/21 02:50:09, bsalomon wrote:
> This guy doesn't also need the profile size?

He's already got it; I'm only overwriting the width and height (reusing the same
key data structure).

https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:694: fTrivialTransform(kLocal_GrCoordSet,
SkMatrix()) {
On 2013/12/21 02:50:09, bsalomon wrote:
> I thought SkMatrix didn't have a default cons and we use SkMatrix::I() for the
> identity, but maybe I'm wrong. Or does any matrix work here?

Has to be the identity.  I believe that's the default, but I'll use ::I to make
it more explicit.

[humper, 2014-01-02 17:21:49]

On 2013/12/23 13:26:58, humper wrote:
>
https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
> File include/core/SkMaskFilter.h (right):
> 
>
https://codereview.chromium.org/119343003/diff/120001/include/core/SkMaskFilt...
> include/core/SkMaskFilter.h:96: virtual bool directFilterMaskGPU(GrContext
> *context,
> On 2013/12/21 02:50:09, bsalomon wrote:
> > minor style nit: type* var 
> > 
> > Also, is there a reason to make the path mutable? I guess the current
calling
> > code always has a mutable path but is that a contract we want to commit to? 
> > 
> > Also for future implementors of this virtual it might be nice to provide
> > SkStrokeRec rather than doFill. That way the stroking parameters are
available
> > to do a stroke fast-path. (It also can communicate that the path is filled
> > rather than stroked).
> 
> Done.
> 
>
https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
> File src/effects/SkBlurMaskFilter.cpp (right):
> 
>
https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
> src/effects/SkBlurMaskFilter.cpp:662: key.fData32[1] = 1;
> On 2013/12/21 02:50:09, bsalomon wrote:
> > This guy doesn't also need the profile size?
> 
> He's already got it; I'm only overwriting the width and height (reusing the
same
> key data structure).
> 
>
https://codereview.chromium.org/119343003/diff/120001/src/effects/SkBlurMaskF...
> src/effects/SkBlurMaskFilter.cpp:694: fTrivialTransform(kLocal_GrCoordSet,
> SkMatrix()) {
> On 2013/12/21 02:50:09, bsalomon wrote:
> > I thought SkMatrix didn't have a default cons and we use SkMatrix::I() for
the
> > identity, but maybe I'm wrong. Or does any matrix work here?
> 
> Has to be the identity.  I believe that's the default, but I'll use ::I to
make
> it more explicit.

ping

[reed1, 2014-01-02 17:32:39]

did not review the gpu-specific changes

https://codereview.chromium.org/119343003/diff/220001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/119343003/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:312: bool SkMaskFilter::directFilterMaskGPU(GrContext
*context,
minor nit: skia places the * or & with the type, not the var

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

https://codereview.chromium.org/119343003/diff/220001/src/effects/SkBlurMask....
src/effects/SkBlurMask.h:66: static uint8_t ProfileLookup(uint8_t *profile, int
loc, int blurred_width, int sharp_width);
Can you add some dox for these?

One example for ComputeBlurProfile
- how was uint8_t allocated (so I can know how to free the memory)?
- how much was allocated, or do I not need to know?

For the scanline routine:
- what is profile_size? bytes (size_t), or count (int)
- can profile be const?
- why is sigma float here, but SkScalar in the previous function?

[humper, 2014-01-02 18:12:10]

https://codereview.chromium.org/119343003/diff/220001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/119343003/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:312: bool SkMaskFilter::directFilterMaskGPU(GrContext
*context,
On 2014/01/02 17:32:39, reed1 wrote:
> minor nit: skia places the * or & with the type, not the var

Done -- someday I will train my fingers to type this way.

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

https://codereview.chromium.org/119343003/diff/220001/src/effects/SkBlurMask....
src/effects/SkBlurMask.h:66: static uint8_t ProfileLookup(uint8_t *profile, int
loc, int blurred_width, int sharp_width);
On 2014/01/02 17:32:39, reed1 wrote:
> Can you add some dox for these?
> 
> One example for ComputeBlurProfile
> - how was uint8_t allocated (so I can know how to free the memory)?
> - how much was allocated, or do I not need to know?
> 
> For the scanline routine:
> - what is profile_size? bytes (size_t), or count (int)
> - can profile be const?
> - why is sigma float here, but SkScalar in the previous function?

Dox added, consts added, unnecessary profile_size argument removed from one of
the functions!

I believe that the reason the sigmas have different types is that ComputeProfile
is used both by the CPU and the GPU, while ComputeBlurredScanline got factored
out of the CPU so we could pass the scanlines down to the GPU, but it looks like
I didn't quite finish the refactor, so there was a little more to do there.  I
fixed it up.

[bsalomon, 2014-01-06 15:04:10]

lgtm with two minor nits inline.

https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:95: *  true if drawing was successful.
Can we add a comment that if it returns false then grp should not be modified
(maybe that's obvious but someday this may get overridden in third party code by
non-Skia devs).

https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:100: const SkPath* path) const;
Style-nit, we usually use:

const Foo& foo - for const object params
Foo* foo - for non-const object params

with the exception that const ref counted objects are often passed as const Foo*
foo.

[humper, 2014-01-27 18:45:18]

On 2014/01/06 15:04:10, bsalomon wrote:
> lgtm with two minor nits inline.
> 
>
https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
> File include/core/SkMaskFilter.h (right):
> 
>
https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
> include/core/SkMaskFilter.h:95: *  true if drawing was successful.
> Can we add a comment that if it returns false then grp should not be modified
> (maybe that's obvious but someday this may get overridden in third party code
by
> non-Skia devs).
> 
>
https://codereview.chromium.org/119343003/diff/290001/include/core/SkMaskFilt...
> include/core/SkMaskFilter.h:100: const SkPath* path) const;
> Style-nit, we usually use:
> 
> const Foo& foo - for const object params
> Foo* foo - for non-const object params
> 
> with the exception that const ref counted objects are often passed as const
Foo*
> foo.

All nits, bugs, and windows build fixes addressed.  PTAL again.

[bsalomon, 2014-01-27 19:07:00]

https://codereview.chromium.org/119343003/diff/570001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/119343003/diff/570001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:622: // builder->fsCodeAppendf("\t%s =
vec4(%s.r,%s.g,0,1);\n", outputColor, texture_coords.c_str(),
texture_coords.c_str());
should we remove this line?

https://codereview.chromium.org/119343003/diff/570001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:648: GrCacheID
horizontalCacheID(gBlurProfileDomain, key);
should we make both the horizontal and vertical texture be 1 pixel high so that
we can use the same cache entries for both?

https://codereview.chromium.org/119343003/diff/570001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:706: fTrivialTransform =
GrCoordTransform(kLocal_GrCoordSet, mat);
Is this the right name for this now (trivial)?

https://codereview.chromium.org/119343003/diff/570001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:766: SkMatrix ctm = context->getMatrix();
this is now guaranteed to be identity because of the AutoMatrix above. Why don't
we move AutoMatrix down to be right above addCoverageEffect. Then I think we get
the right radius adjustment and also ensure the AutoMatrix is the last thing
that gets a chance to fail (since it whacks the GrPaint when it succeeds).

[bsalomon, 2014-01-27 20:31:32]

lgtm

[commit-bot: I haz the power, 2014-01-27 20:58:30]

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

[commit-bot: I haz the power, 2014-01-27 22:41:51]

Change committed as 13210

[epoger, 2014-01-28 01:21:02]

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

The reason for reverting is: Two problems:

1. broke compile on Build-Mac10.6-* bots
2. broke GM expectations on many bots

We'll need to examine the GM impact before re-landing.  I can work with you to
do this on Tuesday if you like; just IM or email me..

[epoger, 2014-01-28 03:45:38]

On 2014/01/28 01:21:02, epoger wrote:
> We'll need to examine the GM impact before re-landing.  I can work with you to
> do this on Tuesday if you like; just IM or email me..

By the time you are reading this, current GM results (as displayed at
http://108.170.217.246:10117/static/index.html , or if you run rebaseline_server
locally) will no longer include the results of this CL (since the revert has
occurred).

Here's how you can examine the GM results at the time this CL had worked its way
through the bots:

1. Patch in https://codereview.chromium.org/download/issue145353007_1.diff
2. Run your own rebaseline_server, telling it to display results as of
skia-autogen's https://code.google.com/p/skia-autogen/source/detail?r=24410 :
gm/rebaseline_server/server.py --export --actuals-revision 24410