Implement morphology as a custom effect

[bsalomon, 2012-05-30 13:31:24]

PTAL. I'm not convinced that a common base class for morphology and convolution
was a great idea.

http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:622: enum MorphologyType {
I'm not crazy about this enum being here but I wanted to remove the Filter enum
values from SampleState (since we are on the path of shrinking sampler state to
nothing as all its configurability is pushed into custom effects).

Longer term we probably should invent a way for SkImageFilters that require
multiple passes etc to be able to interact with a gpu device / GrContext and
embody their own multi-pass logic rather than sticking it on GrContext.

[TomH, 2012-05-30 14:21:12]

On 2012/05/30 13:31:24, bsalomon wrote:
> PTAL. I'm not convinced that a common base class for morphology and
convolution
> was a great idea.

Have you considered either creating a helper class that they could contain, so
that we avoid inheritance-of-implementation, or just putting utility functions
in an anonymous namespace? I did the latter in a previous version of
http://codereview.appspot.com/6251050/, before merging normal + degenerate into
the same class, and it seemed pretty practical in that case.

Here the complexity of emitFS*() seems to outweigh the little bit of
redundancy-savings.

Also, just because we share a base class in GrGL1DKernelEffect doesn't mean we
need to share a base class in Gr1DKernelEffect, or vice-versa.

> http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h#newco...
> include/gpu/GrContext.h:622: enum MorphologyType {
> I'm not crazy about this enum being here but I wanted to remove the Filter
enum
> values from SampleState (since we are on the path of shrinking sampler state
to
> nothing as all its configurability is pushed into custom effects).

Agreed that we haven't yet found a good home for this.

> Longer term we probably should invent a way for SkImageFilters that require
> multiple passes etc to be able to interact with a gpu device / GrContext and
> embody their own multi-pass logic rather than sticking it on GrContext.

AFAIK we're muddling along without a good high-level model. I'm at least going
to throw up what we currently seem to be doing at the Skia tapas on this and let
people take potshots; maybe something will come out of that.

[Stephen White, 2012-05-30 14:22:57]

On 2012/05/30 13:31:24, bsalomon wrote:
> PTAL. I'm not convinced that a common base class for morphology and
convolution
> was a great idea.

Actually, I agree.  The motivation for re-using their parameters originally was
due to the performance hit for adding new parameters to the state hashing
function.  Now that that's no longer an issue, I think we should just split them
into two independent effects.  Do you think you would mind trying that out to
see what it looks like?

http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
http://codereview.appspot.com/6250073/diff/5003/include/gpu/GrContext.h#newco...
> include/gpu/GrContext.h:622: enum MorphologyType {
> I'm not crazy about this enum being here but I wanted to remove the Filter
enum
> values from SampleState (since we are on the path of shrinking sampler state
to
> nothing as all its configurability is pushed into custom effects).
> 
> Longer term we probably should invent a way for SkImageFilters that require
> multiple passes etc to be able to interact with a gpu device / GrContext and
> embody their own multi-pass logic rather than sticking it on GrContext.

[Stephen White, 2012-05-30 14:31:33]

BTW, the width limit of 25 was based on the max. number of texture samples in
DX9SM2 (32).  (25 was chosen simply so that it falls on an integer sigma (((25 -
1) / 2 / 3) = 4).  Of course, it was undocumented...)  Having the limit based on
the full width makes some sense, since it has a link to the hardware limits.

http://codereview.appspot.com/6250073/diff/5003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6250073/diff/5003/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:2150: convolve(fGpu, srcTexture, srcRect, kernelX,
halfWidthX,
I find it a little misleading to be passing the half width here, when the buffer
is actually the full kernel size.  I kind of preferred it the old way, where the
buffer and size match.

[bsalomon, 2012-05-30 14:43:14]

I'll post an updated patch that removes the base classes. I think we all agree
it was a net negative.

http://codereview.appspot.com/6250073/diff/5003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6250073/diff/5003/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:2150: convolve(fGpu, srcTexture, srcRect, kernelX,
halfWidthX,
On 2012/05/30 14:31:33, Stephen White wrote:
> I find it a little misleading to be passing the half width here, when the
buffer
> is actually the full kernel size.  I kind of preferred it the old way, where
the
> buffer and size match.

I was trying to make the convolution and morphology agree about how the width is
specified. I chose morphology's notion that the width is specified as the offset
from the center pixel because it can't be wrong (the full width can't
accidentally be an even number). I suppose if they don't share any
implementation it isn't so important but it seems like it'd be nice for them to
agree.

[bsalomon, 2012-05-30 21:21:08]

The latest patch set both rebaselines against Tom's recent change to how
uniforms are setup (r4083) and removes the common base class for the GL-specific
implementation classes. I kept the common GrCustomStage subclass,
Gr1DKernelEffect, but I could be convinced to split that up as well. It's pretty
small. I did like that it allows the two subclasses to share a Direction enum
type.

[TomH, 2012-05-31 15:28:06]

Great! Let's land this + your other outstanding CLs, then I'll put my gradient
implementation classes up for independent review and try to wrap my brain around
the refcounting issues.

http://codereview.appspot.com/6250073/diff/3002/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6250073/diff/3002/gyp/gpu.gyp#newcode279
gyp/gpu.gyp:279: '../src/gpu/effects/GrConvolutionEffect.cpp',
Error - this was .h

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
File src/gpu/effects/Gr1DKernelEffect.h (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:17: * species the number of texels on either
side of the center texel in X or Y
typo: species -> specifies

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:18: * that are read. Since the center pixel
is also read, the actual width is one
diction: actual -> total?

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:67: kFloat_GrSLType, "uKernel", stage, 2
* fHalfWidth * 2 + 1);
This is *4+1? Shouldn't it be *2+1?

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:112: state->fFSCode.appendf("\t\t%s =
sum%s;\n", outputColor, modulate.c_str());
Can we use state->fModulate.c_str() instead of recreating it here?

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrMorphologyE...
File src/gpu/effects/GrMorphologyEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrMorphologyE...
src/gpu/effects/GrMorphologyEffect.cpp:115: }
Use state->fModulate instead of recomputing it

[bsalomon, 2012-05-31 15:31:37]

On 2012/05/30 21:21:08, bsalomon wrote:
> The latest patch set both rebaselines against Tom's recent change to how
> uniforms are setup (r4083) and removes the common base class for the
GL-specific
> implementation classes. I kept the common GrCustomStage subclass,
> Gr1DKernelEffect, but I could be convinced to split that up as well. It's
pretty
> small. I did like that it allows the two subclasses to share a Direction enum
> type.

New patch rebaselines against recent changes and fixes some GrCustomEffect
leaks.

[TomH, 2012-05-31 15:35:04]

Changes LGTM, but see comment #7 for some issues.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:369: // We probably want to make the genProgram
call be ref-count aware
Not quite sure what this means.

[Stephen White, 2012-05-31 15:45:45]

http://codereview.appspot.com/6250073/diff/2006/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6250073/diff/2006/gyp/gpu.gyp#newcode279
gyp/gpu.gyp:279: '../src/gpu/effects/GrConvolutionEffect.cpp',
GrConvolutionEffect.cpp seems to be in here twice

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/Gr1DKernelEff...
File src/gpu/effects/Gr1DKernelEffect.h (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:15: * has a half-width (or radius). The
kernel is specified in the src texture's
Now that you point it out, radius is a better name than half-width (which I just
made up).  Maybe we should just radius that instead.  Up to you.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:38: int             fHalfWidth;
Nit:  Spacing issue on fHalfWidth.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:67: kFloat_GrSLType, "uKernel", stage, 2
* fHalfWidth * 2 + 1);
= fHalfWidth * 4 + 1.  Is this correct?  (If not, another argument for not doing
this computation in multiple places).

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
File src/gpu/effects/GrConvolutionEffect.h (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.h:40: // would exceed a 32 wide kernel.
Thanks for documenting this.  :)

[bsalomon, 2012-05-31 17:53:38]

Latest patch addresses both Tom and Stephen's comments. I also made filling out
the Gaussian kernel a helper on GrConvolutionEffect.

http://codereview.appspot.com/6250073/diff/3002/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6250073/diff/3002/gyp/gpu.gyp#newcode279
gyp/gpu.gyp:279: '../src/gpu/effects/GrConvolutionEffect.cpp',
On 2012/05/31 15:28:06, TomH wrote:
> Error - this was .h

Done.

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
File src/gpu/effects/Gr1DKernelEffect.h (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:17: * species the number of texels on either
side of the center texel in X or Y
On 2012/05/31 15:28:06, TomH wrote:
> typo: species -> specifies

Done.

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:18: * that are read. Since the center pixel
is also read, the actual width is one
On 2012/05/31 15:28:06, TomH wrote:
> diction: actual -> total?

Done.

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:67: kFloat_GrSLType, "uKernel", stage, 2
* fHalfWidth * 2 + 1);
On 2012/05/31 15:28:06, TomH wrote:
> This is *4+1? Shouldn't it be *2+1?
Duh, done.

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:112: state->fFSCode.appendf("\t\t%s =
sum%s;\n", outputColor, modulate.c_str());
On 2012/05/31 15:28:06, TomH wrote:
> Can we use state->fModulate.c_str() instead of recreating it here?

Didn't realize we had that... Done.

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrMorphologyE...
File src/gpu/effects/GrMorphologyEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/3002/src/gpu/effects/GrMorphologyE...
src/gpu/effects/GrMorphologyEffect.cpp:115: }
On 2012/05/31 15:28:06, TomH wrote:
> Use state->fModulate instead of recomputing it

Done.

http://codereview.appspot.com/6250073/diff/2006/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6250073/diff/2006/gyp/gpu.gyp#newcode279
gyp/gpu.gyp:279: '../src/gpu/effects/GrConvolutionEffect.cpp',
On 2012/05/31 15:45:45, Stephen White wrote:
> GrConvolutionEffect.cpp seems to be in here twice

Done.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/Gr1DKernelEff...
File src/gpu/effects/Gr1DKernelEffect.h (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/Gr1DKernelEff...
src/gpu/effects/Gr1DKernelEffect.h:15: * has a half-width (or radius). The
kernel is specified in the src texture's
On 2012/05/31 15:45:45, Stephen White wrote:
> Now that you point it out, radius is a better name than half-width (which I
just
> made up).  Maybe we should just radius that instead.  Up to you.

I agree radius is better. I changed it and consolidated all the radius->width
conversions (except one) into a single function.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:38: int             fHalfWidth;
On 2012/05/31 15:45:45, Stephen White wrote:
> Nit:  Spacing issue on fHalfWidth.

Done.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:67: kFloat_GrSLType, "uKernel", stage, 2
* fHalfWidth * 2 + 1);
On 2012/05/31 15:45:45, Stephen White wrote:
> = fHalfWidth * 4 + 1.  Is this correct?  (If not, another argument for not
doing
> this computation in multiple places).

Done.

http://codereview.appspot.com/6250073/diff/2006/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6250073/diff/2006/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:369: // We probably want to make the genProgram
call be ref-count aware
On 2012/05/31 15:35:04, TomH wrote:
> Not quite sure what this means.

I guess it is OK since genProgram doesn't hold long lived pointers to the
effects (right?).

Comment removed.

[TomH, 2012-05-31 17:57:17]

LGTM

[Stephen White, 2012-05-31 18:16:16]

http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:2123: int width =
GrConvolutionEffect::WidthFromRadius(radiusX);
This variable now seems to be unused.

http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:2143: int width =
GrConvolutionEffect::WidthFromRadius(radiusY);
Same here.

[bsalomon, 2012-05-31 18:27:29]

On 2012/05/31 18:16:16, Stephen White wrote:
> http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp#newcod...
> src/gpu/GrContext.cpp:2123: int width =
> GrConvolutionEffect::WidthFromRadius(radiusX);
> This variable now seems to be unused.
> 
>
http://codereview.appspot.com/6250073/diff/11023/src/gpu/GrContext.cpp#newcod...
> src/gpu/GrContext.cpp:2143: int width =
> GrConvolutionEffect::WidthFromRadius(radiusY);
> Same here.

Nice catch, removed.

[Stephen White, 2012-05-31 18:36:50]

LGTM

[bsalomon, 2012-06-01 18:59:12]

Landed r4102