I also noted that the commit message says "internalformat_query(2)". Not sure why it uses parenthesis, shouldn't be just "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
