Introduce GrGLShaderBuilder::TextureSampler

[bsalomon, 2012-09-06 19:03:46]

The point of this change is to tunnel GrTextureAccess from GrCustomStage to
GrGLProgramStage. Right now color table effect is the only effect type using
GrTextureAccess. It makes its (long-lived) GrGLProgramStage keep a reference to
a (short-lived) GrCustomStage in order to get at the GrTextureAccess owned by
the GrCustomStage. This is dangerous since the GrGLProgramStage can outlive the
GrCustomStage. Also it is a bit confusing since via the program cache
GrGLProgramStages are reused with multiple GrCustomStages (that have the same
stage key).

The rest of the stage subclass changes are simple and are due to virtual
emitFS() now taking a TextureSampler rather than a char* samplerName. 

The follow up CL will determine a default GrTextureAccess in GenStageCode for
stages that don't currently provide them, eliminate GrGLShaderBuilder::fSwizzle,
and consolidate the custom/non-custom texture lookup functions in
GrGLShaderBuilder. Then a yet later change will make the stage subclasses
provide their own GrTextureAccesses.

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.h#newcod...
src/gpu/gl/GrGLProgram.h:214: static GrGLProgramStage* GenStageCode(const
GrCustomStage* stage,
Soon StageDesc will not exist. It only has InConfigFlags which I plan to remove
ASAP and some flags about the texture matrix. The flags will go away when stages
are responsible for managing their own tex coords / matrices.

I think this function really belongs on the GrGLShaderBuilder.

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgramStage.h
File src/gpu/gl/GrGLProgramStage.h (left):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgramStage.h#o...
src/gpu/gl/GrGLProgramStage.h:28: TODO: lifetime management.
I removed this comment because I think what we're doing today is fine. We don't
ref count these because only GrGLProgram ever owns them and it just deletes them
in its ~.

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h#...
src/gpu/gl/GrGLShaderBuilder.h:65: // TODO: Remove once GrTextureAccess is
required.
Will happen in next CL.

[robertphillips, 2012-09-06 19:42:05]

LGTM + 1 nit & 1 question

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.cpp#newc...
src/gpu/gl/GrGLProgram.cpp:970: const GrTextureAccess* access =
stage->textureAccess(i);
NULL !=?

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h#...
src/gpu/gl/GrGLShaderBuilder.h:40: 
why not fTexture? It seems like this is preserving the fTexture from the old
object which might be a little confusing.

[bsalomon, 2012-09-06 19:59:33]

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgram.cpp#newc...
src/gpu/gl/GrGLProgram.cpp:970: const GrTextureAccess* access =
stage->textureAccess(i);
On 2012/09/06 19:42:06, robertphillips wrote:
> NULL !=?

will change.

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLShaderBuilder.h#...
src/gpu/gl/GrGLShaderBuilder.h:40: 
On 2012/09/06 19:42:06, robertphillips wrote:
> why not fTexture? It seems like this is preserving the fTexture from the old
> object which might be a little confusing.

Good point, I added fTexture after writing this function... will fix.

[TomH, 2012-09-07 09:42:57]

https://codereview.appspot.com/6495099/diff/1/src/gpu/effects/GrColorTableEff...
File src/gpu/effects/GrColorTableEffect.cpp (left):

https://codereview.appspot.com/6495099/diff/1/src/gpu/effects/GrColorTableEff...
src/gpu/effects/GrColorTableEffect.cpp:37: const GrCustomStage& fCustomStage;
Smells like progress.

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgramStage.h
File src/gpu/gl/GrGLProgramStage.h (left):

https://codereview.appspot.com/6495099/diff/1/src/gpu/gl/GrGLProgramStage.h#o...
src/gpu/gl/GrGLProgramStage.h:28: TODO: lifetime management.
Agreed. When I first wrote the comment, we didn't have an agreement on how we'd
manage lifetimes, let alone an implementation; it just got stale.

[bsalomon, 2012-09-07 13:55:11]

Closed with r5422.