On 21/10/17 14:55, Marek Olšák wrote:
> From: Marek Olšák <marek.ol...@amd.com>
>
> This was never tested and it uses functions that are illegal with TBOs,
> like glGetTexParameteriv and glTexImage2DMultisample.
Hi, I didn't check all the patch in detail, but as Illia is saying, it
is correct to pass TEXTURE_BUFFER to GetInternalFormat*. I also disagree
that this case it is not tested, or at least for the case I checked. See
below, I have two extra comments.
> ---
> tests/spec/arb_internalformat_query2/common.h | 4
> +++-
> tests/spec/arb_internalformat_query2/format-components.c | 2 +-
> tests/spec/arb_internalformat_query2/generic-pname-checks.c | 2 +-
> .../spec/arb_internalformat_query2/image-format-compatibility-type.c | 2 +-
> 4 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/spec/arb_internalformat_query2/common.h
> b/tests/spec/arb_internalformat_query2/common.h
> index df68581..0e5e11f 100644
> --- a/tests/spec/arb_internalformat_query2/common.h
> +++ b/tests/spec/arb_internalformat_query2/common.h
> @@ -25,26 +25,28 @@
>
> static const GLenum valid_targets[] = {
> GL_TEXTURE_1D,
> GL_TEXTURE_1D_ARRAY,
> GL_TEXTURE_2D,
> GL_TEXTURE_2D_ARRAY,
> GL_TEXTURE_3D,
> GL_TEXTURE_CUBE_MAP,
> GL_TEXTURE_CUBE_MAP_ARRAY,
> GL_TEXTURE_RECTANGLE,
> - GL_TEXTURE_BUFFER,
> GL_RENDERBUFFER,
> GL_TEXTURE_2D_MULTISAMPLE,
> GL_TEXTURE_2D_MULTISAMPLE_ARRAY,
> + GL_TEXTURE_BUFFER,
> };
>
> +#define NUM_TARGETS_EXCEPT_TBO (ARRAY_SIZE(valid_targets) - 1)
> +
Not sure if this is really needed, as we have texture_targets, defined
on common.h too, that is valid_targets except TEXTURE_BUFFER and
RENDERBUFFER. The latter also need some special handling.
> static const GLenum invalid_targets[] = {
> GL_FRAMEBUFFER,
> GL_COLOR_ATTACHMENT0,
> GL_COLOR_ATTACHMENT1,
> GL_COLOR_ATTACHMENT2,
> GL_COLOR_ATTACHMENT3,
> GL_COLOR_ATTACHMENT4,
> GL_COLOR_ATTACHMENT5,
> GL_COLOR_ATTACHMENT6,
> GL_COLOR_ATTACHMENT7,
> diff --git a/tests/spec/arb_internalformat_query2/format-components.c
> b/tests/spec/arb_internalformat_query2/format-components.c
> index a484e01..983091f 100644
> --- a/tests/spec/arb_internalformat_query2/format-components.c
> +++ b/tests/spec/arb_internalformat_query2/format-components.c
> @@ -244,21 +244,21 @@ check_format_components(void)
> test_data *data = test_data_new(0, 1);
> unsigned i;
> int testing64;
>
> for (i = 0; i < ARRAY_SIZE(pnames); i++) {
> bool pass = true;
>
> for (testing64 = 0; testing64 <= 1; testing64++) {
> test_data_set_testing64(data, testing64);
>
> - pass = try(valid_targets, ARRAY_SIZE(valid_targets),
> + pass = try(valid_targets, NUM_TARGETS_EXCEPT_TBO,
> valid_internalformats,
> ARRAY_SIZE(valid_internalformats),
> pnames[i], data)
> && pass;
> }
>
> piglit_report_subtest_result(pass ? PIGLIT_PASS :
> PIGLIT_FAIL,
> "%s",
> piglit_get_gl_enum_name(pnames[i]));
>
> check_pass = check_pass && pass;
> }
> diff --git a/tests/spec/arb_internalformat_query2/generic-pname-checks.c
> b/tests/spec/arb_internalformat_query2/generic-pname-checks.c
> index e521fac..feb953c 100644
> --- a/tests/spec/arb_internalformat_query2/generic-pname-checks.c
> +++ b/tests/spec/arb_internalformat_query2/generic-pname-checks.c
> @@ -255,21 +255,21 @@ check_basic(const GLenum *pnames, unsigned num_pnames,
> test_data *data = test_data_new(0, 1);
> unsigned i;
> int testing64;
>
> for (i = 0; i < num_pnames; i++) {
> bool pass = true;
>
> for (testing64 = 0; testing64 <= 1; testing64++) {
> test_data_set_testing64(data, testing64);
>
> - pass = try_basic(valid_targets,
> ARRAY_SIZE(valid_targets),
> + pass = try_basic(valid_targets,
> NUM_TARGETS_EXCEPT_TBO,
> valid_internalformats,
> ARRAY_SIZE(valid_internalformats),
> pnames[i],
> possible_values,
> num_possible_values,
> data)
> && pass;
> }
>
> piglit_report_subtest_result(pass ? PIGLIT_PASS :
> PIGLIT_FAIL,
> "%s",
> piglit_get_gl_enum_name(pnames[i]));
>
> diff --git
> a/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
> b/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
> index 28bf292..af46c60 100644
> --- a/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
> +++ b/tests/spec/arb_internalformat_query2/image-format-compatibility-type.c
> @@ -138,21 +138,21 @@ try_local(const GLenum *targets, unsigned num_targets,
> static bool
> check_format_compatibility_type(void)
> {
> bool pass = true;
> test_data *data = test_data_new(0, 1);
> int testing64;
>
> for (testing64 = 0; testing64 <= 1; testing64++) {
> test_data_set_testing64(data, testing64);
>
> - pass = try_local(valid_targets, ARRAY_SIZE(valid_targets),
> + pass = try_local(valid_targets, NUM_TARGETS_EXCEPT_TBO,
Your point is that TEXTURE_BUFFER shouldn't be tested here. But it is
tested in the sense that for this specific pname, querying should return
GL_NONE.
C&P the comment at try_local:
/* From the spec:
*
* "- IMAGE_FORMAT_COMPATIBILITY_TYPE: The matching criteria use for
* the resource when used as an image textures is returned in
* <params>. This is equivalent to calling GetTexParameter with
* <value> set to IMAGE_FORMAT_COMPATIBILITY_TYPE. Possible values
* are IMAGE_FORMAT_COMPATIBILITY_BY_SIZE or
* IMAGE_FORMAT_COMPATIBILITY_BY_CLASS. If the resource is not
* supported for image textures, or if image textures are not
* supported, NONE is returned."
*
* So try_local is equivalent to try_basic, except that instead of
* checking against a list of possible value, we test against the
* value returned by GetTexParameter, or against GL_NONE if not
* supported of if it is not a texture.
*/
So in this case, TBO is tested (or should be, unless a bug on the test),
but the test is just to confirm that we get a GL_NONE. On the code is
checked against texture_targets. As it doesn't belong there, it is not
used glGetTexParameteriv and just checks if GL_NONE is returned.
I still need to check the other cases though.
> valid_internalformats,
> ARRAY_SIZE(valid_internalformats),
> GL_IMAGE_FORMAT_COMPATIBILITY_TYPE,
> data)
> && pass;
> }
>
> piglit_report_subtest_result(pass ? PIGLIT_PASS : PIGLIT_FAIL,
> "%s",
> piglit_get_gl_enum_name(GL_IMAGE_FORMAT_COMPATIBILITY_TYPE));
>
> test_data_clear(&data);
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
