Added API to query for active attribs and uniforms. These functions are model...

Added API to query for active attribs and uniforms. These functions are modeled after glGetShaderiv, glGetProgramiv, glGetActiveAttrib, and glGetActiveUniform. The main difference between this and OpenGL API is that we do not have programs - just shaders.
BUG=26

[Alok Priyadarshi, 2010-09-14 01:53:40]

Please review the API. I will have further work to do once the API is firm.

[gmanchromium, 2010-09-14 16:17:09]

Personally I'd prefer it just hand me some struct but I can see the advantage of
copying the OpenGL API since it means people can just use their existing code
and change a few things.

But, if that's the goal then it would be nice in the various constants matched
GL so I don't have to add lots of code to convert from compiler constants to GL
constants or visa versa

Just a suggestion. Otherwise LGTM

[kbr1, 2010-09-14 17:14:04]

The API looks fine; one comment on precise documentation of null terminators and
buffer lengths in all of the new entry points.

http://codereview.appspot.com/2183041/diff/1/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/2183041/diff/1/include/GLSLANG/ShaderLang.h#new...
include/GLSLANG/ShaderLang.h:158: //          ShGetInfo with
EShInfoInfoLogLength.
It would be helpful to document that the returned string is null terminated and
that "the size of the buffer required to store the returned information log,
including the null terminator, can be obtained...". Here and below.

[Alok Priyadarshi, 2010-09-14 17:31:03]

> But, if that's the goal then it would be nice in the various constants matched
> GL so I don't have to add lots of code to convert from compiler constants to
GL
> constants or visa versa

As I was defining various enums, I was thinking about this issue. I can think of
two ways to do this:
1. Include opengl headers in ShaderLang.h. I have avoided doing this so far to
keep it independent.
2. Define things like SH_FLOAT, SH_FLOAT_VEC3, etc. that have the same values as
GL_FLOAT, GL_FLOAT_VEC3. I am not sure of how difficult it would be to maintain.
Do values for OpenGL enums change?

Any other suggestion on how to make it more convenient to use?

[Alok Priyadarshi, 2010-09-14 17:52:20]

Added more comments about null-terminated strings.

http://codereview.appspot.com/2183041/diff/1/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/2183041/diff/1/include/GLSLANG/ShaderLang.h#new...
include/GLSLANG/ShaderLang.h:158: //          ShGetInfo with
EShInfoInfoLogLength.
On 2010/09/14 17:14:04, kbr1 wrote:
> It would be helpful to document that the returned string is null terminated
and
> that "the size of the buffer required to store the returned information log,
> including the null terminator, can be obtained...". Here and below.
> 

Done.

[kbr1, 2010-09-14 18:05:43]

On 2010/09/14 17:31:03, alokp wrote:
> > But, if that's the goal then it would be nice in the various constants
matched
> > GL so I don't have to add lots of code to convert from compiler constants to
> GL
> > constants or visa versa
> 
> As I was defining various enums, I was thinking about this issue. I can think
of
> two ways to do this:
> 1. Include opengl headers in ShaderLang.h. I have avoided doing this so far to
> keep it independent.
> 2. Define things like SH_FLOAT, SH_FLOAT_VEC3, etc. that have the same values
as
> GL_FLOAT, GL_FLOAT_VEC3. I am not sure of how difficult it would be to
maintain.
> Do values for OpenGL enums change?

(1) is not workable; the OpenGL headers have all sorts of dependencies and will
interfere with code using the shader validator.

(2) should work. These enum values have not changed in years. You'd want to
document that the returned types use precisely the same enum values as in OpenGL
ES 2.0.

[dgkoch, 2010-09-14 18:07:47]

On 2010/09/14 17:31:03, alokp wrote:
> Do values for OpenGL enums change?
No the enum values can never change (although there may be the odd case where
they may be different between OpenGL and OpenGL ES)

Stepping back a bit, I think I'm missing some context -- What is the purpose of
this API ?

