[Piglit] [PATCH] Add shader_runner GLSL version specification via requirements section.
Paul Berry
stereotype441 at gmail.com
Mon Aug 27 19:32:27 PDT 2012
On 27 August 2012 17:23, Stuart Abercrombie <sabercrombie at google.com> wrote:
> Thanks for the detailed response. I had various other changes bundled
> up with this but I thought it best to get some feedback before going
> too far.
>
> 1. I got ESSL from gles2_shader_runner.c. I wanted to use a more
> obvious string but the way the parser is written made it more
> convenient to have a string the same length as GLSL. It's easy enough
> to change with a minor cost in source ugliness.
>
> 2. I envisaged all the shader_test files specifying both GLSL
> versions. Since all .shader_test files would need modification in the
> end I was thinking they could be updated to conform to new
> expectations.
>
> I guess there's some value in handling missing GLSL version
> specifiers.
The main value I see is to cover the case where there is no GLSL ES version
that makes sense for the test (e.g. clipping tests) or where there is no
desktop GLSL version that makes sense (probably rarer, but I bet we'll come
up with a few).
> I assume then that we would also have a GL ES version
> requirement to go with the GL version requirement, operating in a
> similar way.
>
Ah, good point, yes.
>
> 3. I'm very glad to hear about the multiple file loading being
> unnecessary. That does indeed make things much simpler.
>
> 4. I thought a lot about trying to use the existing GLSL requirement
> instead of introducing a new one. As you say, the problem lies with
> general comparison operators. I verified that operators other than >=
> weren't being used, so I was tempted to do exactly what you suggest
> and reject them.
>
> In the end I was worried about a) effectively introducing a more
> irregular syntax and b) foreclosing the possibility of testing GLSL
> features with #version strings decoupled from the GLSL requirement
> (not that I can think of a particular scenario for this). I'm fine
> with a) if you are. As for b), it seems it would be covered by your
> suggestion of making #version strings in the source optional.
>
> Minor problems:
>
> 1. Thanks. I was planning to revisit this.
>
> 2. I hadn't noticed the Open GL ES 3 #version syntax change -- thanks.
>
> The multi-patch plan sounds good. I'll pursue that.
>
> Stuart
>
> On Mon, Aug 27, 2012 at 1:46 PM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 24 August 2012 16:09, Stuart Abercrombie <sabercrombie at chromium.org>
> > wrote:
> >>
> >> The idea is to allow different GLSL versions for GL vs GL ES. There
> >> shouldn't be a functional change until shader scripts are altered too.
> >>
> >> Added ES GLSL version string parsing.
> >
> >
> > I like where you're going with this. A few comments before we get into
> the
> > patch itself:
> >
> > 1. "ESSL" is a nonstandard name for the GLES shading language. Let's use
> > "GLSL ES".
> >
> > 2. How to we want the two different builds of shader_runner to handle the
> > following four possible requirements sections?
> >
> > (a)
> > GLSL >= 130
> >
> > (b)
> > GLSL ES >= 300
> >
> > (c)
> > GLSL >= 130
> > GLSL ES >= 300
> >
> > (d) (no versions specified)
> >
> > My expectation would be:
> > - test (a) should only be run when desktop GLSL version 130 or above is
> > available, and should be skipped when testing GLES.
> > - test (b) should only be run when GLSL ES version 300 or above is
> > available, and should be skipped when testing desktop GL*.
> > - test (c) should be run with GLSL version 130 when testing desktop GL,
> and
> > GLSL ES version 300 when testing GLES*.
> > - test (d) should produce an error message regardless of whether desktop
> GL
> > or GLSL ES is being tested.
> >
> > *At a later date we will probably also to be able to run the GLSL ES
> tests
> > under desktop GL using the ARB_ES{2,3}_compatibility extension, but I'm
> > happy not to worry about that yet :)
> >
> > As written, the patch will run test (a) when desktop GLSL version 130 or
> > above is available, as expected, but it will aso run test (a) when
> testing
> > GLES, which is unlikely to be the test author's intention. Vice versa
> for
> > test (b). Test (c) will work as I would expect. Test (d) will be run on
> > all implementations, which I again think is unlikely to be the author's
> > intention (because in this case the author probably just forgot to
> include
> > the version annotation).
> >
> > 3. A lot of the complexity of the patch has to do with the fact that the
> > version string is inserted into element 0 of the shader_strings array, so
> > the code has to treat it specially, and the special treatment that was
> > previously given to shader_strings[0] has to be shifted over to
> > shader_strings[1]. I don't think we actually have to introduce this
> kind of
> > complication, because it turns out that the only reason that
> shader_strings
> > is an array in the first place is to support a feature that is never used
> > (the ability to specify multiple files in a single "[vertex shader
> file]" or
> > "[fragment shader file]" and have them concatenated). I've talked to Ian
> > about this and we both agree that this feature isn't necessary.
> >
> > 4. It seems inconvenient for every test to have to specify both "GLSL >=
> > xxx" and "GLSL_#version xxx" in the requirements section. My
> recommendation
> > would be to compute the version string based on the "GLSL >= xxx" line,
> and
> > add logic that suppresses autogeneration of a "#version" line if a
> > "#version" line is already present. Note that if we do this we have to
> > decide how to handle requirements of the form "GLSL > xxx", "GLSL < xxx"
> and
> > "GLSL != xxx". I think for now we could just reject those constructs
> (they
> > aren't used anyhow), and if later they turn out to be needed we can
> figure
> > out what to do when we get to that.
> >
> > (Note: eventually Chad and I would like shader_runner to be able to run a
> > "GLSL >= 1.10" test on *all* GLSL versions instead of just one, but I
> think
> > we can leave that for a later task).
> >
> > So I would recommend breaking this into 5 patches:
> >
> > 1. Get rid of the ability to specify multiple files in a single "[vertex
> > shader file]" or "[fragment shader file]", and replace the shader_strings
> > array with a single shader_string.
> > 2. Make shader_runner able to generate a "#version" line (if none is
> already
> > present).
> > 3. If there are any tests that don't have a version requirement line,
> add a
> > "GLSL >= 1.10" requirement (though I don't think there are any).
> > 4. Make shader_runner produce an error if there is no version requirement
> > line.
> > 5. Make shader_runner support "GLSL ES >= xxx" version requirements.
> >
> > A few more specific comments follow.
> >
> >>
> >> ---
> >> tests/shaders/shader_runner.c | 85
> >> +++++++++++++++++++++++++++++-----------
> >> 1 files changed, 61 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/tests/shaders/shader_runner.c
> b/tests/shaders/shader_runner.c
> >> index aa60a62..b9d506a 100644
> >> --- a/tests/shaders/shader_runner.c
> >> +++ b/tests/shaders/shader_runner.c
> >> @@ -48,6 +48,7 @@ extern float piglit_tolerance[4];
> >>
> >> static float gl_version = 0.0;
> >> static float glsl_version = 0.0;
> >> +static float glsl_pound_version = 0.0;
> >> static int gl_max_fragment_uniform_components;
> >> static int gl_max_vertex_uniform_components;
> >>
> >> @@ -69,6 +70,7 @@ GLuint *uniform_block_bos;
> >> * Some test script sections, such as "[vertex shader file]", can
> supply
> >> shader
> >> * source code from multiple disk files. This array stores those
> >> strings.
> >> */
> >> +char shader_ver_string[100];
> >> char *shader_strings[256];
> >> GLsizei shader_string_sizes[256];
> >> unsigned num_shader_strings = 0;
> >> @@ -129,6 +131,16 @@ compile_glsl(GLenum target, bool release_text)
> >> break;
> >> }
> >>
> >> + // Insert version string, if there is one
> >> + if (glsl_pound_version != 0.0f) {
> >> + sprintf(shader_ver_string, "#version %d\n",
> >> + (int)(100.0f * glsl_pound_version));
> >
> >
> > Two minor problems here:
> >
> > 1. This is going to cause rounding errors. For example, 4.1 represented
> as
> > a float is actually 4.0999999, so when you multiply by 100.0 and take the
> > integer part you'll get 409 instead of 410. I'd recommend using round().
> >
> > 2. The correct version string for GLSL ES 1.00 is "#version 100", but the
> > correct version string for GLSL ES 3.00 is "#version 300 es". I think
> it's
> > reasonable to assume that all future versions of GLSL ES will use the
> "es"
> > suffix on the version string as well.
> >
> >>
> >> + } else {
> >> + sprintf(shader_ver_string, "\n");
> >> + }
> >> + shader_strings[0] = shader_ver_string;
> >> + shader_string_sizes[0] = strlen(shader_strings[0]);
> >> +
> >> piglit_ShaderSource(shader, num_shader_strings,
> >> (const GLchar **) shader_strings,
> >> shader_string_sizes);
> >> @@ -155,7 +167,7 @@ compile_glsl(GLenum target, bool release_text)
> >> }
> >>
> >> if (release_text) {
> >> - for (i = 0; i < num_shader_strings; i++)
> >> + for (i = 1; i < num_shader_strings; i++)
> >> free(shader_strings[i]);
> >> }
> >>
> >> @@ -419,11 +431,15 @@ process_requirement(const char *line)
> >> } else if (string_match("!GL_", line)) {
> >> strcpy_to_space(buffer, line + 1);
> >> piglit_require_not_extension(buffer);
> >> - } else if (string_match("GLSL", line)) {
> >> +#if defined USE_OPENGL
> >> + } else if (string_match("GLSL ", line)) {
> >> +#else
> >> + } else if (string_match("ESSL ", line)) {
> >> +#endif
> >> enum comparison cmp;
> >> float version;
> >>
> >> - line = eat_whitespace(line + 4);
> >> + line = eat_whitespace(line + 5);
> >>
> >> line = process_comparison(line, &cmp);
> >>
> >> @@ -435,12 +451,22 @@ process_requirement(const char *line)
> >> version,
> >> glsl_version);
> >> piglit_report_result(PIGLIT_SKIP);
> >> - }
> >> - } else if (string_match("GL", line)) {
> >> + }
> >> +#if defined USE_OPENGL
> >> + } else if (string_match("GLSL_#version", line)) {
> >> +#else
> >> + } else if (string_match("ESSL_#version", line)) {
> >> +#endif
> >> + line = eat_whitespace(line + 14);
> >> +
> >> + glsl_pound_version = strtod(line, NULL);
> >> +
> >> + printf("GLSL #version= %f\n", glsl_pound_version);
> >> + } else if (string_match("GL ", line)) {
> >> enum comparison cmp;
> >> float version;
> >>
> >> - line = eat_whitespace(line + 2);
> >> + line = eat_whitespace(line + 3);
> >>
> >> line = process_comparison(line, &cmp);
> >>
> >> @@ -481,8 +507,8 @@ leave_state(enum states state, const char *line)
> >> break;
> >>
> >> case vertex_shader:
> >> - shader_string_sizes[0] = line - shader_strings[0];
> >> - num_shader_strings = 1;
> >> + shader_string_sizes[1] = line - shader_strings[1];
> >> + num_shader_strings = 2;
> >> compile_glsl(GL_VERTEX_SHADER, false);
> >> break;
> >>
> >> @@ -492,8 +518,8 @@ leave_state(enum states state, const char *line)
> >>
> >> case vertex_program:
> >> compile_and_bind_program(GL_VERTEX_PROGRAM_ARB,
> >> - shader_strings[0],
> >> - line - shader_strings[0]);
> >> + shader_strings[1],
> >> + line - shader_strings[1]);
> >> break;
> >>
> >> case geometry_shader:
> >> @@ -503,8 +529,8 @@ leave_state(enum states state, const char *line)
> >> break;
> >>
> >> case fragment_shader:
> >> - shader_string_sizes[0] = line - shader_strings[0];
> >> - num_shader_strings = 1;
> >> + shader_string_sizes[1] = line - shader_strings[1];
> >> + num_shader_strings = 2;
> >> compile_glsl(GL_FRAGMENT_SHADER, false);
> >> break;
> >>
> >> @@ -514,8 +540,8 @@ leave_state(enum states state, const char *line)
> >>
> >> case fragment_program:
> >> compile_and_bind_program(GL_FRAGMENT_PROGRAM_ARB,
> >> - shader_strings[0],
> >> - line - shader_strings[0]);
> >> + shader_strings[1],
> >> + line - shader_strings[1]);
> >> break;
> >>
> >> case vertex_data:
> >> @@ -629,24 +655,24 @@ process_test_script(const char *script_name)
> >> state = requirements;
> >> } else if (string_match("[vertex shader]",
> line))
> >> {
> >> state = vertex_shader;
> >> - shader_strings[0] = NULL;
> >> + shader_strings[1] = NULL;
> >> } else if (string_match("[vertex program]",
> line))
> >> {
> >> state = vertex_program;
> >> - shader_strings[0] = NULL;
> >> + shader_strings[1] = NULL;
> >> } else if (string_match("[vertex shader file]",
> >> line)) {
> >> state = vertex_shader_file;
> >> - shader_strings[0] = NULL;
> >> - num_shader_strings = 0;
> >> + shader_strings[1] = NULL;
> >> + num_shader_strings = 1;
> >> } else if (string_match("[fragment shader]",
> >> line)) {
> >> state = fragment_shader;
> >> - shader_strings[0] = NULL;
> >> + shader_strings[1] = NULL;
> >> } else if (string_match("[fragment program]",
> >> line)) {
> >> state = fragment_program;
> >> - shader_strings[0] = NULL;
> >> + shader_strings[1] = NULL;
> >> } else if (string_match("[fragment shader
> file]",
> >> line)) {
> >> state = fragment_shader_file;
> >> - shader_strings[0] = NULL;
> >> - num_shader_strings = 0;
> >> + shader_strings[1] = NULL;
> >> + num_shader_strings = 1;
> >> } else if (string_match("[vertex data]", line))
> {
> >> state = vertex_data;
> >> vertex_data_start = NULL;
> >> @@ -671,8 +697,8 @@ process_test_script(const char *script_name)
> >> case geometry_program:
> >> case fragment_shader:
> >> case fragment_program:
> >> - if (shader_strings[0] == NULL)
> >> - shader_strings[0] = (char *)
> line;
> >> + if (shader_strings[1] == NULL)
> >> + shader_strings[1] = (char *)
> line;
> >> break;
> >>
> >> case vertex_shader_file:
> >> @@ -1542,6 +1568,7 @@ piglit_display(void)
> >> void
> >> piglit_init(int argc, char **argv)
> >> {
> >> + const char es_glsl_skip[] = "OpenGL ES GLSL ES ";
> >> const char *glsl_version_string;
> >>
> >> piglit_require_GLSL();
> >> @@ -1550,8 +1577,18 @@ piglit_init(int argc, char **argv)
> >>
> >> glsl_version_string = (char *)
> >> glGetString(GL_SHADING_LANGUAGE_VERSION);
> >> +
> >> +#if defined USE_OPENGL
> >> glsl_version = (glsl_version_string == NULL)
> >> ? 0.0 : strtod(glsl_version_string, NULL);
> >> +#else
> >> + if (glsl_version_string == NULL ||
> >> + strlen(glsl_version_string) <= strlen(es_glsl_skip)) {
> >> + glsl_version = 0.0;
> >> + } else {
> >> + glsl_version =
> strtod(&glsl_version_string[strlen(es_glsl_skip)],
> >> NULL);
> >> + }
> >> +#endif
> >>
> >> glGetIntegerv(GL_MAX_FRAGMENT_UNIFORM_COMPONENTS,
> >> &gl_max_fragment_uniform_components);
> >> --
> >> 1.7.7.3
> >>
> >> _______________________________________________
> >> Piglit mailing list
> >> Piglit at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/piglit
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120827/30992c2b/attachment-0001.html>
More information about the Piglit
mailing list