Wrap uniform creation into a single function

[TomH, 2012-05-25 14:02:54]

Brian, is this the kind of approach you were suggesting in
http://codereview.appspot.com/6214056/?

[bsalomon, 2012-05-25 14:17:06]

Overall, yes (but see comment about precision qualifiers). 

Also, should we return const GrGLShaderVar& ?

http://codereview.appspot.com/6248050/diff/3001/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

http://codereview.appspot.com/6248050/diff/3001/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:191: // and not on the copies that end up in
the FS?
Here's the deal:

ES2 requires that all floats in the FS have a precision qualifier -or- that a
default precision qualifier is established for floats. We do the latter by
declaring "precision mediump float;" in the FS. So all the float uniforms in our
FSs have mediump precision (highp is not guaranteed to be supported in FSs).

The VS implicitly has a "precision highp float;" declaration. So any unqualified
floats in the VS are highp. But if a uniform is shared between VS and FS it must
have the same precision qualifier in both. So, when we have a uniform shared
between FS and VS we add "mediump" to its declaration in the VS.

I think this should just be automatic in this function (no emitPrecisionParam).
If the uniform is both FS and VS then the VS version gets the precision
qualifier.

[TomH, 2012-05-25 14:31:08]

All the call sites I've looked at want GrGLShaderVar*; that's what we store most
places. Why would we return const & instead?

Thanks for the emitPrecision lesson.

http://codereview.appspot.com/6248050/diff/3001/src/gpu/gl/GrGLShaderBuilder.cpp
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

http://codereview.appspot.com/6248050/diff/3001/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:191: // and not on the copies that end up in
the FS?
On 2012/05/25 14:17:06, bsalomon wrote:
> Here's the deal:
> 
> ES2 requires that all floats in the FS have a precision qualifier -or- that a
> default precision qualifier is established for floats. We do the latter by
> declaring "precision mediump float;" in the FS. So all the float uniforms in
our
> FSs have mediump precision (highp is not guaranteed to be supported in FSs).
> 
> The VS implicitly has a "precision highp float;" declaration. So any
unqualified
> floats in the VS are highp. But if a uniform is shared between VS and FS it
must
> have the same precision qualifier in both. So, when we have a uniform shared
> between FS and VS we add "mediump" to its declaration in the VS.
> 
> I think this should just be automatic in this function (no
emitPrecisionParam).
> If the uniform is both FS and VS then the VS version gets the precision
> qualifier.
> 

Done.

[bsalomon, 2012-05-25 14:40:57]

On 2012/05/25 14:31:08, TomH wrote:
> All the call sites I've looked at want GrGLShaderVar*; that's what we store
most
> places. Why would we return const & instead?
>

Why should the caller of this function get to change the returned object? If
GrGLShaderBuilder is to be the central planner of GrCustomStages it can't let
the peons decide their own uniforms' var names, precisions, etc.

[TomH, 2012-05-30 16:14:10]

OK, now has constness & is used throughout GrGLProgram.

I'm not *happy* about GrGLProgram, because it has all its own tiny convenience
functions to generate name + stage# so that we can be consistent between
creating the uniform & getUniformLocationsAndInitCache(). addUniform() also
knows how to do that in a general way, but we don't use the general
functionality in order to guarantee that the name created matches gULAIC().

Crazy idea: augment/replace StageUniLocations with pointers to the
GrGLShaderVars; instead of checking whether they're kUseUniform, check whether
they're non-null. I'm not convinced that's significantly better.

Follow-up CL will change GrGLProgramStage API to use addUniform().

[bsalomon, 2012-05-30 17:07:00]

Isn't StageUniLocations on it's deathbed? Aside from possibly the texture domain
I don't see anything that won't be put in an effect.

http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder...
src/gpu/gl/GrGLShaderBuilder.h:56: /* TODO: can't arbitrarily OR together enum
components, so
We have a macro, GR_MAKE_BITFIELD_OPS, that declares bitwise operation operators
for an enum type.

http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder...
src/gpu/gl/GrGLShaderBuilder.h:70: const GrGLShaderVar&
addUniform(VariableLifetime lifetime,
Can the builder know what stage we're on and append it automatically?

[TomH, 2012-05-30 17:10:52]

On 2012/05/30 17:07:00, bsalomon wrote:

>
http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder...
> src/gpu/gl/GrGLShaderBuilder.h:70: const GrGLShaderVar&
> addUniform(VariableLifetime lifetime,
> Can the builder know what stage we're on and append it automatically?

No! It's frequently being used for variables that don't carry around the stage
number.
  genInputColor()
  genUniformCoverage()
  VIEW_MATRIX_NAME
  COLOR_FILTER_UNI_NAME
  ...

Also, with custom stages being enclosed in {} we need it less (not at all?)

All of which is to say, it probably *could*, but I'm trying to reduce its
incidence instead of increase it.

[bsalomon, 2012-05-30 17:18:29]

On 2012/05/30 17:10:52, TomH wrote:
> On 2012/05/30 17:07:00, bsalomon wrote:
> 
> >
>
http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder...
> > src/gpu/gl/GrGLShaderBuilder.h:70: const GrGLShaderVar&
> > addUniform(VariableLifetime lifetime,
> > Can the builder know what stage we're on and append it automatically?
> 
> No! It's frequently being used for variables that don't carry around the stage
> number.
>   genInputColor()
>   genUniformCoverage()
>   VIEW_MATRIX_NAME
>   COLOR_FILTER_UNI_NAME
>   ...

Ok for now, but I suspect all of these but the view matrix may become stages.
Another option is to make a special value, -1, for the uniforms used by the glue
code that don't need a stage number appended.

> 
> Also, with custom stages being enclosed in {} we need it less (not at all?)

No, the uniforms have global scope.

> 
> All of which is to say, it probably *could*, but I'm trying to reduce its
> incidence instead of increase it.

[bsalomon, 2012-05-30 17:32:48]

LGTM

On 2012/05/30 17:18:29, bsalomon wrote:
> On 2012/05/30 17:10:52, TomH wrote:
> > On 2012/05/30 17:07:00, bsalomon wrote:
> > 
> > >
> >
>
http://codereview.appspot.com/6248050/diff/10002/src/gpu/gl/GrGLShaderBuilder...
> > > src/gpu/gl/GrGLShaderBuilder.h:70: const GrGLShaderVar&
> > > addUniform(VariableLifetime lifetime,
> > > Can the builder know what stage we're on and append it automatically?
> > 
> > No! It's frequently being used for variables that don't carry around the
stage
> > number.
> >   genInputColor()
> >   genUniformCoverage()
> >   VIEW_MATRIX_NAME
> >   COLOR_FILTER_UNI_NAME
> >   ...
> 
> Ok for now, but I suspect all of these but the view matrix may become stages.
> Another option is to make a special value, -1, for the uniforms used by the
glue
> code that don't need a stage number appended.
> 
> > 
> > Also, with custom stages being enclosed in {} we need it less (not at all?)
> 
> No, the uniforms have global scope.
> 
> > 
> > All of which is to say, it probably *could*, but I'm trying to reduce its
> > incidence instead of increase it.

[TomH, 2012-05-30 17:39:22]

Closed with r4081.