Adding support for OES_standard_derivatives extension. This is not the comple...

Adding support for OES_standard_derivatives extension. This is not the complete implementation. Sending it to get feedback on the API. Is it OK to add extension support into TBuiltInResource? I could create a new struct for extensions but that would lead to API change.
BUG=25

[kbr1, 2010-08-23 20:22:38]

Looks OK to me -- seems like a reasonable place to put this flag. Just to
confirm, this will be settable per compiler instance and is completely isolated
to each instance?

[Alok Priyadarshi, 2010-08-23 20:27:52]

Yes and yes.

[kbr1, 2010-08-23 20:29:09]

On 2010/08/23 20:27:52, alokp wrote:
> Yes and yes.

LGTM then.

[Alok Priyadarshi, 2010-08-24 16:58:25]

Daniel, Do you have any objections/suggestions?

[dgkoch, 2010-08-24 19:54:00]

It does seem a little weird to be enabling/disabling extensions via the
resources structure instead of just adding a new "extensions" structure.  

Is there a particular reason you need to minimize changes to the API? 
Technically this is changing the api as well, it's just a bit more subtle.  An
application which uses this will still need to be modified to ensure that a
proper value is set for OES_standard_derivatives, otherwise it could just get
uninitialized memory.  If you add an TBuiltInExtension struct (for example) it
is (or should be) a bit more obvious to applications that they might want to
zero out the memory in this structure (in case it is extended) before they use
it.

Daniel

http://codereview.appspot.com/1953047/diff/1/2
File include/GLSLANG/ResourceLimits.h (right):

http://codereview.appspot.com/1953047/diff/1/2#newcode26
include/GLSLANG/ResourceLimits.h:26: // Set to 1 if extension is supported, else
0.
I might say "set to 1 to enable the extension.." or something like that.

[Alok Priyadarshi, 2010-08-24 20:34:37]

On 2010/08/24 19:54:00, dgkoch wrote:
> It does seem a little weird to be enabling/disabling extensions via the
> resources structure instead of just adding a new "extensions" structure.  
> 
> Is there a particular reason you need to minimize changes to the API? 

I am trying to keep the API compact. If we add another struct for extension,
ShConstructCompiler will need one more parameter. I consider BuiltInResource =
BuiltInConstant + BuiltInExtension + (additional built-in stuff for future).

[dgkoch, 2010-08-24 21:26:09]

On 2010/08/24 20:34:37, alokp wrote:
> On 2010/08/24 19:54:00, dgkoch wrote:
> > It does seem a little weird to be enabling/disabling extensions via the
> > resources structure instead of just adding a new "extensions" structure.  
> > 
> > Is there a particular reason you need to minimize changes to the API? 
> 
> I am trying to keep the API compact. If we add another struct for extension,
> ShConstructCompiler will need one more parameter. I consider BuiltInResource =
> BuiltInConstant + BuiltInExtension + (additional built-in stuff for future).

Fair enough. 
But that does still leave my concern about making sure that clients are updated
appropriately when the structure is extended.  For example if you look at how we
use this in ANGLE, we just declare it on the stack and fill in the known
members. If this is silently extended, we'll probably just get garbage filled in
for the extension bits, which could accidentally enable them (I know you'll fix
ANGLE when you add this, but the problem will remain for any other clients).   I
guess we can recommend zeroing out the entire structure first, and as long as we
make sure that defaults values of "0" for any additional fields mean disabled...


Daniel

[Alok Priyadarshi, 2010-08-24 21:32:13]

> guess we can recommend zeroing out the entire structure first, and as long as
we
> make sure that defaults values of "0" for any additional fields mean
disabled...

I agree. We should either recommend zeroing out the struct or provide an
ShInitBuiltInResource function which fills appropriate (minimum) values allowed.

[dgkoch, 2010-08-24 21:35:43]

On 2010/08/24 21:32:13, alokp wrote:
> > guess we can recommend zeroing out the entire structure first, and as long
as
> we
> > make sure that defaults values of "0" for any additional fields mean
> disabled...
> 
> I agree. We should either recommend zeroing out the struct or provide an
> ShInitBuiltInResource function which fills appropriate (minimum) values
allowed.

I like the idea of ShInitBuiltInResource.  That strikes me as the safest and
most extensible.

[Alok Priyadarshi, 2010-08-24 22:16:26]

Added ShInitBuiltInResource. Please review.

[kbr1, 2010-08-24 23:00:48]

On 2010/08/24 22:16:26, alokp wrote:
> Added ShInitBuiltInResource. Please review.

I really hope there is no expectation that we will support binary upgrades of
ANGLE without recompiling the host application. If there is, and the host app
allocates the TBuiltInResource structure on the stack, then
ShInitBuiltInResource will scribble all over the host app's stack as it attempts
to initialize more storage than the host app allocated. The only way to fix this
is to version the structs like is done in some Microsoft APIs, and require that
the version number be set in the struct before passing it in to various APIs. I
think this would add more complexity than it is worth.

Another option would be to just add a constructor on TBuiltInResource rather
than adding a separate ShInitBuiltInResource entry point.

There is now duplicated code between GenerateResources (translator.cpp) and
ShInitBuiltInResource (ShaderLang.cpp).

[Alok Priyadarshi, 2010-08-24 23:23:25]

> I really hope there is no expectation that we will support binary upgrades of
> ANGLE without recompiling the host application. If there is, and the host app
> allocates the TBuiltInResource structure on the stack, then
> ShInitBuiltInResource will scribble all over the host app's stack as it
attempts
> to initialize more storage than the host app allocated. The only way to fix
this
> is to version the structs like is done in some Microsoft APIs, and require
that
> the version number be set in the struct before passing it in to various APIs.
I
> think this would add more complexity than it is worth.

We can think about versioning if we guarantee ABI compatibility.

> Another option would be to just add a constructor on TBuiltInResource rather
> than adding a separate ShInitBuiltInResource entry point.

So far we have tried to keep it a C API. So a constructor would not work.

> There is now duplicated code between GenerateResources (translator.cpp) and
> ShInitBuiltInResource (ShaderLang.cpp).

translator.cpp is a sample. I usually tweak it to test stuff.

[dgkoch, 2010-08-25 01:44:44]

On 2010/08/24 23:23:25, alokp wrote:
> > I really hope there is no expectation that we will support binary upgrades
of
> > ANGLE without recompiling the host application. If there is, and the host
app
> > allocates the TBuiltInResource structure on the stack, then
> > ShInitBuiltInResource will scribble all over the host app's stack as it
> attempts
> > to initialize more storage than the host app allocated. The only way to fix
> this
> > is to version the structs like is done in some Microsoft APIs, and require
> that
> > the version number be set in the struct before passing it in to various
APIs.
> I
> > think this would add more complexity than it is worth.
> 
> We can think about versioning if we guarantee ABI compatibility.

I certainly wasn't thinking of binary compatibility.  The case I was worried is
if the structure is extended and the application is recompiled, but doesn't
actually initialize the rest of the fields, it could inadvertently start
enabling extensions it really doesn't mean too (or worse have fields initialized
with random memory).

This looks like a reasonable starting point to me.

Daniel

[Alok Priyadarshi, 2010-08-25 18:11:25]

There may be other ways to make this binary compatible such as making
TBuiltInResource opaque and having functions to set each resource or moving the
resource settings to compiler which already has an opaque handle.

But I do not want tackle this unless absolutely necessary