[Piglit] [PATCH] built-in-constants: check required GLSL versions for #extensions

[Ilia Mirkin]

On Tue, Nov 14, 2017 at 2:26 PM, Juan A. Suarez Romero
<jasuarez at igalia.com> wrote:
> On Mon, 2017-11-13 at 13:24 -0500, Ilia Mirkin wrote:
>> On Mon, Nov 13, 2017 at 12:48 PM, Juan A. Suarez Romero
>> <jasuarez at igalia.com> wrote:
>> > GL_OES_geometry_shader and GL_OES_tessellation_shader specifications
>> > require OpenGL ES Shading Language 3.10.
>> >
>> > GL_ARB_tessellation_shader specification requires GLSL 1.50.
>> > ---
>> >  tests/shaders/built-in-constants.c | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/tests/shaders/built-in-constants.c b/tests/shaders/built-in-constants.c
>> > index d470fe1bf..6ad26dccf 100644
>> > --- a/tests/shaders/built-in-constants.c
>> > +++ b/tests/shaders/built-in-constants.c
>> > @@ -386,7 +386,7 @@ create_shader(GLenum type)
>> >                         return 0;
>> >         } else {
>> >                 if (is_tessellation_type(type) &&
>> > -                   (required_glsl_version < 400 &&
>> > +                   (required_glsl_version < 150 ||
>> >                      !piglit_is_extension_supported("GL_ARB_tessellation_shader")))
>>
>> I believe this code was correct as-is. With your version, you'll
>> disallow a shader with #version 400 when the impl doesn't advertise
>> the separate GL_ARB_tessellation_shader (which there's no requirement
>> it advertise, AFAIK).
>>
>
> Yes, you're right. I think this hunk was added in the patchset by
> mistake.
>
>
>> The one thing is that it'll allow shaders through with
>> required_glsl_version < 150 if the impl supports tess, but ... hard to
>> care about that.
>>
>> >                         return 0;
>> >
>> > @@ -461,7 +461,7 @@ piglit_init(int argc, char **argv)
>> >         /* Geometry shaders must use the #extension directive in GLSL ES
>> >          * before version 3.20.
>> >          */
>> > -       if (es_shader && required_glsl_version < 320 &&
>> > +       if (es_shader && required_glsl_version == 310 &&
>> >             piglit_is_extension_supported("GL_OES_geometry_shader")) {
>> >                 assert(num_required_extensions < ARRAY_SIZE(required_extensions));
>> >                 required_extensions[num_required_extensions] =
>>
>> What situation are you trying to fix here?
>>
>> > @@ -473,7 +473,9 @@ piglit_init(int argc, char **argv)
>> >         const char *const tess_ext_name = es_shader
>> >                 ? "GL_OES_tessellation_shader"
>> >                 : "GL_ARB_tessellation_shader";
>> > -       if (piglit_is_extension_supported(tess_ext_name)) {
>> > +       if (((es_shader && required_glsl_version == 310) ||
>> > +           (!es_shader && required_glsl_version >= 150)) &&
>> > +           piglit_is_extension_supported(tess_ext_name)) {
>> >                 assert(num_required_extensions < ARRAY_SIZE(required_extensions));
>> >                 required_extensions[num_required_extensions] =
>> >                         strdup(tess_ext_name);
>>
>> And here?
>>
>
>
> First all, I'll push a new version of the patch, as I did some changes
> there.
>
>
> The fix I am trying to do is that in order to declare
> OES_tessellation_shader or OES_geometry_shader extensions in the
> shader, OpenGL ES Shading Language must be 3.10 or newer.
>
> Thus we need to avoid adding those extensions in the shader code on
> older versions.

Is this for a hypothetical implementation that advertises
GL_OES_geometry_shader but not ES 3.1?