CC: list. ---------- Forwarded message ---------- From: Tom Gall <[email protected]> Date: Mon, Jan 14, 2013 at 10:57 PM Subject: Re: [PATCH 1/1] Alter shader_runner GL/GLSL ES version requirement syntax. To: Stuart Abercrombie <[email protected]> Cc: [email protected], Chad Versace <[email protected]>, Paul Berry <[email protected]>
On Mon, Jan 14, 2013 at 10:22 PM, Stuart Abercrombie <[email protected]> wrote: > The current syntax isn't compatible with the same shader_test supporting GL > and GLES in the future. > > Modify existing GL ES tests to use the new syntax, and remove explicit > #version directives, which will instead be inserted based on GLSL >= > requirements. Explicit #version directives aren't necessarily evil. I agree the test(s) that use it should be minimal, but sanity tests within a glsl-es-x.xx directory makes sense that #version would be used. > --- > tests/shaders/shader_runner.c | 22 ++++++------------- > .../spec/glsl-es-1.00/execution/sanity.shader_test | 7 +---- > .../spec/glsl-es-3.00/execution/sanity.shader_test | 8 +----- > 3 files changed, 11 insertions(+), 26 deletions(-) > > diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c > index db92b8f..8135ea7 100644 > --- a/tests/shaders/shader_runner.c > +++ b/tests/shaders/shader_runner.c > @@ -459,15 +459,8 @@ process_comparison(const char *src, enum comparison *cmp) > > > /** > - * To specify an ES version, append "es" to the version. For example: > - * GL >= 3.0 es > - * GLSL >= 3.00 es > - * > - * GLSL ES 1.00 is a special case. Despite being an ES shading language, > - * the #version directive lacks "es"; that is, the directive is > - * `#version 100` rather than `#version 100 es`. Therefore be lenient in > - * parsing that version. Interpret `GLSL >= 100` and `GLSL >= 100 es` > - * identically. > + * " ES" before the comparison operator indicates the version > + * pertains to GL ES. Would one always need to specify GL ES and GLSL ES? Or is say GLSL ES >= 1.00 good enough to imply GLSL ES 1.00 and GL ES 2.0 ? Likewise can only GL ES >= 2.0 imply GLSL ES >= 1.00 as well? Stepping back, why is the GL (es) version necessary? This is testing shaders after all. What happens if someone specified : GL >= 3.0 GLSL ES >= 1.00 What would that even mean? Silly, I know. > */ > void > parse_version_comparison(const char *line, enum comparison *cmp, > @@ -476,8 +469,12 @@ parse_version_comparison(const char *line, enum > comparison *cmp, > unsigned major; > unsigned minor; > unsigned full_num; > - bool es; > + bool es = false; > > + if (string_match(" ES", line)) { > + es = true; > + line += 3; > + } > line = eat_whitespace(line); > line = process_comparison(line, cmp); > > @@ -485,9 +482,6 @@ parse_version_comparison(const char *line, enum > comparison *cmp, > sscanf(line, "%u.%u", &major, &minor); > line = eat_text(line); > > - line = eat_whitespace(line); > - es = string_match("es", line); > - > /* This hack is so that we can tell the difference between GL versions > * and GLSL versions. All GL versions look like 3.2, and we want the > * integer to be 32. All GLSL versions look like 1.40, and we want > @@ -495,8 +489,6 @@ parse_version_comparison(const char *line, enum > comparison *cmp, > */ > if (tag == VERSION_GLSL) { > full_num = (major * 100) + minor; > - if (full_num == 100) > - es = true; Well technically there isn't a GLSL 100. It has to be es. > } else { > full_num = (major * 10) + minor; > } > diff --git a/tests/spec/glsl-es-1.00/execution/sanity.shader_test > b/tests/spec/glsl-es-1.00/execution/sanity.shader_test > index a1dc07a..0884e2c 100644 > --- a/tests/spec/glsl-es-1.00/execution/sanity.shader_test > +++ b/tests/spec/glsl-es-1.00/execution/sanity.shader_test > @@ -1,11 +1,10 @@ > # Fill the window with red, then green, then blue. > > [require] > -GL >= 2.0 es > +GL ES >= 2.0 > +GLSL ES >= 1.00 > > [vertex shader] > -#version 100 > - This is a sanity test for GLSL ES 1.00. #version 100 within the vertex shader should be fine and not removed IMHO especially for a test located in a specific to glsl es 1.00 directory. > attribute vec4 vertex; > > void main() { > @@ -13,8 +12,6 @@ void main() { > } > > [fragment shader] > -#version 100 > - Same comment as above. > uniform vec4 u_color; > > void main() { > diff --git a/tests/spec/glsl-es-3.00/execution/sanity.shader_test > b/tests/spec/glsl-es-3.00/execution/sanity.shader_test > index e7e6ded..1709d78 100644 > --- a/tests/spec/glsl-es-3.00/execution/sanity.shader_test > +++ b/tests/spec/glsl-es-3.00/execution/sanity.shader_test > @@ -1,12 +1,10 @@ > # Fill the window with red, then green, then blue. > > [require] > -GL >= 3.0 es > -GLSL >= 3.00 es > +GL ES >= 3.0 > +GLSL ES >= 3.00 > > [vertex shader] > -#version 300 es > - > in vec4 vertex; > > void main() { > @@ -14,8 +12,6 @@ void main() { > } > > [fragment shader] > -#version 300 es > - > uniform vec4 u_color; > out vec4 color; > > -- > 1.7.5.4 > In closing, it seems like a lot of *.shader_test across piglit should be moved into a more common glsl directory and be made #version free and as makes sense modified such that they would be appropriate for use with and without es. It would promote a great deal of commonality across the shader tests. If welcome, happy to submit patches. Thanks! -- Regards, Tom "Where's the kaboom!? There was supposed to be an earth-shattering kaboom!" Marvin Martian Graphics Working Group | Linaro.org │ Open source software for ARM SoCs w) tom.gall att linaro.org h) tom_gall att mac.com -- Regards, Tom "Where's the kaboom!? There was supposed to be an earth-shattering kaboom!" Marvin Martian Graphics Working Group | Linaro.org │ Open source software for ARM SoCs w) tom.gall att linaro.org h) tom_gall att mac.com _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