If the goal it to ensure more consistency between implementations, I'm not sure
how this is going to achieve it.  The "activeness" of attributes and uniforms is
inherently compiler (and linker) dependent.  

In particular for GetActiveAttrib, the spec states: "A generic attribute
variable is considered active if it is determined by the compiler and linker
that the attribute may be accessed when the shader is executed. Attribute
variables that are declared in a vertex shader but never used are not con-
sidered active. In cases where the compiler and linker cannot make a conclusive
determination, an attribute will be considered active. "  

Similarly for Uniforms, the spec says: "A uniform is considered active if it is
determined by the compiler and linker that the uniform will actually be accessed
when the executable code is executed. In cases where the compiler and linker
cannot make a conclusive determination, the uniform will be considered active."

Thus, if one implementation has a better optimizer, or smarts, then another, you
could end up with different information.  Furthermore, in both cases the spec
states that the program must have had LinkProgram called on it in the past, and
that they operation on _programs_ and not _shaders_. 

I'm quite certain that there are implementations which do "whole program"
optimizations and factor in both stages into the optimization.  This allows them
to remove variables and entire calculations from the vertex shader if the output
from them is never used by the pixel shader.   This could certainly result in
different "activeness" for uniforms and possibly even attributes.

Daniel

[Alok Priyadarshi, 2010-09-14 18:26:25]

On 2010/09/14 18:07:47, dgkoch wrote:
> On 2010/09/14 17:31:03, alokp wrote:
> > Do values for OpenGL enums change?
> No the enum values can never change (although there may be the odd case where
> they may be different between OpenGL and OpenGL ES)
> 
> Stepping back a bit, I think I'm missing some context -- What is the purpose
of
> this API ?

Gregg noticed that a few OpenGL drivers return incorrect information about
attributes and uniforms. I guess he needs a more reliable source of information
to cross-check. Gregg?

> If the goal it to ensure more consistency between implementations, I'm not
sure
> how this is going to achieve it.  The "activeness" of attributes and uniforms
is
> inherently compiler (and linker) dependent.  
> 

What if we drop "active" from the function names? It will just return all
attribs and uniforms used in the shader.

[kbr1, 2010-09-14 19:53:47]

On 2010/09/14 18:26:25, alokp wrote:
> On 2010/09/14 18:07:47, dgkoch wrote:
> > On 2010/09/14 17:31:03, alokp wrote:
> > > Do values for OpenGL enums change?
> > No the enum values can never change (although there may be the odd case
where
> > they may be different between OpenGL and OpenGL ES)
> > 
> > Stepping back a bit, I think I'm missing some context -- What is the purpose
> of
> > this API ?
> 
> Gregg noticed that a few OpenGL drivers return incorrect information about
> attributes and uniforms. I guess he needs a more reliable source of
information
> to cross-check. Gregg?
> 
> > If the goal it to ensure more consistency between implementations, I'm not
> sure
> > how this is going to achieve it.  The "activeness" of attributes and
uniforms
> is
> > inherently compiler (and linker) dependent.  
> > 
> 
> What if we drop "active" from the function names? It will just return all
> attribs and uniforms used in the shader.

Daniel's point is a good one. The indices passed to the currently named
ShGetActiveAttrib and ShGetActiveUniform bear no relationship to the true
indices returned by OpenGL or ANGLE. Given this, it may make sense to structure
the API similarly to how Gregg suggested, returning information about all of the
uniforms and all of the attributes in bulk.

The reason this API is needed is to work around bugs in OpenGL drivers that
return incorrect type information for certain uniforms and attributes. The way
these APIs will be used is: make the underlying glGetActiveUniform or
glGetActiveAttrib call, do a hash table lookup with the name as the key and the
information from the shader validator as the value, and use the validator's type
information when returning information to the caller.

[Alok Priyadarshi, 2010-09-14 19:59:05]

> (2) should work. These enum values have not changed in years. You'd want to
> document that the returned types use precisely the same enum values as in
OpenGL
> ES 2.0.

Done.

