GLSL backend now emits "#version 120" to legally access glPointCoord.

GLSL backend now emits "#version 120" to legally access invariant keyword and gl_PointCoord built-in variable.
BUG=35

[dgkoch, 2010-10-01 23:47:19]

http://codereview.appspot.com/2341043/diff/1/src/compiler/TranslatorGLSL.cpp
File src/compiler/TranslatorGLSL.cpp (right):

http://codereview.appspot.com/2341043/diff/1/src/compiler/TranslatorGLSL.cpp#...
src/compiler/TranslatorGLSL.cpp:17: // which supports GLSL 1.5, it should be
pretty safe to write this version
Actually GL 2.0 only mandates GLSL version 1.10.  It was GL 2.1 that mandated
GLSL 1.20 support.

[kbr1, 2010-10-02 00:51:25]

I am concerned about the potential compatibility impact of always writing this
version directive, especially because multiple browser vendors are using ANGLE
as the shader front end. I think it would be safer to only emit it when
gl_PointCoord is referenced in the fragment shader, in which case there would
only be potential breakage on demos actually using this functionality. Daniel,
do you share my concern?

[dgkoch, 2010-10-02 23:50:07]

On 2010/10/02 00:51:25, kbr1 wrote:
> I am concerned about the potential compatibility impact of always writing this
> version directive, especially because multiple browser vendors are using ANGLE
> as the shader front end. I think it would be safer to only emit it when
> gl_PointCoord is referenced in the fragment shader, in which case there would
> only be potential breakage on demos actually using this functionality. Daniel,
> do you share my concern?

That is one way of looking at it.  On the other hand if you *don't* have
#version 120 enabled all the time, you might suddenly get surprised by the
stricter semantics it enforces (it is a bit more rigorous about requiring
explicit cast, etc), just because you started using PointCoord in your shader. 
Now, I'm not sure how much of that matters in practice, since it is actually the
GLSL translator that is emitting the Desktop GLSL code, and not the application.
 I think the vital piece of information is -- are there any target platforms
that are going to use the GL backend that don't support GLSL 1.20?  I'd rather
expect those to be few and far between at this point.  The other option is to
actually check the version of GLSL supported by the underlying drivers (using
the SHADING_LANGUAGE_VERSION query) and only emit the #version 120 if that or
better is supported.

Hope this helps,
Daniel

[Alok Priyadarshi, 2010-10-04 20:26:49]

> Actually GL 2.0 only mandates GLSL version 1.10.  It was GL 2.1 that mandated
> GLSL 1.20 support.

I read the spec wrong. Thanks for pointing this out. In this case it may make
sense to write out "#version 120" only if gl_PointCoord is used. Do you guys
know if there are other problematic built-in features in GLSL ES 1.0 that are
not available in GLSL 1.0.

[kbr1, 2010-10-04 20:44:49]

On 2010/10/04 20:26:49, alokp wrote:
> > Actually GL 2.0 only mandates GLSL version 1.10.  It was GL 2.1 that
mandated
> > GLSL 1.20 support.
> 
> I read the spec wrong. Thanks for pointing this out. In this case it may make
> sense to write out "#version 120" only if gl_PointCoord is used. Do you guys
> know if there are other problematic built-in features in GLSL ES 1.0 that are
> not available in GLSL 1.0.

This is the only language-level feature I am aware of. We found earlier that it
was necessary to enable some desktop OpenGL state in order to allow the vertex
shader to modify the point size, but that did not affect which tokens were
accepted by the GLSL compiler.

[kbr1, 2010-10-04 21:03:52]

On 2010/10/02 23:50:07, dgkoch wrote:
> On 2010/10/02 00:51:25, kbr1 wrote:
> > I am concerned about the potential compatibility impact of always writing
this
> > version directive, especially because multiple browser vendors are using
ANGLE
> > as the shader front end. I think it would be safer to only emit it when
> > gl_PointCoord is referenced in the fragment shader, in which case there
would
> > only be potential breakage on demos actually using this functionality.
Daniel,
> > do you share my concern?
> 
> That is one way of looking at it.  On the other hand if you *don't* have
> #version 120 enabled all the time, you might suddenly get surprised by the
> stricter semantics it enforces (it is a bit more rigorous about requiring
> explicit cast, etc), just because you started using PointCoord in your shader.

