[Piglit] [PATCH] arb_shader_image_load_store: test duplicate format qualifier more thoroughly

[Francisco Jerez]

Timothy Arceri <timothy.arceri at collabora.com> writes:

> From the ARB_shader_image_load_store spec:
>
>    "Only one format qualifier may be specified for any image variable
>     declaration."
>
> ARB_enhanced_layouts
>
>   Allows duplicates within a single layout qualifier e.g.
>
>   layout(location = 0, location = 1) out vec4 a;
>
> ARB_shading_language_420pack
>
>   Allows multiple layout qualifiers e.g.
>
>   layout(location = 0) layout(location = 2) out vec4 b;
>
> However layout(rgba32f, rgba32f) uniform image2D img;
> and layout(rgba32f) layout(rgba32f) uniform image2D img;
>
> Should still fail when these extensions are enabled according to the
> spec GLSL 4.5 spec.
>
> Cc: Francisco Jerez <currojerez at riseup.net>
> ---
>  .../gen_shader_image_load_store_tests.py           | 39 ++++++++++++++++++----
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/generated_tests/gen_shader_image_load_store_tests.py b/generated_tests/gen_shader_image_load_store_tests.py
> index 8d6be9c..d52070c 100644
> --- a/generated_tests/gen_shader_image_load_store_tests.py
> +++ b/generated_tests/gen_shader_image_load_store_tests.py
> @@ -29,24 +29,28 @@ from textwrap import dedent
>  from modules import utils
>  
>  
> -def gen_header(status):
> +def gen_header(status, extra_extension=''):
>      """
>      Generate a GLSL program header.
>  
>      Generate header code for ARB_shader_image_load_store GLSL parser
>      tests that are expected to give status as result.
>      """
> +    extra_extension_s = ''
> +    if extra_extension != '':
> +        extra_extension_s = "#extension {0}: require".format(extra_extension)
>      return dedent("""\
>          /*
>           * [config]
>           * expect_result: {0}
>           * glsl_version: 1.50
> -         * require_extensions: GL_ARB_shader_image_load_store
> +         * require_extensions: GL_ARB_shader_image_load_store {1}
>           * [end config]
>           */
>          #version 150
>          #extension GL_ARB_shader_image_load_store: require
> -    """.format(status))
> +        {2}
> +    """.format(status, extra_extension, extra_extension_s))
>  
>  

You can probably achieve the same more generally and readably by doing
something like:

| def gen_header(status, extra_extensions=[]):
[...]
|     extensions = ['GL_ARB_shader_image_load_store'] + extra_extensions
|     extension_enables = ['#extension {0}: require'.format(x) for x in extensions]
|
|     return dedent("""\
|         /*
|          * [config]
|          * expect_result: {0}
|          * glsl_version: 1.50
|          * require_extensions: {1}
|          * [end config]
|          */
|          #version 150
|          {2}
|      """).format(status, ' '.join(extensions), '\n'.join(extension_enables))

(note that I've swapped the order of application of dedent() with
respect to format() because the join of extension enables is not going
to give you indented text as result)

>  def gen_vector_type(scalar, dim):
> @@ -458,20 +462,41 @@ gen('declaration-format-qualifier', """\
>       'prefix': ''}
>  ]))
>  
> -gen('declaration-format-qualifier-duplicate', """\
> -    ${header('fail')}
> +gen('declaration-format-qualifier', """\
> +    ${header('fail', extra_extension)}
> +    ${description}

I wouldn't bother to pass a different description for each generated
subtest, it makes it kind of difficult to read the rationale from the
generator source.  I'd simply mention in the comment below that the same
test is repeated with different extension enables which have an
influence on whether duplicate layout qualifiers are considered valid...

>      /*
>       * From the ARB_shader_image_load_store spec:
>       *
>       * "Only one format qualifier may be specified for any image variable
>       *  declaration."
>       */
> -    layout(rgba32f) layout(rgba32f) uniform image2D img;
> +    ${layout_qualifier} uniform image2D img;
>  
>      void main()
>      {
>      }
> -""", shader_stages)
> +""", product(shader_stages, [
> +    {'name': 'duplicate',
> +     'extra_extension': '',
> +     'description': '',
> +     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
> +    {'name': 'duplicate-420pack',
> +     'extra_extension': 'GL_ARB_shading_language_420pack',
> +     'description': '/* Check duplicates still fail with '
> +     'GL_ARB_shading_language_420pack \n * which allows multiple layout '
> +     'qualifers\n */',
> +     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
> +    {'name': 'duplicate-within-layout',
> +     'extra_extension': '',
> +     'description': '',
> +     'layout_qualifier': 'layout(rgba32f, rgba32f)'},
> +    {'name': 'duplicate-within-layout-enhanced-layouts',
> +     'extra_extension': 'GL_ARB_enhanced_layouts',
> +     'description': '/* Check duplicates still fail with ARB_enhanced_layouts'
> +     ' \n * which allows duplicates in a single layout\n */',
> +     'layout_qualifier': 'layout(rgba32f, rgba32f)'}
> +]))
>
You're basically computing part of the cartesian product by hand here,
we could be even lazier like:

| gen('declaration-format-qualifier-duplicate', """\
|     ${header('fail', [extra_extension] if extended else [])}
[...]
| """, product(shader_stages, [
|     {'name': 'single-layout-qualifier',
|      'layout_qualifier': 'layout(rgba32f, rgba32f)',
|      'extra_extension': 'GL_ARB_enhanced_layouts'},
|     {'name': 'multiple-layout-qualifiers',
|      'layout_qualifier': 'layout(rgba32f) layout(rgba32f)',
|      'extra_extension': 'GL_ARB_shading_language_420pack'}
| ], [
|     {'name': 'unextended', 'extended': False },
|     {'name': 'extended', 'extended': True }
| ]))


>  gen('declaration-format-qualifier-missing', """\
>      ${header(status)}
> -- 
> 2.4.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20160119/1ccc2213/attachment.sig>