Add an extension for querying translated HLSL shader source.

Add an extension for querying translated HLSL shader source.
Committed: http://code.google.com/p/angleproject/source/detail?r=780

[Zhenyao Mo, 2011-09-29 20:29:44]

Please review.

[dgkoch, 2011-09-30 20:11:21]

Overall looks pretty good.  A few comments about the extension, and a bigger
design question as to if we should add a new entry point or just a new token to
get the translated shader source length.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:55: GLsizei* length, GLchar*
source);
In an extension the GLtypes don't normally have the GL prefix on them.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:58: GLint* pnames);
ditto

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:65: Operations and the
Framebuffer)
Chapter 6 is actually "State and State Requests"

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:74: returns properties of the
shader object named shader in params. If pname is
use angle brackets to denote function parameters - ie "named <shader> in
<params>."

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:77: CompileShader hasn't been
called, the returned length will be 0.
might want to word this as: "If no source has been defined, CompileShader has
not been called, or the translation has failed for <shader>, zero is returned."

I do wonder though, is it really worth adding a whole new entry point just to
query the length?  It seems simpler just to add a new token
TRANSLATED_SHADER_SOURCE_LENGTH_ANGLE which can be passed into GetShaderiv. 
(simpler API-wise other than needing to get an ENUM assigned for it from the
registry -- I'll ask for it though if you decide to go that way).

http://codereview.appspot.com/5149046/diff/3001/src/libGLESv2/Shader.cpp
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/5149046/diff/3001/src/libGLESv2/Shader.cpp#newc...
src/libGLESv2/Shader.cpp:160: int Shader::getTranslatedSourceLength() const
looks like we're missing the part about returning an INVALID_OP if
SHADER_COMPILER is not TRUE (and also in getSourceLength).

[Zhenyao Mo, 2011-09-30 23:48:27]

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:77: CompileShader hasn't been
called, the returned length will be 0.
On 2011/09/30 20:11:21, dgkoch wrote:
> might want to word this as: "If no source has been defined, CompileShader has
> not been called, or the translation has failed for <shader>, zero is
returned."
> 
> I do wonder though, is it really worth adding a whole new entry point just to
> query the length?  It seems simpler just to add a new token
> TRANSLATED_SHADER_SOURCE_LENGTH_ANGLE which can be passed into GetShaderiv. 
> (simpler API-wise other than needing to get an ENUM assigned for it from the
> registry -- I'll ask for it though if you decide to go that way).

I thought about this, only I don't know how to get an ENUM from the registry. 
How formal should this be?

[dgkoch, 2011-10-01 02:50:43]

On 2011/09/30 23:48:27, Zhenyao Mo wrote:
>
http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
> File extensions/ANGLE_translated_shader_source.txt (right):
> 
>
http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
> extensions/ANGLE_translated_shader_source.txt:77: CompileShader hasn't been
> called, the returned length will be 0.
> On 2011/09/30 20:11:21, dgkoch wrote:
> > might want to word this as: "If no source has been defined, CompileShader
has
> > not been called, or the translation has failed for <shader>, zero is
> returned."
> > 
> > I do wonder though, is it really worth adding a whole new entry point just
to
> > query the length?  It seems simpler just to add a new token
> > TRANSLATED_SHADER_SOURCE_LENGTH_ANGLE which can be passed into GetShaderiv. 
> > (simpler API-wise other than needing to get an ENUM assigned for it from the
> > registry -- I'll ask for it though if you decide to go that way).
> 
> I thought about this, only I don't know how to get an ENUM from the registry. 
> How formal should this be?

It't not difficult, assuming I can get the registrar to do it in a timely
fashion (see: http://www.opengl.org/registry/#policies).  I've already got a
range of EGL enums for ANGLE.  I've done the same for ES2.  (see
https://cvs.khronos.org/bugzilla/show_bug.cgi?id=8100)

For now use the temporary value 0x6900 and I'll assign a proper value once I've
obtained a range.

[Zhenyao Mo, 2011-10-01 20:14:21]

Thanks for reviewing this CL.  Revised, please have another look.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:55: GLsizei* length, GLchar*
source);
On 2011/09/30 20:11:21, dgkoch wrote:
> In an extension the GLtypes don't normally have the GL prefix on them.

Done.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:65: Operations and the
Framebuffer)
On 2011/09/30 20:11:21, dgkoch wrote:
> Chapter 6 is actually "State and State Requests"

Done.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:74: returns properties of the
shader object named shader in params. If pname is
On 2011/09/30 20:11:21, dgkoch wrote:
> use angle brackets to denote function parameters - ie "named <shader> in
> <params>."

Done.

http://codereview.appspot.com/5149046/diff/3001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:77: CompileShader hasn't been
called, the returned length will be 0.
On 2011/09/30 20:11:21, dgkoch wrote:
> might want to word this as: "If no source has been defined, CompileShader has
> not been called, or the translation has failed for <shader>, zero is
returned."
> 
> I do wonder though, is it really worth adding a whole new entry point just to
> query the length?  It seems simpler just to add a new token
> TRANSLATED_SHADER_SOURCE_LENGTH_ANGLE which can be passed into GetShaderiv. 
> (simpler API-wise other than needing to get an ENUM assigned for it from the
> registry -- I'll ask for it though if you decide to go that way).

Done.

http://codereview.appspot.com/5149046/diff/3001/src/libGLESv2/Shader.cpp
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/5149046/diff/3001/src/libGLESv2/Shader.cpp#newc...
src/libGLESv2/Shader.cpp:160: int Shader::getTranslatedSourceLength() const
On 2011/09/30 20:11:21, dgkoch wrote:
> looks like we're missing the part about returning an INVALID_OP if
> SHADER_COMPILER is not TRUE (and also in getSourceLength).

In SHADER_COMPILER is always TRUE right?  Because we don't accept binary shader
source.

[dgkoch, 2011-10-02 20:11:00]

LGTM with the export number fixed up.

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/libGLESv2.def
File src/libGLESv2/libGLESv2.def (right):

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/libGLESv2.def#n...
src/libGLESv2/libGLESv2.def:157: glGetTranslatedShaderSourceANGLE @159
I guess this should be @158 now?

[dgkoch, 2011-10-02 20:12:52]

On 2011/10/01 20:14:21, Zhenyao Mo wrote:
> On 2011/09/30 20:11:21, dgkoch wrote:
> > looks like we're missing the part about returning an INVALID_OP if
> > SHADER_COMPILER is not TRUE (and also in getSourceLength).
> 
> In SHADER_COMPILER is always TRUE right?  Because we don't accept binary
shader
> source.

You're correct. I was thinking this toggled to FALSE if ReleaseCompiler is
called but this is only FALSE if you don't have an online compiler -- which we
always do.

[Zhenyao Mo, 2011-10-03 17:53:41]

158 is used by glBindTexImage (the last line of this doc)

On 2011/10/02 20:11:00, dgkoch wrote:
> LGTM with the export number fixed up.
> 
> http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/libGLESv2.def
> File src/libGLESv2/libGLESv2.def (right):
> 
>
http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/libGLESv2.def#n...
> src/libGLESv2/libGLESv2.def:157: glGetTranslatedShaderSourceANGLE @159
> I guess this should be @158 now?

[dgkoch, 2011-10-03 17:55:35]

Right. I missed that. LGTM as is then.

[kbr1, 2011-10-03 20:09:22]

Basically looks good to me but couple of comments.

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:41: This extension addes a new
function to query the translated HLSL
Please provide some more background here. You could mention that WebGL uses the
GLSL ES 2.0 spec on all platforms, and translates these shaders to the host
platform's native language (HLSL, GLSL, and even GLSL ES). For debugging
purposes, it is useful to be able to examine the shader after translation.

Also, I think this extension could be generalized to not be HLSL specific. It's
useful to examine the translated GLSL source on desktop GL systems, and even the
post-processed GLSL ES source on native GLES20 systems. If we're going to expose
this functionality in the command buffer code, it makes sense to me to do so
under this new entry point.

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:71: HLSL source string, including
a null terminator, is returned. If no source has
Here and below, doesn't need to be HLSL specific.

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/Shader.cpp
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/Shader.cpp#newc...
src/libGLESv2/Shader.cpp:216: *length = index;
How about a refactoring to eliminate the duplicated code with Shader::getSource?

[Zhenyao Mo, 2011-10-03 20:56:48]

revised, please have another look.

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:41: This extension addes a new
function to query the translated HLSL
On 2011/10/03 20:09:22, kbr1 wrote:
> Please provide some more background here. You could mention that WebGL uses
the
> GLSL ES 2.0 spec on all platforms, and translates these shaders to the host
> platform's native language (HLSL, GLSL, and even GLSL ES). For debugging
> purposes, it is useful to be able to examine the shader after translation.
> 
> Also, I think this extension could be generalized to not be HLSL specific.
It's
> useful to examine the translated GLSL source on desktop GL systems, and even
the
> post-processed GLSL ES source on native GLES20 systems. If we're going to
expose
> this functionality in the command buffer code, it makes sense to me to do so
> under this new entry point.

Done.

http://codereview.appspot.com/5149046/diff/8001/extensions/ANGLE_translated_s...
extensions/ANGLE_translated_shader_source.txt:71: HLSL source string, including
a null terminator, is returned. If no source has
On 2011/10/03 20:09:22, kbr1 wrote:
> Here and below, doesn't need to be HLSL specific.

Done.

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/Shader.cpp
File src/libGLESv2/Shader.cpp (right):

http://codereview.appspot.com/5149046/diff/8001/src/libGLESv2/Shader.cpp#newc...
src/libGLESv2/Shader.cpp:216: *length = index;
On 2011/10/03 20:09:22, kbr1 wrote:
> How about a refactoring to eliminate the duplicated code with
Shader::getSource?

Done.

[kbr1, 2011-10-03 21:32:46]

LGTM with one minor nit and one comment.

http://codereview.appspot.com/5149046/diff/15001/extensions/ANGLE_translated_...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/15001/extensions/ANGLE_translated_...
extensions/ANGLE_translated_shader_source.txt:43: ES). For debugging purpose, it
is useful to be able to examine the shader
purpose -> purposes

http://codereview.appspot.com/5149046/diff/15001/src/libGLESv2/Shader.h
File src/libGLESv2/Shader.h (right):

http://codereview.appspot.com/5149046/diff/15001/src/libGLESv2/Shader.h#newco...
src/libGLESv2/Shader.h:83: void getSourceImpl(GLsizei bufSize, GLsizei *length,
char *buffer, char *source);
I'd put "source" as the first argument so that outputs follow inputs, but this
is a matter of style.

[Zhenyao Mo, 2011-10-03 21:45:10]

http://codereview.appspot.com/5149046/diff/15001/extensions/ANGLE_translated_...
File extensions/ANGLE_translated_shader_source.txt (right):

http://codereview.appspot.com/5149046/diff/15001/extensions/ANGLE_translated_...
extensions/ANGLE_translated_shader_source.txt:43: ES). For debugging purpose, it
is useful to be able to examine the shader
On 2011/10/03 21:32:46, kbr1 wrote:
> purpose -> purposes

Done.

http://codereview.appspot.com/5149046/diff/15001/src/libGLESv2/Shader.h
File src/libGLESv2/Shader.h (right):

http://codereview.appspot.com/5149046/diff/15001/src/libGLESv2/Shader.h#newco...
src/libGLESv2/Shader.h:83: void getSourceImpl(GLsizei bufSize, GLsizei *length,
char *buffer, char *source);
On 2011/10/03 21:32:46, kbr1 wrote:
> I'd put "source" as the first argument so that outputs follow inputs, but this
> is a matter of style.

Done.