[GPU] Use new Var type for inputs/outputs of FS and VS

This change makes us gather all the inputs / outputs of the FS and VS using a new type, Var, and defers generating the declarations until the end. At that time the GLSL version is used to determine how they are declared.

[junov1, 2011-09-19 20:30:48]

In the shader code segments, could we get rid of VSAttrs and VSUnis, and just
have VSInputs.  Let Var handle GR_GL_ATTRIBUTE_MATRICES

[junov1, 2011-09-19 20:40:14]

On 2011/09/19 20:30:48, junov1 wrote:
> In the shader code segments, could we get rid of VSAttrs and VSUnis, and just
> have VSInputs.  Let Var handle GR_GL_ATTRIBUTE_MATRICES

Actually, I just realized there is stuff that explicitly needs to be Attrs. So
maybe when parsing the unis, some could just get auto-switched to attrs.  The
switch would be based on the same build flag we have now, but could be dynamic
in the future, and could apply to more than just matrices.

[bsalomon, 2011-09-19 20:47:57]

On 2011/09/19 20:40:14, junov1 wrote:
> On 2011/09/19 20:30:48, junov1 wrote:
> > In the shader code segments, could we get rid of VSAttrs and VSUnis, and
just
> > have VSInputs.  Let Var handle GR_GL_ATTRIBUTE_MATRICES
> 
> Actually, I just realized there is stuff that explicitly needs to be Attrs. So
> maybe when parsing the unis, some could just get auto-switched to attrs.  The
> switch would be based on the same build flag we have now, but could be dynamic
> in the future, and could apply to more than just matrices.

I thought about adding a field to Var that specifies whether it is an "in",
"out", or "uniform" but decided to hold back on that for now.

BTW GR_GL_ATTRIBUTE_MATRICES doesn't work as well as it used to because we can
hit the attribute limit with some of our shaders. If it still is performant on
some powervr gpu/drivers we'll have to make it aware of how many attribute
locations are available and spill matrices that don't fit in attribute slots to
uniforms.

[junov1, 2011-09-19 21:08:36]

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:178: typedef SkTArray<Var> VarArray;
How about this:
class VarArray : public SkTArray<Var> {
    void addVar(...) {push_back().set(...);}
    void expandDecls(...)

private:
    static const char* varyingPrefix(...);
    static const char* attributePrefix(...);

};

Also, I think this file is getting big enough that it would make sense to factor
out Var/VarArray into a separate file. This code might even be easily reusable
by an eventual DX implementation.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.h#newcode39
gpu/src/GrGLProgram.h:39: static GLSLVersion GetGLSLVersion(GrGLBinding binding,
float glVersion);
I think this should go in GrGpuGL

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:278: this->glVersion() >= 3.3f ||
Floating point comparison... danger!
Perhaps comparing to (3.3f - epsilon) would be safer?

[Stephen White, 2011-09-19 22:01:59]

This looks ok.  I'm a little concerned that this level of refactoring is
resulting in more code rather than less, although I do see the need for it WRT
the GLSL 1.5 issue.  Consider my comments as non-binding, and really more style
than substance.

LGTM.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:75: class Var {
Non-gating suggestions:  Should this be in its own .h?  If so, it should
probably also be renamed to something that reflects its GL-isms (GLVar? 
GLSLVar?).

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:95: void set(Type type,
Should these be constructors?  Or do Vars really need the ability to be mutated
from one type to another?  Hmm, I see below that there's lots of
push_back().set(...).  I guess you need it for that idiom.  I suppose I'd prefer
something like push_back(Var(...)) although that'd require a copy cons.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:174: bool            fSpecifyPrecision;
Naming thing:  fSpecifyPrecision at first glance is a little confusing as to its
function.  Maybe fEmitPrecisionSpecifier? fEmitsPrecisionSpecifier? 
fGeneratePrecisionSpecifier?  (the latter is kind of verbose, though).

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:216: return (Type)(count - 1);
Should there still be some kind of runtime assert on count?

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:1386: // may have relocated @ 2nd push_back
This is another spot where constructing the vars before pushing them would avoid
this kind of trickiness.

[bsalomon, 2011-09-20 14:39:21]

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:75: class Var {
On 2011/09/19 22:01:59, Stephen White wrote:
> Non-gating suggestions:  Should this be in its own .h?  If so, it should
> probably also be renamed to something that reflects its GL-isms (GLVar? 
> GLSLVar?).

Done.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:95: void set(Type type,
On 2011/09/19 22:01:59, Stephen White wrote:
> Should these be constructors?  Or do Vars really need the ability to be
mutated
> from one type to another?  Hmm, I see below that there's lots of
> push_back().set(...).  I guess you need it for that idiom.  I suppose I'd
prefer
> something like push_back(Var(...)) although that'd require a copy cons.

I kind of like the push_back() to construct the name string without forcing a
copy. Maybe its an example of early optimization but I don't think its too ugly
:)

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:174: bool            fSpecifyPrecision;
On 2011/09/19 22:01:59, Stephen White wrote:
> Naming thing:  fSpecifyPrecision at first glance is a little confusing as to
its
> function.  Maybe fEmitPrecisionSpecifier? fEmitsPrecisionSpecifier? 
> fGeneratePrecisionSpecifier?  (the latter is kind of verbose, though).

I think you're right that this isn't a good name. I changed it to
fEmitPrecision, not quite as long as fEmitPrecisionSpecifier but still clearer
(hopefully).

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:178: typedef SkTArray<Var> VarArray;
On 2011/09/19 21:08:37, junov1 wrote:
> How about this:
> class VarArray : public SkTArray<Var> {
>     void addVar(...) {push_back().set(...);}
>     void expandDecls(...)
> 
> private:
>     static const char* varyingPrefix(...);
>     static const char* attributePrefix(...);
> 
> };
> 
> Also, I think this file is getting big enough that it would make sense to
factor
> out Var/VarArray into a separate file. This code might even be easily reusable
> by an eventual DX implementation.

I moved it to GrGLShaderVar.h (and changed the name). When a DX backend is
written we can decide whether it makes sense to subclass or share an
implementation.

I don't think I want to commit too much to having arrays of these things. It may
make sense in the future to lump all the inputs into a single collection and
decide in a post-process which should be uniforms vs. attributes passed through
as varyings if needed in a later stage.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:216: return (Type)(count - 1);
On 2011/09/19 22:01:59, Stephen White wrote:
> Should there still be some kind of runtime assert on count?

Done.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.cpp#newco...
gpu/src/GrGLProgram.cpp:1386: // may have relocated @ 2nd push_back
On 2011/09/19 22:01:59, Stephen White wrote:
> This is another spot where constructing the vars before pushing them would
avoid
> this kind of trickiness.

I changed the VarArray type to be a GrAllocator which creates fixed sized blocks
and doesn't ever move previously added elements.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGLProgram.h#newcode39
gpu/src/GrGLProgram.h:39: static GLSLVersion GetGLSLVersion(GrGLBinding binding,
float glVersion);
On 2011/09/19 21:08:37, junov1 wrote:
> I think this should go in GrGpuGL

Done.

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/5056048/diff/3006/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:278: this->glVersion() >= 3.3f ||
On 2011/09/19 21:08:37, junov1 wrote:
> Floating point comparison... danger!
> Perhaps comparing to (3.3f - epsilon) would be safer?

OpenGL has 2 digit version numbers, well within floating point precision. It
would make more sense to represent the GL version as a fixed point, though.
Filed issue 367 for this.

[junov1, 2011-09-20 15:53:20]

> OpenGL has 2 digit version numbers, well within floating point precision. It
> would make more sense to represent the GL version as a fixed point, though.
> Filed issue 367 for this.

Even with just two digits, the float representation may not be able to represent
3.3 exactly.  Suppose the runtime conversion yielded 3.29999999, while the
compile time conversion of the literal gave 3.30000001, you'd be screwed.  My
main worry is that the way the test is currently written, it might unjustly fail
on certain models of CPU that implement fp rounding differently.  If we know
there is always only 1 digit after the decimal point, then the comparison could
be this->glVersion() > 3.25f

[Stephen White, 2011-09-20 15:58:30]

LGTM.

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:43: : fVSUnis(8)
Nit:  these integer constants seem kind of "magic".

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:127: inline void tex_matrix_name(int stage,
GrStringBuilder* s) {
Nit:  shouldn't these functions still be static as well?  Or are they now needed
outside this file?

[bsalomon, 2011-09-20 17:21:48]

>Even with just two digits, the float representation may not be able to
represent
>3.3 exactly.  Suppose the runtime conversion yielded 3.29999999, while the
>compile time conversion of the literal gave 3.30000001, you'd be screwed.  My
>main worry is that the way the test is currently written, it might unjustly
fail
>on certain models of CPU that implement fp rounding differently.  If we know
>there is always only 1 digit after the decimal point, then the comparison could
>be this->glVersion() > 3.25f

Got it. That's a great observation. I'll make it 3.25 for now and resolving
Issue 367 will be a better long term fix.

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:43: : fVSUnis(8)
On 2011/09/20 15:58:30, Stephen White wrote:
> Nit:  these integer constants seem kind of "magic".

I'll stick a comment in there.

http://codereview.appspot.com/5056048/diff/27002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:127: inline void tex_matrix_name(int stage,
GrStringBuilder* s) {
On 2011/09/20 15:58:30, Stephen White wrote:
> Nit:  shouldn't these functions still be static as well?  Or are they now
needed
> outside this file?

I put them in an anonymous namespace.

[bsalomon, 2011-09-21 15:16:34]

Closed with r2291