[Piglit] [PATCH] internalformat_query(2): fix SAMPLES pname checks for RGB9_E5
Antia Puentes
apuentes at igalia.com
Wed Feb 21 11:10:35 UTC 2018
On 21/02/18 08:08, Alejandro Piñeiro wrote:
> On 20/02/18 17:28, Antia Puentes wrote:
>>
>> 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).
> Hmm, but this are the tests for arb_internalformat_query2. So if the
> under parenthesis is to point that are changes on two tests, it would be
> something like "arb_internalformat_query2(2)", that looks weird. I think
> that it is not needed the number on parenthesis, and I would go for
> just "arb_internalformat_query2"
I see that the name I chose is confusing. I will use
arb_internalformat_query* then
because the patch modifies query1 and query2 tests as I commented.
>>> 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
>>
>
More information about the Piglit
mailing list