[Piglit] [PATCH] internalformat_query(2): fix SAMPLES pname checks for RGB9_E5
Antia Puentes
apuentes at igalia.com
Tue Feb 20 16:30:57 UTC 2018
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").
I could use tex_, its use is more extended as shortname for texture
than text_.
>
>> + 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.
Oh yes, I will change it.
>
>> + 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>
>
>
>
More information about the Piglit
mailing list