Ganesh shader infrastructure: custom shader stages

GrCustomStage, GrGLFactory, and GrGLCustomStage base classes.

Adds a #include to GrAllocator to follow IWYU (include-what-you-use); otherwise it depends on other files being #included first.

[TomH, 2012-04-17 15:01:59]

Time to talk about nittygritty and TODOs?

[bsalomon, 2012-04-17 19:53:29]

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:23: virtual bool isOpaque(bool inputTextureIsOpaque)
const;
comment?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:14: inline GrGLShaderVar::Type
float_vector_type(int count) {
We have a GrGLSL.h that already has a bunch of helpers for generating GLSL. We
could consolidate these there.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:73: case kDefault_SamplerMode:
indent

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:97: case kDefault_SamplerMode:
indent

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:32: class GrGLStage {
ProgramStage maybe? Each instance of this guy is specific to a GL program (it
emits the code for the program and then manages updating uniform data for the
program).

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:49: virtual void setupVSUnis(VarArray& vsUnis, int
stage);
Do we want the setup*SUnis and emit*S to be separate steps? I'm wondering if it
makes implementing a subclass easier/clearer if you get to mix emitting code and
"declaring" uniforms.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:73: parameters? */
I don't understand the last part about forcing honesty.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:95: TODO: why don't we just merge this with
setData()? */
why not?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:109: GrStringBuilder
emitTextureSetup(GrStringBuilder* code,
Why is this public rather than protected?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:119: void emitTextureLookup(GrStringBuilder* code,
Do we need to force all texture lookups through here to enforce texture domains?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:131: class GrGLFactory {
Should we have a more specific name for this?

[TomH, 2012-04-18 14:40:33]

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:23: virtual bool isOpaque(bool inputTextureIsOpaque)
const;
On 2012/04/17 19:53:29, bsalomon wrote:
> comment?

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:14: inline GrGLShaderVar::Type
float_vector_type(int count) {
Started doing this. To avoid circular includes, it requires moving
GrGLShaderVar::Type to GrGLSL.h, which means enum names get significantly
longer: kFloat_GLShaderVarType. This has a pretty big footprint. Is this the
right thing to do, or am I missing an easier approach?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:73: case kDefault_SamplerMode:
On 2012/04/17 19:53:29, bsalomon wrote:
> indent

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:97: case kDefault_SamplerMode:
On 2012/04/17 19:53:29, bsalomon wrote:
> indent

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:32: class GrGLStage {
Done.
(Renamed .cpp and .h files; assuming we want to leave the Factory in this file.)

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:49: virtual void setupVSUnis(VarArray& vsUnis, int
stage);
I want to wait for more evidence before making that change. Once we have 5?
effects I think the tradeoffs will be a little clearer.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:73: parameters? */
We've had verbal conversations about not trusting implementors to respect
TextureDomain; the TODO is (as I understood it) what you'd suggested. Not sure
how important i really is.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:95: TODO: why don't we just merge this with
setData()? */
On 2012/04/17 19:53:29, bsalomon wrote:
> why not?

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:109: GrStringBuilder
emitTextureSetup(GrStringBuilder* code,
On 2012/04/17 19:53:29, bsalomon wrote:
> Why is this public rather than protected?

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:119: void emitTextureLookup(GrStringBuilder* code,
On 2012/04/17 19:53:29, bsalomon wrote:
> Do we need to force all texture lookups through here to enforce texture
domains?

I think that was our thought, and the reason we'd talked about only exposing an
opaque texture handle rather than the sampler name to the emit*() functions.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:131: class GrGLFactory {
Done.
(GrGLProgramStageFactory)

[bsalomon, 2012-04-18 14:52:47]

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:14: inline GrGLShaderVar::Type
float_vector_type(int count) {
On 2012/04/18 14:40:34, TomH wrote:
> Started doing this. To avoid circular includes, it requires moving
> GrGLShaderVar::Type to GrGLSL.h, which means enum names get significantly
> longer: kFloat_GLShaderVarType. This has a pretty big footprint. Is this the
> right thing to do, or am I missing an easier approach?

kFloat_GLSLType?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:49: virtual void setupVSUnis(VarArray& vsUnis, int
stage);
On 2012/04/18 14:40:34, TomH wrote:
> I want to wait for more evidence before making that change. Once we have 5?
> effects I think the tradeoffs will be a little clearer.

SGTM

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:73: parameters? */
On 2012/04/18 14:40:34, TomH wrote:
> We've had verbal conversations about not trusting implementors to respect
> TextureDomain; the TODO is (as I understood it) what you'd suggested. Not sure
> how important i really is.

Got it. I think it'd be a nice follow up change. Otherwise we may find ourselves
writing "you aren't respecting the texture domain" in future code reviews.

[robertphillips, 2012-04-18 15:08:47]

Nits

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:16: class GrCustomStage {
Derive from GrNoncopyable?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:21: GrCustomStage();
Should we add a virtual destructor now to set up for a lot of derived classes?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:74: case kProj_SamplerMode:
Don't we need a "fall through" indicator?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:135: virtual ~GrGLFactory (void);
We don't seem to use void parameters any where else?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:150: GrGLFactory () { }
private?

[TomH, 2012-04-18 15:49:08]

Brian's review & Robert's nits answered, for the most part.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:16: class GrCustomStage {
Why?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:21: GrCustomStage();
On 2012/04/18 15:08:47, robertphillips wrote:
> Should we add a virtual destructor now to set up for a lot of derived classes?

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:74: case kProj_SamplerMode:
On 2012/04/18 15:08:47, robertphillips wrote:
> Don't we need a "fall through" indicator?

Done.
(I'm not used to using them when there's *zero* content in a case and it falls
through directly, since the next case is on the next line, but I suppose it
doesn't hurt to be explicit.)

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:135: virtual ~GrGLFactory (void);
On 2012/04/18 15:08:47, robertphillips wrote:
> We don't seem to use void parameters any where else?

Done.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:150: GrGLFactory () { }
On 2012/04/18 15:08:47, robertphillips wrote:
> private?

Making all constructors private disables subclasses.
I suppose the answer is that this is unnecessary, and it doesn't matter what we
do with the constructor here, we can't constrain subclasses to be singletons?

[robertphillips, 2012-04-18 15:56:57]

LGTM

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:16: class GrCustomStage {
Mainly paranoia on my part. I usually disallow the copy constructor and
assignment operator unless they proven to be required.

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h
File src/gpu/gl/GrGLCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.h#...
src/gpu/gl/GrGLCustomStage.h:150: GrGLFactory () { }
You're right - we have to allow some form of construction for the derived
classes.

[bsalomon, 2012-04-18 16:29:08]

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h
File src/gpu/GrCustomStage.h (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/GrCustomStage.h#newco...
src/gpu/GrCustomStage.h:16: class GrCustomStage {
On 2012/04/18 15:49:08, TomH wrote:
> Why?

Semi-related: should this be ref counted?

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cpp
File src/gpu/gl/GrGLCustomStage.cpp (right):

http://codereview.appspot.com/6052047/diff/1007/src/gpu/gl/GrGLCustomStage.cp...
src/gpu/gl/GrGLCustomStage.cpp:74: case kProj_SamplerMode:
On 2012/04/18 15:49:08, TomH wrote:
> On 2012/04/18 15:08:47, robertphillips wrote:
> > Don't we need a "fall through" indicator?
> 
> Done.
> (I'm not used to using them when there's *zero* content in a case and it falls
> through directly, since the next case is on the next line, but I suppose it
> doesn't hurt to be explicit.)

I think it'd be OK to modify the style guide to say the comment is only
necessary when there is a statement between the cases (not that it hurts to have
it anyway).

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

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:91: return
GrGLShaderVar::TypeString(GrGLSLFloatVectorType(count));
Should this be GrSLFloatVectorType since the returned enum is SL no GLSL?

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp
File src/gpu/gl/GrGLSL.cpp (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp#newcode84
src/gpu/gl/GrGLSL.cpp:84: GrSLType GrGLSLFloatVectorType (int count) {
Is it worth inlining these in the header? they're pretty trivial, though shader
generation is not fast anyway.

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h#newcode33
src/gpu/gl/GrGLSL.h:33: * Types of GrGLShaderVar variables we can create.
Shader language types?

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h
File src/gpu/gl/GrGLShaderVar.h (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h#ne...
src/gpu/gl/GrGLShaderVar.h:24: /*
kill it! kill it!

[TomH, 2012-04-18 17:37:21]

Added comment "TODO: may want to refcount these" to GrCustomStage.

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

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:91: return
GrGLShaderVar::TypeString(GrGLSLFloatVectorType(count));
On 2012/04/18 16:29:08, bsalomon wrote:
> Should this be GrSLFloatVectorType since the returned enum is SL no GLSL?

Done.

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp
File src/gpu/gl/GrGLSL.cpp (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp#newcode84
src/gpu/gl/GrGLSL.cpp:84: GrSLType GrGLSLFloatVectorType (int count) {
On 2012/04/18 16:29:08, bsalomon wrote:
> Is it worth inlining these in the header? they're pretty trivial, though
shader
> generation is not fast anyway.

I steered away from it for the two that return const char* because of the static
char arrays that would be replicated. You're right that they're trivial, but
they're also little-used and not in inner loops, and it seems to me the
sacrifice in readability & maintainability & compile speed isn't really worth
the change until we see them show up in profiles.

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h#newcode33
src/gpu/gl/GrGLSL.h:33: * Types of GrGLShaderVar variables we can create.
On 2012/04/18 16:29:08, bsalomon wrote:
> Shader language types?

Done.

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h
File src/gpu/gl/GrGLShaderVar.h (right):

http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h#ne...
src/gpu/gl/GrGLShaderVar.h:24: /*
On 2012/04/18 16:29:08, bsalomon wrote:
> kill it! kill it!
Oops. Terminated with prejudice.

[bsalomon, 2012-04-18 17:40:58]

On 2012/04/18 17:37:21, TomH wrote:
> Added comment "TODO: may want to refcount these" to GrCustomStage.
> 
> http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLProgram.cpp
> File src/gpu/gl/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLProgram.cpp#ne...
> src/gpu/gl/GrGLProgram.cpp:91: return
> GrGLShaderVar::TypeString(GrGLSLFloatVectorType(count));
> On 2012/04/18 16:29:08, bsalomon wrote:
> > Should this be GrSLFloatVectorType since the returned enum is SL no GLSL?
> 
> Done.
> 
> http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp
> File src/gpu/gl/GrGLSL.cpp (right):
> 
>
http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.cpp#newcode84
> src/gpu/gl/GrGLSL.cpp:84: GrSLType GrGLSLFloatVectorType (int count) {
> On 2012/04/18 16:29:08, bsalomon wrote:
> > Is it worth inlining these in the header? they're pretty trivial, though
> shader
> > generation is not fast anyway.
> 
> I steered away from it for the two that return const char* because of the
static
> char arrays that would be replicated. You're right that they're trivial, but
> they're also little-used and not in inner loops, and it seems to me the
> sacrifice in readability & maintainability & compile speed isn't really worth
> the change until we see them show up in profiles.
> 
> http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h
> File src/gpu/gl/GrGLSL.h (right):
> 
> http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLSL.h#newcode33
> src/gpu/gl/GrGLSL.h:33: * Types of GrGLShaderVar variables we can create.
> On 2012/04/18 16:29:08, bsalomon wrote:
> > Shader language types?
> 
> Done.
> 
> http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h
> File src/gpu/gl/GrGLShaderVar.h (right):
> 
>
http://codereview.appspot.com/6052047/diff/4003/src/gpu/gl/GrGLShaderVar.h#ne...
> src/gpu/gl/GrGLShaderVar.h:24: /*
> On 2012/04/18 16:29:08, bsalomon wrote:
> > kill it! kill it!
> Oops. Terminated with prejudice.

LGTM

[TomH, 2012-04-18 18:17:04]

Committed in r3726.