Add a requiresVertexShader method to GrGLEffect

Add a requiresVertexShader method to GrGLEffect

Adds requiresVertexShader to GrGLEffect and updates the necessary
effects to override it and return true. Also reworks GrGLProgram
and GrGLShaderBuilder so the program creates all the GL effects
at the beginning, and determines if it needs a vertex shader before
creating the shader builder.

Committed: http://code.google.com/p/skia/source/detail?r=11140

[Chris Dalton, 2013-09-06 05:05:21]

This change is a bit heftier, so I'm open to suggestions.

[bsalomon, 2013-09-06 14:10:03]

mostly looks good

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
File src/gpu/effects/GrSimpleTextureEffect.cpp (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
src/gpu/effects/GrSimpleTextureEffect.cpp:28: const GrSimpleTextureEffect& ste =
drawEffect.castEffect<GrSimpleTextureEffect>();
Maybe we should make defer this decision to the effect matrix

e.g.

return fEffectMatrix->requiresVertexShader();

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:464: SkTArray<GrDrawEffect>
colorDrawEffects(needColor ? fDesc.numColorEffects() : 0);
A couple small style things.. we try to wrap at 100 cols unless it makes the
code obscene, and bool operators go at the end of the line, e.g.:

if (a_really_long_condition ||
    another_condition) {

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:261: GrBackendEffectFactory::EffectKey const
effectKeys[],
what's with the const placement?

[Chris Dalton, 2013-09-06 17:17:24]

That's what I get for not reading the style guide :). I just went over it and
have a question:

Should we be using SK_OVERRIDE on the requiresVertexShader overrides?

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
File src/gpu/effects/GrSimpleTextureEffect.cpp (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
src/gpu/effects/GrSimpleTextureEffect.cpp:28: const GrSimpleTextureEffect& ste =
drawEffect.castEffect<GrSimpleTextureEffect>();
On 2013/09/06 14:10:03, bsalomon wrote:
> Maybe we should make defer this decision to the effect matrix
> 
> e.g.
> 
> return fEffectMatrix->requiresVertexShader();
> 

Actually, the way this code works is that when the coords type is
'kCustom_CoordsType', GrGLSimpleTextureEffect skips the effect matrix altogether
and outputs varyings directly to the vertex shader.

fEffectMatrix can always work without a vertex shader, because if there isn't
one, we're guaranteed that there aren't explicit local coords (we only go
vertexless if position is the only attribute).

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:464: SkTArray<GrDrawEffect>
colorDrawEffects(needColor ? fDesc.numColorEffects() : 0);
On 2013/09/06 14:10:03, bsalomon wrote:
> A couple small style things.. we try to wrap at 100 cols unless it makes the
> code obscene, and bool operators go at the end of the line, e.g.:
> 
> if (a_really_long_condition ||
>     another_condition) {

Will do.

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:261: GrBackendEffectFactory::EffectKey const
effectKeys[],
On 2013/09/06 14:10:03, bsalomon wrote:
> what's with the const placement?

For GrDrawEffect and GrBackendEffectFactory, it means the same thing on either
side of the type name. I just moved it to match GrGLEffect* (I'm going to move
it back tough).

For GrGLEffect*, this means the GrGLEffects (2 levels deep) are const:

  const GrGLEffect* glEffects[]

And this means the GrGLEffect* pointers (1 level deep) are const:

  GrGLEffect* const glEffects[]

We can't do the first way because it turns out C++ makes it illegal to promote a
'T**' to a 'const T**' (it creates a back door to modify const values). Thanks
for pointing it out though. We should really make both levels const:

  const GrGLEffect* const glEffects[]

[Chris Dalton, 2013-09-06 17:31:36]

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:261: GrBackendEffectFactory::EffectKey const
effectKeys[],
On 2013/09/06 17:17:24, cdalton wrote:
> On 2013/09/06 14:10:03, bsalomon wrote:
> > what's with the const placement?
> 
> For GrDrawEffect and GrBackendEffectFactory, it means the same thing on either
> side of the type name. I just moved it to match GrGLEffect* (I'm going to move
> it back tough).
> 
> For GrGLEffect*, this means the GrGLEffects (2 levels deep) are const:
> 
>   const GrGLEffect* glEffects[]
> 
> And this means the GrGLEffect* pointers (1 level deep) are const:
> 
>   GrGLEffect* const glEffects[]
> 
> We can't do the first way because it turns out C++ makes it illegal to promote
a
> 'T**' to a 'const T**' (it creates a back door to modify const values). Thanks
> for pointing it out though. We should really make both levels const:
> 
>   const GrGLEffect* const glEffects[]

Oh yeah, I forgot. The GrGLEffects can't be const because emitCode is non-const.
So we do need just make the GrGLEffects* pointers const:

GrGLEffect* const glEffects[]

But I'll still move the other consts back.

[bsalomon, 2013-09-06 17:37:17]

Yes, we should use SK_OVERRIDE.

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
File src/gpu/effects/GrSimpleTextureEffect.cpp (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/effects/GrSimpleTextu...
src/gpu/effects/GrSimpleTextureEffect.cpp:28: const GrSimpleTextureEffect& ste =
drawEffect.castEffect<GrSimpleTextureEffect>();
On 2013/09/06 17:17:24, cdalton wrote:
> On 2013/09/06 14:10:03, bsalomon wrote:
> > Maybe we should make defer this decision to the effect matrix
> > 
> > e.g.
> > 
> > return fEffectMatrix->requiresVertexShader();
> > 
> 
> Actually, the way this code works is that when the coords type is
> 'kCustom_CoordsType', GrGLSimpleTextureEffect skips the effect matrix
altogether
> and outputs varyings directly to the vertex shader.
> 
> fEffectMatrix can always work without a vertex shader, because if there isn't
> one, we're guaranteed that there aren't explicit local coords (we only go
> vertexless if position is the only attribute).

oh, right, makes sense

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23471008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:261: GrBackendEffectFactory::EffectKey const
effectKeys[],
On 2013/09/06 17:31:37, cdalton wrote:
> On 2013/09/06 17:17:24, cdalton wrote:
> > On 2013/09/06 14:10:03, bsalomon wrote:
> > > what's with the const placement?
> > 
> > For GrDrawEffect and GrBackendEffectFactory, it means the same thing on
either
> > side of the type name. I just moved it to match GrGLEffect* (I'm going to
move
> > it back tough).
> > 
> > For GrGLEffect*, this means the GrGLEffects (2 levels deep) are const:
> > 
> >   const GrGLEffect* glEffects[]
> > 
> > And this means the GrGLEffect* pointers (1 level deep) are const:
> > 
> >   GrGLEffect* const glEffects[]
> > 
> > We can't do the first way because it turns out C++ makes it illegal to
promote
> a
> > 'T**' to a 'const T**' (it creates a back door to modify const values).
Thanks
> > for pointing it out though. We should really make both levels const:
> > 
> >   const GrGLEffect* const glEffects[]
> 
> Oh yeah, I forgot. The GrGLEffects can't be const because emitCode is
non-const.
> So we do need just make the GrGLEffects* pointers const:
> 
> GrGLEffect* const glEffects[]
> 
> But I'll still move the other consts back.

Gotcha

[Chris Dalton, 2013-09-06 19:29:13]

All the comments should be addressed by now.

[bsalomon, 2013-09-06 19:44:01]

on microscopic style nit, otherwise lgtm.

https://codereview.chromium.org/23471008/diff/12001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/23471008/diff/12001/src/gpu/gl/GrGLProgram.cp...
src/gpu/gl/GrGLProgram.cpp:705: {
nit: this goes on prev line

[Chris Dalton, 2013-09-06 19:49:07]

I snuck in one quick rename so now the GrGLShaderBuilder ctor takes a boolean
called 'hasVertexShaderEffects' instead of 'requiresVertexShader'.

[commit-bot: I haz the power, 2013-09-06 19:49:55]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/23471008/31001

[commit-bot: I haz the power, 2013-09-06 20:20:04]

Change committed as 11140