On Sun, Oct 22, 2017 at 12:33 PM, Alejandro Piñeiro
<apinhe...@igalia.com> wrote:
> 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.
This test does use glGetTexParameter(GL_TEXTURE_BUFFER).
Marek
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
