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

[Alejandro Piñeiro]

I also noted that the commit message says "internalformat_query(2)". Not
sure why it uses parenthesis, shouldn't be just "internalformat_query2"?

On 20/02/18 17:14, Alejandro Piñeiro wrote:
>
> 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>
>
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit