[Piglit] Fwd: [PATCH 1/1] Alter shader_runner GL/GLSL ES version requirement syntax.
Tom Gall
tom.gall at linaro.org
Mon Jan 14 20:59:49 PST 2013
CC: list.
---------- Forwarded message ----------
From: Tom Gall <tom.gall at linaro.org>
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 <sabercrombie at chromium.org>
Cc: piglit at lists.freedeskto.org, Chad Versace
<chad.versace at linux.intel.com>, Paul Berry <stereotype441 at gmail.com>
On Mon, Jan 14, 2013 at 10:22 PM, Stuart Abercrombie
<sabercrombie at chromium.org> 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
More information about the Piglit
mailing list