Remove uniform var pointer from custom effects

[bsalomon, 2012-07-13 14:29:19]

Introduces UniformHandle as a way for a GrGLProgramStage to hold onto a uniform.
The stages no longer hold dangling pointers.  The next steps are:

1) Make the calls to getUniformLocation automatic (eliminate initUniforms)

2) Make the uniform uploads go through a uniform manager that outlives the
GrGLShaderBuilder and lives in the GrGLProgram (changes the setData interface).

I also renamed pointers to GrGLShaderBuilder from state to builder.

No GMs have been harmed in development of this CL.

[TomH, 2012-07-13 20:38:57]

I like it enough overall to LGTM, although I had a few questions & ended up
reading it really slowly.

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
src/effects/SkLightingImageFilter.cpp:1231: void
GrGLLight::setupVariables(GrGLShaderBuilder* builder, int stage) {
I was going to say "why not just store names, rather than handles & do indirect
lookup of names?", but it looks like this actually mutates the variable?

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
src/effects/SkLightingImageFilter.cpp:1349: GR_GL_CALL_RET(gl, fSLocation,
GetUniformLocation(programID, s));
This is *such* a common pattern - could we write
int GrGLShaderBuilder::getUniformLocation(const GrGLInterface*,
uniform-handle-thingy, int programID)?

Then call
fSLocation = builder->getUniformLocation(gl, fSUni, programID);
instead of 2 lines?

Or is that just anticipating some of your declared future work?

http://codereview.appspot.com/6374067/diff/6001/src/gpu/effects/GrGradientEff...
File src/gpu/effects/GrGradientEffects.cpp (right):

http://codereview.appspot.com/6374067/diff/6001/src/gpu/effects/GrGradientEff...
src/gpu/effects/GrGradientEffects.cpp:204:
builder->getUniformVariable(fFSParamUni).appendArrayAccess(5, &p5);
Tangent: I never finished delivering the appendArrayAccess patch, we ended up
using the other approach for working around Xoom bugs; I think I ought to back
it out and go back to using [0] .. [5] for readability?

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

http://codereview.appspot.com/6374067/diff/6001/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:119: inline int
handle_to_index(GrGLShaderBuilder::UniformHandle h) { return ~h; }
Inverting them just to make them more opaque?

[bsalomon, 2012-07-13 20:43:50]

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
src/effects/SkLightingImageFilter.cpp:1231: void
GrGLLight::setupVariables(GrGLShaderBuilder* builder, int stage) {
On 2012/07/13 20:38:57, TomH wrote:
> I was going to say "why not just store names, rather than handles & do
indirect
> lookup of names?", but it looks like this actually mutates the variable?

They could store the names, but in a coming CL they will have to have the handle
anyway to do the update. The prog stages will no longer have a Location and
instead will update the uniform value through a Manager using the Handle.

http://codereview.appspot.com/6374067/diff/6001/src/effects/SkLightingImageFi...
src/effects/SkLightingImageFilter.cpp:1349: GR_GL_CALL_RET(gl, fSLocation,
GetUniformLocation(programID, s));
On 2012/07/13 20:38:57, TomH wrote:
> This is *such* a common pattern - could we write
> int GrGLShaderBuilder::getUniformLocation(const GrGLInterface*,
> uniform-handle-thingy, int programID)?
> 
> Then call
> fSLocation = builder->getUniformLocation(gl, fSUni, programID);
> instead of 2 lines?
> 
> Or is that just anticipating some of your declared future work?

All initUniforms() will be deleted in a coming CL. The manager will query the
location of all program uniforms in a loop.

http://codereview.appspot.com/6374067/diff/6001/src/gpu/effects/GrGradientEff...
File src/gpu/effects/GrGradientEffects.cpp (right):

http://codereview.appspot.com/6374067/diff/6001/src/gpu/effects/GrGradientEff...
src/gpu/effects/GrGradientEffects.cpp:204:
builder->getUniformVariable(fFSParamUni).appendArrayAccess(5, &p5);
On 2012/07/13 20:38:57, TomH wrote:
> Tangent: I never finished delivering the appendArrayAccess patch, we ended up
> using the other approach for working around Xoom bugs; I think I ought to back
> it out and go back to using [0] .. [5] for readability?

Prolly

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

http://codereview.appspot.com/6374067/diff/6001/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.cpp:119: inline int
handle_to_index(GrGLShaderBuilder::UniformHandle h) { return ~h; }
On 2012/07/13 20:38:57, TomH wrote:
> Inverting them just to make them more opaque?

Really it was just to make 0 be the invalid value. I'd be happy to change it to
not invert and use ~0 as the invalid value.

[bsalomon, 2012-07-16 14:45:26]

Landed as r4616, fixes at r4617&9