[Piglit] [PATCH] internalformat_query(2): fix SAMPLES pname checks for RGB9_E5

[Alejandro Piñeiro]

On 20/02/18 14:54, Antia Puentes wrote:
> SAMPLES and NUM_SAMPLE_COUNTS queries accept internalformats which
> are defined as color-, depth- or stencil-renderable. RGB9_E5 is marked
> as non color-renderable in OpenGL 4.6, however if the
> EXT_texture_shared_exponent extension is exposed it must be considered
> as such. The later was discussed in:
> https://github.com/KhronosGroup/OpenGL-API/issues/32
> ---
>  tests/spec/arb_internalformat_query/api-errors.c   | 16 ++++++++++++++-
>  .../arb_internalformat_query2/samples-pnames.c     | 23 +++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/tests/spec/arb_internalformat_query/api-errors.c b/tests/spec/arb_internalformat_query/api-errors.c
> index a174143a3..eabf6a8d5 100644
> --- a/tests/spec/arb_internalformat_query/api-errors.c
> +++ b/tests/spec/arb_internalformat_query/api-errors.c
> @@ -128,7 +128,10 @@ static const GLenum invalid_formats[] = {
>  	GL_BGRA_INTEGER_EXT,
>  	GL_LUMINANCE_INTEGER_EXT,
>  	GL_LUMINANCE_ALPHA_INTEGER_EXT,
> -	GL_RGB9_E5
> +};
> +
> +static const GLenum text_shared_exponent_formats[] = {

I dont like too much using "text" as the shortname for "texture" (it is
slightly confusing). I would prefer the full "texture" or just remove it
if you think that becomes too long (so "shared_exponent_formats").

> +	GL_RGB9_E5,
>  };
>  
>  static const GLenum valid_pnames[] = {
> @@ -295,6 +298,17 @@ piglit_init(int argc, char **argv)
>  		   GL_INVALID_ENUM)
>  		&& pass;
>  
> +	/* RGB9_E5 is defined as color-renderable if EXT_texture_shared_exponent
> +	 * is exposed, otherwise INVALID_ENUM should be returned.
> +	 */
> +	if (!piglit_is_extension_supported("GL_EXT_texture_shared_exponent")) {
> +		pass = try(valid_targets, ARRAY_SIZE(valid_targets),
> +			   text_shared_exponent_formats, 1,
> +			   valid_pnames, ARRAY_SIZE(valid_pnames),
> +			   GL_INVALID_ENUM)
> +			&& pass;
> +	}
> +
>  	/* The GL_ARB_internalformat_query spec says:
>  	 *
>  	 *     "If the <target> parameter to GetInternalformativ is not one of
> diff --git a/tests/spec/arb_internalformat_query2/samples-pnames.c b/tests/spec/arb_internalformat_query2/samples-pnames.c
> index f6def23d0..48972705e 100644
> --- a/tests/spec/arb_internalformat_query2/samples-pnames.c
> +++ b/tests/spec/arb_internalformat_query2/samples-pnames.c
> @@ -96,7 +96,10 @@ static const GLenum non_renderable_internalformats[] = {
>  	GL_BGRA_INTEGER_EXT,
>  	GL_LUMINANCE_INTEGER_EXT,
>  	GL_LUMINANCE_ALPHA_INTEGER_EXT,
> -	GL_RGB9_E5
> +};
> +
> +static const GLenum text_shared_exponent_formats[] = {
> +	GL_RGB9_E5,
>  };

Ditto (about name)

>  
>  enum piglit_result
> @@ -217,6 +220,15 @@ check_num_sample_counts(void)
>                             GL_NUM_SAMPLE_COUNTS, data)
>                          && pass;
>  
> +		/* RGB9_E5 is not defined as color-renderable unless
> +		 * EXT_texture_shared_exponent is exposed. */

The closing "*/" should be on the next line. I know that we didn't
respect it consistently on this query2 tests, but just checking it is
the common on most piglit tests. Additionally, you do that on the
previous comment. So at least this patch should be consistent with one
option or the other.

> +		if (!piglit_is_extension_supported("GL_EXT_texture_shared_exponent")) {
> +			pass = try(valid_targets, ARRAY_SIZE(valid_targets),
> +				   text_shared_exponent_formats, 1,
> +				   GL_NUM_SAMPLE_COUNTS, data)
> +				&& pass;
> +		}
> +
>                  /*
>                   *        ... or if <target> does not support
>                   *        multiple samples (ie other than
> @@ -266,6 +278,15 @@ check_samples(void)
>                             GL_SAMPLES, data)
>                          && pass;
>  
> +		/* RGB9_E5 is not defined as color-renderable unless
> +		 * EXT_texture_shared_exponent is exposed. */

Ditto (about comment).

> +		if (!piglit_is_extension_supported("GL_EXT_texture_shared_exponent")) {
> +			pass = try(valid_targets, ARRAY_SIZE(valid_targets),
> +				   text_shared_exponent_formats, 1,
> +				   GL_SAMPLES, data)
> +				&& pass;
> +		}
> +
>                  /*
>                   * or if <target> does not support multiple samples (ie other
>                   * than TEXTURE_2D_MULTISAMPLE, TEXTURE_2D_MULTISAMPLE_ARRAY,

With those nitpicks fixed:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>