GPU-based Gaussian blur

This is a first stab at implementing a GPU-based Gaussian blur in Ganesh.  The convolution shader is implemented as a new filtering mode.  There are several known issues:

- no support for blur types other than "normal"
- FBO truncation problem at high zoom values
- uses bilinear for upsampling instead of Mitchell

Nevertheless, I'd like to get it in (default "off") so I can continue work without too much code churn.

[Stephen White, 2011-07-06 20:46:18]

Revert some unnecessary changes

[Stephen White, 2011-07-06 20:51:28]

Please take a look.  Thanks!

[reed1, 2011-07-06 21:29:21]

Lets chip away at this in pieces, so we don't feel rushed. I will submit a tiny
CL that just extends maskfilter so it can be queried (in the pattern we use for
shaders).

[bsalomon, 2011-07-06 21:32:07]

Overall the Gr changes look good to me. I'll defer to Mike on the Sk mask filter
changes.

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:164:
GrGLProgram::ProgramDesc::StageDesc::kConvolution_FetchMode,
When you sync to TOT you'll find this array is gone and instead we have
kFetchModeCnt. Should be a painless merge.

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:824: stage.fKernelWidth =
fCurrDrawState.fSamplerStates[s].getKernelWidth();
The program desc also serves as the cache key. So unless we reset the kernel
width to some known value when the convolution filter is turned off I think we
may get unnecessary programs generated based on a stale value of kernel width.
We might have to do something like:
if (convolution filter) {
   stage.fKernelWidth = ...getKernelWidth();
} else {
   stage.fKernelWidth = 0; // set to known default when not used
}

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

