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

[Alejandro Piñeiro]

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"

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