> Now, I'm not sure how much of that matters in practice, since it is actually
the
> GLSL translator that is emitting the Desktop GLSL code, and not the
application.
>  I think the vital piece of information is -- are there any target platforms
> that are going to use the GL backend that don't support GLSL 1.20?  I'd rather
> expect those to be few and far between at this point.  The other option is to
> actually check the version of GLSL supported by the underlying drivers (using
> the SHADING_LANGUAGE_VERSION query) and only emit the #version 120 if that or
> better is supported.

I expect Chrome will probably be OK, since we aren't using OpenGL at all on
Windows, and GLSL 1.20 seems to be supported on Mac OS hardware and OS versions
a few years old. (Linux users are probably running recent OpenGL drivers in
general.) However, I am still concerned because of browers (like Firefox) which
are still implementing WebGL support on Windows using OpenGL, and may be running
on pretty old OpenGL hardware.

I think we should implement the scan for gl_PointCoord in the fragment shader
and only emit the version directive if needed. This is the change that will have
the least impact. The stricter casting rules, etc. should presumably already be
caught by the shader translator.

[Alok Priyadarshi, 2010-10-05 19:01:29]

Yes I am not concerned about the stricter casting rules either. I scoured the
change history of GLSL spec from 1.0 to 1.2 and have noted the changes that
seemed relevant below:

------------------------------------------------------------------

Language Version 1.10
Document Revision 59

Changes since version 1.051
- 3.3 Added #version and #extension to declare version and extensions.
- 8 Added built-in functions refract, exp, and log
- 8.3 Change step() to compare x < edge instead of x <= edge.

We need to write version 1.1 if any of the built-in functions - refract, exp,
log, or step - are used.

------------------------------------------------------------------

Language Version: 1.20
Document Revision: 8

Changes from revision 4 of version 1.20
- Removed embedded structures to match ES, waiting for scoping operator to
access embedded type.
- Many changes to reduce differences when compared to the ES specification.

Changes from revision 3 of version 1.20
- Add the invariant keyword and its support
- Add C++ style name hiding rules

Changes from revision 60 of version 1.10
- Add gl_PointCoord for fragment shaders to query a fragment's position within a
point sprite.

We need to write version 1.2 if invariant keyword or gl_PointCoord variable are
used. I am not too sure whether other changes are relevant or not. But if they
are it may make sense to always write version 1.2.

------------------------------------------------------------------

What do you guys think?

[kbr1, 2010-10-05 19:22:46]

On 2010/10/05 19:01:29, alokp wrote:
> Yes I am not concerned about the stricter casting rules either. I scoured the
> change history of GLSL spec from 1.0 to 1.2 and have noted the changes that
> seemed relevant below:
> 
> ------------------------------------------------------------------
> 
> Language Version 1.10
> Document Revision 59
> 
> Changes since version 1.051
> - 3.3 Added #version and #extension to declare version and extensions.
> - 8 Added built-in functions refract, exp, and log
> - 8.3 Change step() to compare x < edge instead of x <= edge.
> 
> We need to write version 1.1 if any of the built-in functions - refract, exp,
> log, or step - are used.
> 
> ------------------------------------------------------------------
> 
> Language Version: 1.20
> Document Revision: 8
> 
> Changes from revision 4 of version 1.20
> - Removed embedded structures to match ES, waiting for scoping operator to
> access embedded type.
> - Many changes to reduce differences when compared to the ES specification.
> 
> Changes from revision 3 of version 1.20
> - Add the invariant keyword and its support
> - Add C++ style name hiding rules
> 
> Changes from revision 60 of version 1.10
> - Add gl_PointCoord for fragment shaders to query a fragment's position within
a
> point sprite.
> 
> We need to write version 1.2 if invariant keyword or gl_PointCoord variable
are
> used. I am not too sure whether other changes are relevant or not. But if they
> are it may make sense to always write version 1.2.
> 
> ------------------------------------------------------------------
> 
> What do you guys think?

