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

[Antia Puentes]

On 20/02/18 17:18, Alejandro Piñeiro wrote:
> I also noted that the commit message says "internalformat_query(2)". Not
> sure why it uses parenthesis, shouldn't be just "internalformat_query2"?
It includes changes both in the ARB_internalformat_query tests 
(api-errors.c)
and query2 (samples-pnames.c).

> 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
>