[Alok Priyadarshi, 2010-09-14 20:04:45]

> Daniel's point is a good one. The indices passed to the currently named
> ShGetActiveAttrib and ShGetActiveUniform bear no relationship to the true
> indices returned by OpenGL or ANGLE. Given this, it may make sense to
structure
> the API similarly to how Gregg suggested, returning information about all of
the
> uniforms and all of the attributes in bulk.
> 
> The reason this API is needed is to work around bugs in OpenGL drivers that
> return incorrect type information for certain uniforms and attributes. The way
> these APIs will be used is: make the underlying glGetActiveUniform or
> glGetActiveAttrib call, do a hash table lookup with the name as the key and
the
> information from the shader validator as the value, and use the validator's
type
> information when returning information to the caller.

Yeah I had envisioned it to be used in a similar fashion. I would however like
to keep the API similar to OpenGL mainly because returning a list from a c-api
is a hassle. The application can always get all the attribs/uniforms and
construct whatever data-structure is appropriate - hashtable in this case.
Sounds OK?

[kbr1, 2010-09-14 20:11:28]

On 2010/09/14 20:04:45, alokp wrote:
> > Daniel's point is a good one. The indices passed to the currently named
> > ShGetActiveAttrib and ShGetActiveUniform bear no relationship to the true
> > indices returned by OpenGL or ANGLE. Given this, it may make sense to
> structure
> > the API similarly to how Gregg suggested, returning information about all of
> the
> > uniforms and all of the attributes in bulk.
> > 
> > The reason this API is needed is to work around bugs in OpenGL drivers that
> > return incorrect type information for certain uniforms and attributes. The
way
> > these APIs will be used is: make the underlying glGetActiveUniform or
> > glGetActiveAttrib call, do a hash table lookup with the name as the key and
> the
> > information from the shader validator as the value, and use the validator's
> type
> > information when returning information to the caller.
> 
> Yeah I had envisioned it to be used in a similar fashion. I would however like
> to keep the API similar to OpenGL mainly because returning a list from a c-api
> is a hassle. The application can always get all the attribs/uniforms and
> construct whatever data-structure is appropriate - hashtable in this case.
> Sounds OK?

I guess that sounds OK. Let's do rename them though to get rid of the "Active"
part of the function and enum names.

[Alok Priyadarshi, 2010-09-14 22:09:18]

> I guess that sounds OK. Let's do rename them though to get rid of the "Active"
> part of the function and enum names.

After thinking about this more I think it will be better to leave "active". If
we are not doing any optimization it is not against the spec to consider all
attribs or uniforms as active. We are not optimizing now but we may in the
future. It may not be too difficult to strip out unused vertex attribs.

If it is not too big a deal I think it will be better to leave it consistent
with OpenGL API under the assumption that all attribs and uniforms are active
since we are not optimizing.

[gmanchromium, 2010-09-14 22:51:47]

On Tue, Sep 14, 2010 at 12:53 PM, <kbr@chromium.org> wrote:

> On 2010/09/14 18:26:25, alokp wrote:
>
>> On 2010/09/14 18:07:47, dgkoch wrote:
>> > On 2010/09/14 17:31:03, alokp wrote:
>> > > Do values for OpenGL enums change?
>> > No the enum values can never change (although there may be the odd
>>
> case where
>
>> > they may be different between OpenGL and OpenGL ES)
>> >
>> > Stepping back a bit, I think I'm missing some context -- What is the
>>
> purpose
>
>> of
>> > this API ?
>>
>
>  Gregg noticed that a few OpenGL drivers return incorrect information
>>
> about
>
>> attributes and uniforms. I guess he needs a more reliable source of
>>
> information
>
>> to cross-check. Gregg?
>>
>
>  > If the goal it to ensure more consistency between implementations,
>>
> I'm not
>
>> sure
>> > how this is going to achieve it.  The "activeness" of attributes and
>>
> uniforms
>
>> is
>> > inherently compiler (and linker) dependent.
>> >
>>
>
>  What if we drop "active" from the function names? It will just return
>>
> all
>
>> attribs and uniforms used in the shader.
>>
>
> Daniel's point is a good one. The indices passed to the currently named
> ShGetActiveAttrib and ShGetActiveUniform bear no relationship to the
> true indices returned by OpenGL or ANGLE. Given this, it may make sense
> to structure the API similarly to how Gregg suggested, returning
> information about all of the uniforms and all of the attributes in bulk.
>
> The reason this API is needed is to work around bugs in OpenGL drivers
> that return incorrect type information for certain uniforms and
> attributes. The way these APIs will be used is: make the underlying
> glGetActiveUniform or glGetActiveAttrib call, do a hash table lookup
> with the name as the key and the information from the shader validator
> as the value, and use the validator's type information when returning
> information to the caller.



