Ganesh custom shader: GrConvolutionEffect

Here's GrConvolutionEffect.

Passes GM and GLPrograms unit test; Linux GPU benchmarking is noisy, but the numbers don't look much worse there.

Current shader output looks like:

...
	coverage2 = value2 * coverage1;
	vec4 coverage3;
	float c3 = dot(vStage3, vStage3) - uRadial2Params3[4];
	{ // stage 3 Convolution 
		vec4 sum = vec4(0, 0, 0, 0);
		vec2 coord = vec2((-c3 / vRadial2BCoeff3), 0.5);
		for (int i = 0; i < 7; i++) {
			sum += texture2D(uSampler3, coord) * uKernel3[i];
			coord += uImageIncrement3;
		}
		coverage3 = sum * coverage2;
	}
	dualSourceOut = vec4(coverage3);
	fsColorOut = vec4(matrixedColor * coverage3);

[TomH, 2012-05-08 21:57:10]

Ready to be torn apart by my discerning coworkers.

[robertphillips, 2012-05-09 13:54:57]

LGTM modulo some minor nits

Do we care at all about the human readability of the shader code? If so we might
want to add an indentation variable somewhere.

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:43: };
INHERITED?

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:55: void
GrGLConvolutionEffect::setupVSUnis(VarArray& vsUnis,
aren't only const references allowed - shouldn't "vsUnis" be a pointer?

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:68: void
GrGLConvolutionEffect::setupFSUnis(VarArray& fsUnis,
Same here for fsUnis

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:182: };
INHERITED?

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.h:34: };
private block w/ an INHERITED definition?

[bsalomon, 2012-05-09 14:02:19]

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/GrContext.cpp#newcode300
src/gpu/GrContext.cpp:300: drawState->sampler(0)->setCustomStage(
Convolution is pretty heavyweight but as the custom effect idea takes over the
world we're going to have come up with a way to not call new for every stage of
every draw.

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:57: fImageIncrementVar =
&vsUnis.push_back();
For the furture, can we think of a convenient way for the Effect to only worry
about var name uniqueness within the code it generates, not across stages?

I can think of two approaches:

1) scoping using the shader language. You've done this for local vars using a {}
block

