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 <apinhe...@igalia.com> >>> >>> >>> _______________________________________________ >>> Piglit mailing list >>> Piglit@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/piglit >> > > _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