Yes, this is how I was planning on using it.  Get the attribs and uniforms
from GL. Cross reference with the compiler to get correct type values and
hopefully correct names.  "uniform float someArray[1]" should return
"someArray[0]" as its name where as "uniform float someValue;" should return
"someValue". Some GL drivers get this wrong and hence there is no way to
tell if a uniform is an array of 1 or a single value.




>
>
>
> http://codereview.appspot.com/2183041/
>

[Alok Priyadarshi, 2010-09-14 23:30:35]

All changes are uploaded. Please review.

I have left "active" in the function and enum names to be consistent, but am
open to removing it if it is too confusing. I will implement all the new
functions in a different CL. As of this CL, everything works as before except
for a few API changes.

[dgkoch, 2010-09-15 03:17:24]

On 2010/09/14 23:30:35, alokp wrote:
> I have left "active" in the function and enum names to be consistent, but am
> open to removing it if it is too confusing. I will implement all the new
> functions in a different CL. As of this CL, everything works as before except
> for a few API changes.

Thanks for explaining the intended usage.  

However, if this is intended to do cross validation with the information that is
returned from the underlying driver, I don't know if it can ever do this based
on the results of an optimization pass.  What happens if this optimization pass
is smarter than that of the underlying driver?   --- Thinking about it a bit
more, I guess that wouldn't be a problem since we'd presumably be sending the
pre-optimized shader down to the underlying driver.

Overall approach LsGTM.

[dgkoch, 2010-09-15 03:17:45]

http://codereview.appspot.com/2183041/diff/22001/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (left):

http://codereview.appspot.com/2183041/diff/22001/src/compiler/ShaderLang.cpp#...
src/compiler/ShaderLang.cpp:310: if (!InitThread())
did you mean to drop the !InitThread check from here?

[Alok Priyadarshi, 2010-09-15 03:46:40]

Thanks guys!

http://codereview.appspot.com/2183041/diff/22001/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (left):

http://codereview.appspot.com/2183041/diff/22001/src/compiler/ShaderLang.cpp#...
src/compiler/ShaderLang.cpp:310: if (!InitThread())
On 2010/09/15 03:17:46, dgkoch wrote:
> did you mean to drop the !InitThread check from here?

Yes. It is only needed in ShCompile() and ShConstructCompiler(). Rest of the
functions do not access TLS. They only return information already stored in the
compiler object.

BTW for all the threading code here, I do not think that this library is thread
safe. The preprocessor interface modifies many global variables. I also believe
there will be memory leaks if used in a multi-threaded way. I plan to fix all
that eventually.

[dgkoch, 2010-09-15 04:04:10]

On 2010/09/15 03:46:40, alokp wrote:
> 
> Yes. It is only needed in ShCompile() and ShConstructCompiler(). Rest of the
> functions do not access TLS. They only return information already stored in
the
> compiler object.

Ok thanks.

> BTW for all the threading code here, I do not think that this library is
thread
> safe. The preprocessor interface modifies many global variables. I also
believe
> there will be memory leaks if used in a multi-threaded way. I plan to fix all
> that eventually.

No I don't really expect it is either.  All of that is from the original 3Dlabs
code.

Daniel