http://codereview.appspot.com/4645082/diff/10001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:778: static GrPathFill skToGrFillType(SkPath::FillType
fillType) {
It's about time to get rid of the GrPathFill enum. (Not nec. in this change)

[twiz, 2011-07-06 21:40:04]

Quick drive-by.

http://codereview.appspot.com/4645082/diff/10001/gpu/include/GrSamplerState.h
File gpu/include/GrSamplerState.h (right):

http://codereview.appspot.com/4645082/diff/10001/gpu/include/GrSamplerState.h...
gpu/include/GrSamplerState.h:231: memcpy(fKernel, kernel, kernelWidth *
sizeof(float));
Small nit:  Is it worth while to ASSERT here on the kernelWidth being less than
MAX_KERNEL_WIDTH?

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

http://codereview.appspot.com/4645082/diff/10001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:796: return exp(- (x * x) / (2.0f * sigma * sigma));
Maybe add a comment on why the scaling factor of 1/sqrt(2*pi*sigma*sigma) isn't
needed here due to renormalization.

[Stephen White, 2011-07-06 21:49:42]

On 2011/07/06 21:29:21, reed1 wrote:
> Lets chip away at this in pieces, so we don't feel rushed. I will submit a
tiny
> CL that just extends maskfilter so it can be queried (in the pattern we use
for
> shaders).

Sounds good.  Note that I need to be (and am not currently) checking for the
IgnoreTransform flag, so either the MaskFilter accessors should provide access
to the flags, or we could refactor the (very simple) final radius calculation
into a "getMappedRadius(const SkMatrix& matrix)" function.

[Stephen White, 2011-07-06 22:07:55]

http://codereview.appspot.com/4645082/diff/10001/gpu/include/GrSamplerState.h
File gpu/include/GrSamplerState.h (right):

http://codereview.appspot.com/4645082/diff/10001/gpu/include/GrSamplerState.h...
gpu/include/GrSamplerState.h:231: memcpy(fKernel, kernel, kernelWidth *
sizeof(float));
On 2011/07/06 21:40:04, twiz wrote:
> Small nit:  Is it worth while to ASSERT here on the kernelWidth being less
than
> MAX_KERNEL_WIDTH?  

Definitely!  Done, thanks.

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:164:
GrGLProgram::ProgramDesc::StageDesc::kConvolution_FetchMode,
On 2011/07/06 21:32:07, bsalomon wrote:
> When you sync to TOT you'll find this array is gone and instead we have
> kFetchModeCnt. Should be a painless merge.

Yeah, I noticed that.  No problem.

http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:824: stage.fKernelWidth =
fCurrDrawState.fSamplerStates[s].getKernelWidth();
On 2011/07/06 21:32:07, bsalomon wrote:
> The program desc also serves as the cache key. So unless we reset the kernel
> width to some known value when the convolution filter is turned off I think we
> may get unnecessary programs generated based on a stale value of kernel width.
> We might have to do something like:
> if (convolution filter) {
>    stage.fKernelWidth = ...getKernelWidth();
> } else {
>    stage.fKernelWidth = 0; // set to known default when not used
> }

I was using a kernel width of zero to indicate that the filter is off, and I was
assuming that the AutoStateRestore in GrContext::convolveRect() would set it
back to zero, and that the stage would pick it up from there.  Is that a bad
assumption?

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

http://codereview.appspot.com/4645082/diff/10001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:796: return exp(- (x * x) / (2.0f * sigma * sigma));
On 2011/07/06 21:40:04, twiz wrote:
> Maybe add a comment on why the scaling factor of 1/sqrt(2*pi*sigma*sigma)
isn't
> needed here due to renormalization.

Thanks Jeff, will do.

[Stephen White, 2011-07-06 22:21:02]

Respond to review comments

[bsalomon, 2011-07-07 13:06:48]

>http://codereview.appspot.com/4645082/diff/10001/gpu/src/GrGpuGLShaders.cpp#n...
>gpu/src/GrGpuGLShaders.cpp:824: stage.fKernelWidth =
>fCurrDrawState.fSamplerStates[s].getKernelWidth();
>On 2011/07/06 21:32:07, bsalomon wrote:
>> The program desc also serves as the cache key. So unless we reset the kernel
>> width to some known value when the convolution filter is turned off I think
we
>> may get unnecessary programs generated based on a stale value of kernel
width.
>> We might have to do something like:
>> if (convolution filter) {
>>    stage.fKernelWidth = ...getKernelWidth();
>> } else {
>>    stage.fKernelWidth = 0; // set to known default when not used
>> }
>
>I was using a kernel width of zero to indicate that the filter is off, and I
was
>assuming that the AutoStateRestore in GrContext::convolveRect() would set it
>back to zero, and that the stage would pick it up from >there.  Is that a bad
>assumption?

I think you're correct that the call site will restore the width to zero. I'm
just a bit uncomfortable relying on all future call sites to restore the width
to zero. There are cases where its OK whack the GrDrawTarget state and not
restore it. If I understand correctly, this could cause a new identical
non-convolving shader to be generated.

One other thought: The 2x2 fetch mode uses a texelSize uniform. Would it make
any sense to combine this and the imageIncrement? They seem related.

Now that I've stared at this a bit longer it occurs to me that the convolveRect
call on GrContext is pretty low-level (convolving in 1 direction and requiring
the caller to have computed the tex coord increment). Can we push a lot of the
logic from the drawWithGPUMaskFilter down into GrContext? [I'm OK with it going
in as is and doing this as part of the next iteration.]

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGL.cpp#newcode1831
gpu/src/GrGpuGL.cpp:1831: if (GrSamplerState::kNearest_Filter ==
sampler.getFilter()) {
Is this logic correct for the new filter type? I'm guessing you want nearest
since you're applying the filter in the shader. We might want to change this to
a switch case to be clear about the intentions for each filter type.
GR_GL_LINEAR for kBilinear_Filter and k4x4Downsample_Filter, GR_GL_NEAREST for
kNearest_Filter and kConvolution_Filter.

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:791: stage.fFetchMode =
GrGLProgram::ProgramDesc::StageDesc::kConvolution_FetchMode;
nit: now that you've rebased you can just say StageDesc::kConvolution_FetchMode
(no need for GrGLProgram::ProgramDesc::)

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

http://codereview.appspot.com/4645082/diff/12003/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:938: SkAutoMalloc kernelStorage(kernelWidth *
sizeof(float));
nit: there is an SkAutoTMalloc (templated).

http://codereview.appspot.com/4645082/diff/12003/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1138: if (USE_GPU_BLUR) {
When this gets turned on I think we'll need to replace the macro with a query
function on the GrContext (which in turn asks the GrGpu object) that reports
whether convolution is supported. I say this because we do still have the
fixed-pipe code.

[Stephen White, 2011-07-07 15:56:52]

Thanks for your review.

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGL.cpp#newcode1831
gpu/src/GrGpuGL.cpp:1831: if (GrSamplerState::kNearest_Filter ==
sampler.getFilter()) {
Good catch.  Actually, I'm sampling at the pixel centers, so it should be pretty
much the same either way.  (In a previous implementation, I sampled at pixel
edges, and nearest gave 50/50 chance of being either pixel).  That said, nearest
should be faster, and it's better to be explicit, so I've replaced it with a
switch per your suggestion.

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4645082/diff/12003/gpu/src/GrGpuGLShaders.cpp#n...
gpu/src/GrGpuGLShaders.cpp:791: stage.fFetchMode =
GrGLProgram::ProgramDesc::StageDesc::kConvolution_FetchMode;
On 2011/07/07 13:06:48, bsalomon wrote:
> nit: now that you've rebased you can just say
StageDesc::kConvolution_FetchMode
> (no need for GrGLProgram::ProgramDesc::)

Done.

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

http://codereview.appspot.com/4645082/diff/12003/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:938: SkAutoMalloc kernelStorage(kernelWidth *
sizeof(float));
On 2011/07/07 13:06:48, bsalomon wrote:
> nit: there is an SkAutoTMalloc (templated).

Done.

[Stephen White, 2011-07-07 15:58:35]

Updated for new SkMaskFilter accessors; review comments

[Stephen White, 2011-07-07 16:04:12]

Fix indentation; add default case for grToGLFilter

[Stephen White, 2011-07-07 16:06:17]

(Re-uploading) Use new SkMaskFilter accessors, review comments

[Stephen White, 2011-07-07 16:10:23]

Remove unnecessary changes to SampleBlur

[Stephen White, 2011-07-07 16:20:49]

Sorry for the flurry of patches -- Rietveld seems to be having a hissy fit, and
I also uploaded more than I intended to.  Please take a look at your leisure.

[reed1, 2011-07-07 16:23:43]

Hehehe, and they tell us the machine has no soul....

On Thu, Jul 7, 2011 at 12:20 PM,  <senorblanco@chromium.org> wrote:
> Sorry for the flurry of patches -- Rietveld seems to be having a hissy
> fit, and I also uploaded more than I intended to.  Please take a look at
> your leisure.
>
> http://codereview.appspot.com/4645082/
>

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

LGTM

[reed1, 2011-07-08 15:25:09]

Mostly just api comments.

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrContext.h
File gpu/include/GrContext.h (left):

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrContext.h#oldc...
gpu/include/GrContext.h:527: 
If this is in the public section, can we add some comment about its parameters?
esp. imageIncrement (for me). I presume the kernel is assumed to be square
(kernelWidth x kernelWidth)?

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrSamplerState.h
File gpu/include/GrSamplerState.h (left):

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrSamplerState.h...
gpu/include/GrSamplerState.h:150: Filter getFilter() const { return fFilter; }
These 3 are fine, though it is more logical/clear to return all 3 values at
once?

e.g.

const float* getKernel(int* width, float[2] imageIncrement) const;

That would sort of parallel setConvolutionParameters(). Speaking of which,
should we name it getConvolutionParameters instead of getKernel?

http://codereview.appspot.com/4645082/diff/11013/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4645082/diff/11013/gpu/src/GrGpuGL.cpp#newcode578
gpu/src/GrGpuGL.cpp:578: float ii[2] = {0.0f, 0.0f};
Should we allow NULL for the increment[] as well, if the kernelWidth is 0? That
would avoid needing to construct a dummy array like this.

[Stephen White, 2011-07-08 16:40:44]

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrContext.h
File gpu/include/GrContext.h (left):

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrContext.h#oldc...
gpu/include/GrContext.h:527: 
On 2011/07/08 15:25:09, reed1 wrote:
> If this is in the public section, can we add some comment about its
parameters?
> esp. imageIncrement (for me). I presume the kernel is assumed to be square
> (kernelWidth x kernelWidth)?

Actually it's a 1D separable convolution, so for a 2D convolution,
convolveRect() is called twice:  once with imageIncrement is set to (1/w, 0),
once with it set to (0, 1/h).  I'll document that.

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrSamplerState.h
File gpu/include/GrSamplerState.h (left):

http://codereview.appspot.com/4645082/diff/11013/gpu/include/GrSamplerState.h...
gpu/include/GrSamplerState.h:150: Filter getFilter() const { return fFilter; }
On 2011/07/08 15:25:09, reed1 wrote:
> These 3 are fine, though it is more logical/clear to return all 3 values at
> once?
> 
> e.g.
> 
> const float* getKernel(int* width, float[2] imageIncrement) const;
> 
> That would sort of parallel setConvolutionParameters(). Speaking of which,
> should we name it getConvolutionParameters instead of getKernel?

On this one, I'm going to stick with separate functions if it's ok with you,
since it makes the callsite a bit cleaner, and there's precedent with the
setRadial2Params() and getRadial2*().

http://codereview.appspot.com/4645082/diff/11013/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4645082/diff/11013/gpu/src/GrGpuGL.cpp#newcode578
gpu/src/GrGpuGL.cpp:578: float ii[2] = {0.0f, 0.0f};
On 2011/07/08 15:25:09, reed1 wrote:
> Should we allow NULL for the increment[] as well, if the kernelWidth is 0?
That
> would avoid needing to construct a dummy array like this.

Done.

[Stephen White, 2011-07-08 16:41:24]

Fixes for review comments

[reed1, 2011-07-08 16:44:51]

LGTM

[Stephen White, 2011-07-08 18:50:47]

Landed as r1830 -- closing.