2) The effect doesn't get to actually produce the final name but has to use some
name factory passed to it. The name factory appends a stage num, inserts into
the array, and returns the final name.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:88: code->appendf("// convolution
VS\n");
Idea for tiny enhancement:

The effect has a name string virtual. The generator auto-adds a
{ // stage <n> <name>

}


I also have a small pref that the { and } are indented even if the code within
them isn't. Like

void main(...) {
    vec4 someBoilerplateVar = uMat * aPosition;
    { // Stage 0 Convolution
    vec3 localConvolutionVar;
    ...
    }
    { // Stage 1 MyFavoriteEffect
    ...
    }
}

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:193: return (sEffectId << 8) |
(c->width() & 0xff);
Going along with the TODO above about not hand-coding values, it'd be nice if
the program description generator forced the id into the key rather than
trusting each subclass to do it.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp
File src/gpu/gl/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:302: // TODO: is there a more elegant way to
express this?
As we discussed yesterday it feels like we might want a random effect factory of
some sort as the number of subclasses grows.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:303: if (stage.fFetchMode ==
StageDesc::kConvolution_FetchMode) {
Should we still have this enum value at all?

[TomH, 2012-05-09 15:44:50]

Also responded to verbal comments: made factory class name backend-agnostic,
moved from gpu/gl/ to its own source files in gpu/.

For now I've hardwired the additional indent that the {} around the
CustomStage-generated GLSL suggests.

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/GrContext.cpp#newcode300
src/gpu/GrContext.cpp:300: drawState->sampler(0)->setCustomStage(
Well, we wanted to not carry around lots of extra parameters for unused stages,
so I don't want to prereserve space inside the sampler.
If we switch from new to a static factory on the class, we can hide this choice
& switch the default over to an auto-growing buffer where appropriate (or
always?), but then that adds a bunch of buffer management code.
I'd like to see that show up as a problem before we take on a lot of code
complexity to solve it?

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:43: };
On 2012/05/09 13:54:57, robertphillips wrote:
> INHERITED?

Done.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:55: void
GrGLConvolutionEffect::setupVSUnis(VarArray& vsUnis,
On 2012/05/09 13:54:57, robertphillips wrote:
> aren't only const references allowed - shouldn't "vsUnis" be a pointer?

Done.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:68: void
GrGLConvolutionEffect::setupFSUnis(VarArray& fsUnis,
On 2012/05/09 13:54:57, robertphillips wrote:
> Same here for fsUnis

Done.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:88: code->appendf("// convolution
VS\n");
On 2012/05/09 14:02:20, bsalomon wrote:
> Idea for tiny enhancement:
> 
> The effect has a name string virtual. The generator auto-adds a
> { // stage <n> <name>
> 
> }
> 
> 
> I also have a small pref that the { and } are indented even if the code within
> them isn't. Like
> 
> void main(...) {
>     vec4 someBoilerplateVar = uMat * aPosition;
>     { // Stage 0 Convolution
>     vec3 localConvolutionVar;
>     ...
>     }
>     { // Stage 1 MyFavoriteEffect
>     ...
>     }
> }

Done.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:182: };
On 2012/05/09 13:54:57, robertphillips wrote:
> INHERITED?

Done.

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.h:34: };
On 2012/05/09 13:54:57, robertphillips wrote:
> private block w/ an INHERITED definition?

Done.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp
File src/gpu/gl/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:303: if (stage.fFetchMode ==
StageDesc::kConvolution_FetchMode) {
On 2012/05/09 14:02:20, bsalomon wrote:
> Should we still have this enum value at all?

I left it in merely so that we can use the current unit test generation flow
here.

[bsalomon, 2012-05-09 16:00:22]

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

http://codereview.appspot.com/6199053/diff/8001/src/gpu/GrContext.cpp#newcode300
src/gpu/GrContext.cpp:300: drawState->sampler(0)->setCustomStage(
On 2012/05/09 15:44:50, TomH wrote:
> Well, we wanted to not carry around lots of extra parameters for unused
stages,
> so I don't want to prereserve space inside the sampler.
> If we switch from new to a static factory on the class, we can hide this
choice
> & switch the default over to an auto-growing buffer where appropriate (or
> always?), but then that adds a bunch of buffer management code.
> I'd like to see that show up as a problem before we take on a lot of code
> complexity to solve it?

Yes, I didn't mean to imply that you should make any change at this time. We can
see if it does show up but I'd bet a lot that it will.

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp
File src/gpu/gl/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/6199053/diff/8001/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:303: if (stage.fFetchMode ==
StageDesc::kConvolution_FetchMode) {
On 2012/05/09 15:44:50, TomH wrote:
> On 2012/05/09 14:02:20, bsalomon wrote:
> > Should we still have this enum value at all?
> 
> I left it in merely so that we can use the current unit test generation flow
> here.

Ok, can we put a comment in GrGLProgram.h where it is defined and say that the
enum value is no longer valid, left for testing custom stage convolution, will
be removed soon.

http://codereview.appspot.com/6199053/diff/13003/src/gpu/GrProgramStageFactory.h
File src/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6199053/diff/13003/src/gpu/GrProgramStageFactor...
src/gpu/GrProgramStageFactory.h:11: #include <SkTypes.h>
Probably better to include GrTypes here since it will pull GrConfig. None of
your code here depends on stuff in GrConfig right now but we generally assume
every Gr file brings in GrConfig.

http://codereview.appspot.com/6199053/diff/13003/src/gpu/effects/GrConvolutio...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

http://codereview.appspot.com/6199053/diff/13003/src/gpu/effects/GrConvolutio...
src/gpu/effects/GrConvolutionEffect.cpp:250: if (fKernelWidth != s->fKernelWidth
||
minor nit, feel free to ignore, but this seems simpler:

return fKernelWidth == s->fKernelWidth &&
       fDirection == s->fDirection &&
       0 == memcmp(fKernel, s->fKernel, fKernelWidth * sizeof(float));

[TomH, 2012-05-09 16:24:26]

On 2012/05/09 16:00:22, bsalomon wrote:
> ...

Done.

[bsalomon, 2012-05-09 16:29:20]

On 2012/05/09 16:24:26, TomH wrote:
> On 2012/05/09 16:00:22, bsalomon wrote:
> > ...
> 
> Done.

LGTM

[Stephen White, 2012-05-09 19:15:53]

LGTM, assuming it passes the WebKit layout tests.  Most of my comments are nits.

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

http://codereview.appspot.com/6199053/diff/1012/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:293: /// NULL; we need to have a complex ID system
here so that we can
Is this comment still valid?  Looks like you're doing comparisons now?

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

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrContext.cpp#newcode301
src/gpu/GrContext.cpp:301: new GrConvolutionEffect(direction, kernelWidth,
kernel));
<bikeshed>  Is it a stage, or an effect?  </bikeshed>

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrProgramStageFactory.h
File src/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrProgramStageFactory...
src/gpu/GrProgramStageFactory.h:11: #include <GrTypes.h>
Should be #include "GrTypes.h".

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

http://codereview.appspot.com/6199053/diff/1012/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:9: #include "../gl/GrGLProgramStage.h"
I don't see many other places in skia where we use "..".  I think we generally
add the to the base include path via -I, rather than using relative paths.

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:16: #include "../GrProgramStageFactory.h"
unnecessary "../" (but not the first one in this file).

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGpuGLShaders.cpp
File src/gpu/gl/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:11: #include "../effects/GrConvolutionEffect.h"
Should be in the -I path, I think.

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:303: // TODO: is there a more elegant way to
express this?
Maybe the factory should have a function to generate a random instance?  More
scalable, but it does spread the testing code over more files.

[TomH, 2012-05-09 19:39:39]

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

http://codereview.appspot.com/6199053/diff/1012/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:293: /// NULL; we need to have a complex ID system
here so that we can
On 2012/05/09 19:15:53, Stephen White wrote:
> Is this comment still valid?  Looks like you're doing comparisons now?

Done.

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

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrContext.cpp#newcode301
src/gpu/GrContext.cpp:301: new GrConvolutionEffect(direction, kernelWidth,
kernel));
On 2012/05/09 19:15:53, Stephen White wrote:
> <bikeshed>  Is it a stage, or an effect?  </bikeshed>

To anybody in Sk-land, it's an Effect; to anybody in Ganesh-internals, it's a
Stage.

Yeah, it's unclear, and it could be clearer, but we're in the process of
translating between two nomenclatures. A bunch of these may correspond directly
to classes in src/effects/, while at the end they're dropping into a single
Stage of a Gr(GL)Program.

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrProgramStageFactory.h
File src/gpu/GrProgramStageFactory.h (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/GrProgramStageFactory...
src/gpu/GrProgramStageFactory.h:11: #include <GrTypes.h>
On 2012/05/09 19:15:53, Stephen White wrote:
> Should be #include "GrTypes.h".

Done.

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

http://codereview.appspot.com/6199053/diff/1012/src/gpu/effects/GrConvolution...
src/gpu/effects/GrConvolutionEffect.cpp:9: #include "../gl/GrGLProgramStage.h"
On 2012/05/09 19:15:53, Stephen White wrote:
> I don't see many other places in skia where we use "..".  I think we generally
> add the to the base include path via -I, rather than using relative paths.

We introduced this when we broke a bunch of stuff off into gpu/gl/. Agreed that
it could be cleaned up.

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:16: #include "../GrProgramStageFactory.h"
On 2012/05/09 19:15:53, Stephen White wrote:
> unnecessary "../" (but not the first one in this file).

Actually, removing either of those causes a compilation failure for me.

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGpuGLShaders.cpp
File src/gpu/gl/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/6199053/diff/1012/src/gpu/gl/GrGpuGLShaders.cpp...
src/gpu/gl/GrGpuGLShaders.cpp:303: // TODO: is there a more elegant way to
express this?
On 2012/05/09 19:15:53, Stephen White wrote:
> Maybe the factory should have a function to generate a random instance?  More
> scalable, but it does spread the testing code over more files.

Yes, the instance or the factory should eventually have that. Brian/Rob/I
discussed earlier, didn't update the comment.

[bsalomon, 2012-05-09 21:12:07]

We should probably put src/gpu in the gpu target's -I list so that files in
src/gpu/gl and src/gpu/effects subdirs don't have to do "..". I do think there
is value in forcing the "gl/" on includes of headers living in src/gpu/gl into
files in src/gpu since it means the code-writer has to acknowledge that they're
bringing in gl-specific files. Same with include/gpu and include/gpu/gl.

I do agree with Stephen that effect/stage conflation is a bit confusing. Effect
may be better.. Stage implies a linear flow, which is our current but perhaps
not our future model. Personally, I'd be OK with a follow up change that
rectifies the naming both in the files added here and in previous changes.

[TomH, 2012-05-10 12:14:31]

On 2012/05/09 21:12:07, bsalomon wrote:
> We should probably put src/gpu in the gpu target's -I list so that files in
> src/gpu/gl and src/gpu/effects subdirs don't have to do "..". I do think there
> is value in forcing the "gl/" on includes of headers living in src/gpu/gl into
> files in src/gpu since it means the code-writer has to acknowledge that
they're
> bringing in gl-specific files. Same with include/gpu and include/gpu/gl.

Done.

Closed with r3887.

[TomH, 2012-05-10 13:36:22]

On 2012/05/10 12:14:31, TomH wrote:
> Closed with r3887.

... and Windows/Mac compiler errors fixed with r3889.