Thanks for researching this so thoroughly. After talking with apatrick and
vangelis the consensus is that it's risky for e.g. WebGL implementors to rely on
OpenGL support on Windows, so it should be OK to emit the identifier in all
cases as you're currently doing.

LGTM, but could you please update the comment based on Daniel's feedback and
your research above? Thanks.

[dgkoch, 2010-10-05 19:39:57]

LGTM too (with updated comments).

Might also be worth a courtesy note to any other implementors that are using the
GLSL backend about new requirement as well.

[Alok Priyadarshi, 2010-10-05 22:14:31]

Updated comments.

However one of the conformance tests started failing. My desktop GLSL compiler
(NVIDIA Quadro FX 380, driver version 8.16.11.9187) does not like one of the
translated shaders. The simplified case is as follows:

// ORIGINAL SHADER
void main() {
  bvec4 v5;
  vec3 v6 = vec3(v5);
}

// TRANSLATED SHADER
#version 120
void main(){
bvec4 v5;
vec3 v6 = vec3(vec4(v5)); <- ERROR ON THIS LINE
}

// ERROR
0(4) : error C7011: implicit cast from "float" to "vec3"

I am stumped. Do you see any problem with the translated shader? The error goes
away if I do not write #version.

[kbr1, 2010-10-05 23:55:36]

On 2010/10/05 22:14:31, alokp wrote:
> Updated comments.
> 
> However one of the conformance tests started failing. My desktop GLSL compiler
> (NVIDIA Quadro FX 380, driver version 8.16.11.9187) does not like one of the
> translated shaders. The simplified case is as follows:
> 
> // ORIGINAL SHADER
> void main() {
>   bvec4 v5;
>   vec3 v6 = vec3(v5);
> }
> 
> // TRANSLATED SHADER
> #version 120
> void main(){
> bvec4 v5;
> vec3 v6 = vec3(vec4(v5)); <- ERROR ON THIS LINE
> }
> 
> // ERROR
> 0(4) : error C7011: implicit cast from "float" to "vec3"
> 
> I am stumped. Do you see any problem with the translated shader? The error
goes
> away if I do not write #version.

I've gone through the GLSL spec version 1.20.8 and as far as I can see the
shader should be legal (aside from the fact that it does nothing useful).
Construction of vec4 from bvec4 should be allowed, as is dropping the fourth
component of the vec4. I think we should file a GLSL driver bug with NVIDIA.

This motivates only adding this directive when it's absolutely necessary. Note
that GLSL version 1.10 does not require the #version directive, only version
1.20, so you'd really only need to introduce the directive when 1.20 features
are explicitly used.

[Alok Priyadarshi, 2010-10-06 04:55:30]

Forgot to mention that I was testing this on my Windows 7 machine. I will test
it on Linux tomorrow.
 
> This motivates only adding this directive when it's absolutely necessary. Note
> that GLSL version 1.10 does not require the #version directive, only version
> 1.20, so you'd really only need to introduce the directive when 1.20 features
> are explicitly used.

Which means scanning for invariant qualifier and gl_PointCoord variable. I will
also look into what the spec means by "c++ style name hiding rules". Le Sigh...

[Alok Priyadarshi, 2010-10-12 22:31:42]

Now I write the version directive only if invariant qualifier or gl_PointCoord
variable are used. Please review.

[kbr1, 2010-10-13 01:15:31]

LGTM

http://codereview.appspot.com/2341043/diff/16001/src/compiler/VersionGLSL.h
File src/compiler/VersionGLSL.h (right):

http://codereview.appspot.com/2341043/diff/16001/src/compiler/VersionGLSL.h#n...
src/compiler/VersionGLSL.h:29: // Returns 120 if "invariant" keyword of
"gl_PointCoord" is used
of -> or

http://codereview.appspot.com/2341043/diff/16001/src/compiler/VersionGLSL.h#n...
src/compiler/VersionGLSL.h:30: // in the shader. Else 110 is returned.
Might be nicer to define the static constants as enums in this class, but this
is not that big a deal.

[dgkoch, 2010-10-13 02:49:26]

LGTM