DSA: fix error value for *TextureSubImage* when target doesn't match

Submitted by Arthur Huillet on April 22, 2015, 3:25 p.m.

Details

Message ID 1429716333-10814-2-git-send-email-arthur.huillet@free.fr
State New
Headers show

Not browsing as part of any series.

Commit Message

Arthur Huillet April 22, 2015, 3:25 p.m.
From: Arthur Huillet <ahuillet@nvidia.com>

Section 8.6 of the OpenGL 4.5 compatibility specification states:
"An INVALID_OPERATION error is generated by *TextureSubImage* if
the effective target of texture does not match the command, as shown in ta-
ble 8.23."

Piglit was expecting INVALID_ENUM.

Signed-off-by: Arthur Huillet <ahuillet@nvidia.com>
---
 tests/spec/arb_direct_state_access/texture-errors.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/spec/arb_direct_state_access/texture-errors.c b/tests/spec/arb_direct_state_access/texture-errors.c
index c79fbc4..fa93ee4 100644
--- a/tests/spec/arb_direct_state_access/texture-errors.c
+++ b/tests/spec/arb_direct_state_access/texture-errors.c
@@ -87,18 +87,22 @@  test_pos_and_sizes(void)
 	glCopyTextureSubImage2D(name, 2, 0, 0, 0, 0, 4, 4);
 	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
 
-	/* To test 1D and 3D entry points, let's try using the wrong functions. */
+	/* To test 1D and 3D entry points, let's try using the wrong functions.
+       Section 8.6 of the OpenGL 4.5 compatibility specification states:
+       "An INVALID_OPERATION error is generated by *TextureSubImage* if
+       the effective target of texture does not match the command, as shown
+       in table 8.23."*/
 	glTextureSubImage1D(name, 0, 0, 4, GL_RGBA, GL_FLOAT, NULL);
-	pass &= piglit_check_gl_error(GL_INVALID_ENUM);
+	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
 
 	glTextureSubImage3D(name, 0, 0, 0, 0, 4, 4, 4, GL_RGBA, GL_FLOAT, NULL);
-	pass &= piglit_check_gl_error(GL_INVALID_ENUM);
+	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
 
 	glCopyTextureSubImage1D(name, 0, 0, 0, 0, 4);
-	pass &= piglit_check_gl_error(GL_INVALID_ENUM);
+	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
 
 	glCopyTextureSubImage3D(name, 0, 0, 0, 0, 0, 0, 4, 4);
-	pass &= piglit_check_gl_error(GL_INVALID_ENUM);
+	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
 
 	return pass;
 }

Comments

From the ARB_dsa spec:

    An INVALID_ENUM error is generated by *TexSubImage* if <target> does
    not match the command, as shown in table 8.subtarg.

    An INVALID_OPERATION error is generated by *TextureSubImage* if the
    effective target of <texture> does not match the command, as shown in
    table 8.subtarg.

Probably the source of the confusion? AFAIK a bunch of spec bugs were
filed about these little inconsistencies... (But I don't know exactly
what they were, no Khronos access for me.)

On Wed, Apr 22, 2015 at 11:25 AM, Arthur Huillet <arthur.huillet@free.fr> wrote:
> From: Arthur Huillet <ahuillet@nvidia.com>
>
> Section 8.6 of the OpenGL 4.5 compatibility specification states:
> "An INVALID_OPERATION error is generated by *TextureSubImage* if
> the effective target of texture does not match the command, as shown in ta-
> ble 8.23."
>
> Piglit was expecting INVALID_ENUM.
>
> Signed-off-by: Arthur Huillet <ahuillet@nvidia.com>
> ---
>  tests/spec/arb_direct_state_access/texture-errors.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/spec/arb_direct_state_access/texture-errors.c b/tests/spec/arb_direct_state_access/texture-errors.c
> index c79fbc4..fa93ee4 100644
> --- a/tests/spec/arb_direct_state_access/texture-errors.c
> +++ b/tests/spec/arb_direct_state_access/texture-errors.c
> @@ -87,18 +87,22 @@ test_pos_and_sizes(void)
>         glCopyTextureSubImage2D(name, 2, 0, 0, 0, 0, 4, 4);
>         pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
> -       /* To test 1D and 3D entry points, let's try using the wrong functions. */
> +       /* To test 1D and 3D entry points, let's try using the wrong functions.
> +       Section 8.6 of the OpenGL 4.5 compatibility specification states:
> +       "An INVALID_OPERATION error is generated by *TextureSubImage* if
> +       the effective target of texture does not match the command, as shown
> +       in table 8.23."*/
>         glTextureSubImage1D(name, 0, 0, 4, GL_RGBA, GL_FLOAT, NULL);
> -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
>         glTextureSubImage3D(name, 0, 0, 0, 0, 4, 4, 4, GL_RGBA, GL_FLOAT, NULL);
> -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
>         glCopyTextureSubImage1D(name, 0, 0, 0, 0, 4);
> -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
>         glCopyTextureSubImage3D(name, 0, 0, 0, 0, 0, 0, 4, 4);
> -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
>         return pass;
>  }
> --
> 2.3.5
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
Hi,

On 2015-04-22 18:37, Ilia Mirkin wrote:
> From the ARB_dsa spec:
> 
>     An INVALID_ENUM error is generated by *TexSubImage* if <target> 
> does
>     not match the command, as shown in table 8.subtarg.
> 
>     An INVALID_OPERATION error is generated by *TextureSubImage* if the
>     effective target of <texture> does not match the command, as shown 
> in
>     table 8.subtarg.
> 
> Probably the source of the confusion?

Yes, I believe this was the source of the confusion. At least it 
confused *me* for a few minutes :) That's why I'm suggesting the policy 
to always quote spec, so that reviewers and users can more easily spot 
these problems.

> AFAIK a bunch of spec bugs were
> filed about these little inconsistencies... (But I don't know exactly
> what they were, no Khronos access for me.)

I haven't seen one. I haven't looked very hard.

Does the patch look OK to you?
On Wed, Apr 22, 2015 at 12:53 PM, Arthur Huillet <arthur.huillet@free.fr> wrote:
> Hi,
>
> On 2015-04-22 18:37, Ilia Mirkin wrote:
>>
>> From the ARB_dsa spec:
>>
>>     An INVALID_ENUM error is generated by *TexSubImage* if <target> does
>>     not match the command, as shown in table 8.subtarg.
>>
>>     An INVALID_OPERATION error is generated by *TextureSubImage* if the
>>     effective target of <texture> does not match the command, as shown in
>>     table 8.subtarg.
>>
>> Probably the source of the confusion?
>
>
> Yes, I believe this was the source of the confusion. At least it confused
> *me* for a few minutes :) That's why I'm suggesting the policy to always
> quote spec, so that reviewers and users can more easily spot these problems.

In a universe where you're getting too many piglit contributions and
have you do quality control *somehow*, sure. But in this one, getting
people to write tests is like pulling teeth (and I'm guilty of this as
well), so adding even more onerous requirements on test cases seems
like it might be a losing proposition.

>
>> AFAIK a bunch of spec bugs were
>> filed about these little inconsistencies... (But I don't know exactly
>> what they were, no Khronos access for me.)
>
>
> I haven't seen one. I haven't looked very hard.
>
> Does the patch look OK to you?

Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
Martin perhaps, who reviewed a lot of these) about whether this was
done on purpose or not, pending a spec fix.

  -ilia
[fixing Laura's email]

On Wed, Apr 22, 2015 at 1:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Wed, Apr 22, 2015 at 12:53 PM, Arthur Huillet <arthur.huillet@free.fr> wrote:
>> Hi,
>>
>> On 2015-04-22 18:37, Ilia Mirkin wrote:
>>>
>>> From the ARB_dsa spec:
>>>
>>>     An INVALID_ENUM error is generated by *TexSubImage* if <target> does
>>>     not match the command, as shown in table 8.subtarg.
>>>
>>>     An INVALID_OPERATION error is generated by *TextureSubImage* if the
>>>     effective target of <texture> does not match the command, as shown in
>>>     table 8.subtarg.
>>>
>>> Probably the source of the confusion?
>>
>>
>> Yes, I believe this was the source of the confusion. At least it confused
>> *me* for a few minutes :) That's why I'm suggesting the policy to always
>> quote spec, so that reviewers and users can more easily spot these problems.
>
> In a universe where you're getting too many piglit contributions and
> have you do quality control *somehow*, sure. But in this one, getting
> people to write tests is like pulling teeth (and I'm guilty of this as
> well), so adding even more onerous requirements on test cases seems
> like it might be a losing proposition.
>
>>
>>> AFAIK a bunch of spec bugs were
>>> filed about these little inconsistencies... (But I don't know exactly
>>> what they were, no Khronos access for me.)
>>
>>
>> I haven't seen one. I haven't looked very hard.
>>
>> Does the patch look OK to you?
>
> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
> Martin perhaps, who reviewed a lot of these) about whether this was
> done on purpose or not, pending a spec fix.
>
>   -ilia
On 22/04/15 20:06, Ilia Mirkin wrote:
> [fixing Laura's email]
>
> On Wed, Apr 22, 2015 at 1:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Wed, Apr 22, 2015 at 12:53 PM, Arthur Huillet <arthur.huillet@free.fr> wrote:
>>> Hi,
>>>
>>> On 2015-04-22 18:37, Ilia Mirkin wrote:
>>>>  From the ARB_dsa spec:
>>>>
>>>>      An INVALID_ENUM error is generated by *TexSubImage* if <target> does
>>>>      not match the command, as shown in table 8.subtarg.
>>>>
>>>>      An INVALID_OPERATION error is generated by *TextureSubImage* if the
>>>>      effective target of <texture> does not match the command, as shown in
>>>>      table 8.subtarg.
>>>>
>>>> Probably the source of the confusion?
>>>
>>> Yes, I believe this was the source of the confusion. At least it confused
>>> *me* for a few minutes :) That's why I'm suggesting the policy to always
>>> quote spec, so that reviewers and users can more easily spot these problems.
>> In a universe where you're getting too many piglit contributions and
>> have you do quality control *somehow*, sure. But in this one, getting
>> people to write tests is like pulling teeth (and I'm guilty of this as
>> well), so adding even more onerous requirements on test cases seems
>> like it might be a losing proposition.
>>
>>>> AFAIK a bunch of spec bugs were
>>>> filed about these little inconsistencies... (But I don't know exactly
>>>> what they were, no Khronos access for me.)
>>>
>>> I haven't seen one. I haven't looked very hard.
>>>
>>> Does the patch look OK to you?
>> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
>> Martin perhaps, who reviewed a lot of these) about whether this was
>> done on purpose or not, pending a spec fix.

I am not aware of any bug report filed by Laura on this. Given that she 
wrote this code in October last year, I am sure she would have received 
an answer from Khronos if she had filed a bug. This would suggest it is 
a bug in the test.

Let's wait for her comment to make sure of it.

Anyway, thanks for the patch Arthur! I know it can be frustrating to 
have to check every failing test while wondering whether this is a test 
or a driver bug. We should encourage developers writing piglit tests to 
try them on multiple drivers that already support the extension they are 
testing. This is however impossible to mandate.

Do you try your tests against Intel's or AMD's driver for Windows? If 
so, why not start working on them in the open and push them to piglit 
since it now supports Windows?
Hi,

On Thu, 23 Apr 2015 14:26:32 +0300
Martin Peres <martin.peres@linux.intel.com> wrote:

> >>> Does the patch look OK to you?
> >> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
> >> Martin perhaps, who reviewed a lot of these) about whether this was
> >> done on purpose or not, pending a spec fix.
> 
> I am not aware of any bug report filed by Laura on this. Given that she 
> wrote this code in October last year, I am sure she would have received 
> an answer from Khronos if she had filed a bug. This would suggest it is 
> a bug in the test.
> 
> Let's wait for her comment to make sure of it.
> 
> Anyway, thanks for the patch Arthur! I know it can be frustrating to 
> have to check every failing test while wondering whether this is a test 
> or a driver bug. We should encourage developers writing piglit tests to 
> try them on multiple drivers that already support the extension they are 
> testing. This is however impossible to mandate.

Asking Piglit contributors to test their tests (...) on more than one implementation
is certainly a good idea, but it's not always practical. I think it is easier to ask,
for the tests for which it is appropriate (like the ones I fixed), to quote the part
of the specification that they are testing. This also has the advantage of making 
the reviewers' life easier - I don't think many people know by heart what error 
is supposed to be returned by a particular error condition, so having a piece of 
spec to check is helpful.
I'll go a little further and suggest that in order to review a "GL_INVALID_*" 
test properly, you *need* to look at the spec. So better quote it directly.
 
Here are more examples from DSA:

"bind-texture-unit.c:63:

    glGenTextures(1, &name);
    glBindTextureUnit(0, name);
    pass &= piglit_check_gl_error(GL_INVALID_ENUM);

But here's the spec:
    "An INVALID_OPERATION error is generated by BindTextureUnit if tex-
ture is not zero or the name of an existing texture object."

So I believe INVALID_OPERATION is the correct thing to return (will send a patch
when I can).

Then, bind-texture-unit.c:70:

    /* Texture unit doesn't exist */

    glDeleteTextures(1, &name);
    glCreateTextures(GL_TEXTURE_2D, 1, &name);
    glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &nunits);
    glBindTextureUnit(nunits, name); /* Too High */
    pass &= piglit_check_gl_error(GL_INVALID_OPERATION);

I haven't been able to find the relevant language to justify
GL_INVALID_OPERATION here. (NVIDIA returns INVALID_VALUE)
I won't send a patch since I can't back up my claim with spec."

> Do you try your tests against Intel's or AMD's driver for Windows? If 
> so, why not start working on them in the open and push them to piglit 
> since it now supports Windows?

I'm a Linux engineer and if I can avoid working on Windows, I will. :)
On 23/04/15 15:12, Arthur Huillet wrote:
> Hi,
>
> On Thu, 23 Apr 2015 14:26:32 +0300
> Martin Peres <martin.peres@linux.intel.com> wrote:
>
>>>>> Does the patch look OK to you?
>>>> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
>>>> Martin perhaps, who reviewed a lot of these) about whether this was
>>>> done on purpose or not, pending a spec fix.
>> I am not aware of any bug report filed by Laura on this. Given that she
>> wrote this code in October last year, I am sure she would have received
>> an answer from Khronos if she had filed a bug. This would suggest it is
>> a bug in the test.
>>
>> Let's wait for her comment to make sure of it.
>>
>> Anyway, thanks for the patch Arthur! I know it can be frustrating to
>> have to check every failing test while wondering whether this is a test
>> or a driver bug. We should encourage developers writing piglit tests to
>> try them on multiple drivers that already support the extension they are
>> testing. This is however impossible to mandate.
> Asking Piglit contributors to test their tests (...) on more than one implementation
> is certainly a good idea, but it's not always practical. I think it is easier to ask,
> for the tests for which it is appropriate (like the ones I fixed), to quote the part
> of the specification that they are testing. This also has the advantage of making
> the reviewers' life easier - I don't think many people know by heart what error
> is supposed to be returned by a particular error condition, so having a piece of
> spec to check is helpful.

I think we already ask piglit developers to quote the spec at the 
beginning of their tests. I certainly tried to do that on my tests and 
only test the behaviour described in this test. If it has been done, 
then I guess the review was a problem here. It would seem like multiple 
months passed between the moment the patches were written and the moment 
they got commited. I fear that no-one wanted to do a good review so it 
just got pushed.

Sometimes, it is good for readability to re-quote the spec in-line or 
move part of the spec documentation where it is tested (like in this 
test [1]), but this is not always working or possible, like in this test 
[2].

In any case, we indeed need to be careful about these errors.

> I'll go a little further and suggest that in order to review a "GL_INVALID_*"
> test properly, you *need* to look at the spec. So better quote it directly.
>   
> Here are more examples from DSA:
>
> "bind-texture-unit.c:63:
>
>      glGenTextures(1, &name);
>      glBindTextureUnit(0, name);
>      pass &= piglit_check_gl_error(GL_INVALID_ENUM);
>
> But here's the spec:
>      "An INVALID_OPERATION error is generated by BindTextureUnit if tex-
> ture is not zero or the name of an existing texture object."
>
> So I believe INVALID_OPERATION is the correct thing to return (will send a patch
> when I can).
Please do :)

Seems like the test has been written to check for no change in behaviour 
with between the DSA and non-DSA paths instead of a close look at the 
documentation.

>
> Then, bind-texture-unit.c:70:
>
>      /* Texture unit doesn't exist */
>
>      glDeleteTextures(1, &name);
>      glCreateTextures(GL_TEXTURE_2D, 1, &name);
>      glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &nunits);
>      glBindTextureUnit(nunits, name); /* Too High */
>      pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>
> I haven't been able to find the relevant language to justify
> GL_INVALID_OPERATION here. (NVIDIA returns INVALID_VALUE)
> I won't send a patch since I can't back up my claim with spec."

We should not test for unspecified behaviour. That also deserves a patch 
to get rid of the check altogether.
>
>> Do you try your tests against Intel's or AMD's driver for Windows? If
>> so, why not start working on them in the open and push them to piglit
>> since it now supports Windows?
> I'm a Linux engineer and if I can avoid working on Windows, I will. :)

Ah ah, indeed!

I assumed that the tests were written by the windows engineers and 
suggested that they could piglit too. If you also write tests from 
Linux, then you have even less excuses not to make them public in piglit :p



[1] 
http://cgit.freedesktop.org/piglit/commit/?id=8c1d2e8bac5d3526091ca911dd5d4ed0f859d392
[2] 
http://cgit.freedesktop.org/piglit/commit/?id=fa7c2ac879962e3922e7e694955c6879156b7d45
On Wednesday 22 April 2015, Ilia Mirkin wrote:
> From the ARB_dsa spec:
> 
>     An INVALID_ENUM error is generated by *TexSubImage* if <target> does
>     not match the command, as shown in table 8.subtarg.
> 
>     An INVALID_OPERATION error is generated by *TextureSubImage* if the
>     effective target of <texture> does not match the command, as shown in
>     table 8.subtarg.
> 
> Probably the source of the confusion? AFAIK a bunch of spec bugs were
> filed about these little inconsistencies... (But I don't know exactly
> what they were, no Khronos access for me.)

I don't see any inconsistency here.

Section 2.3.1 of the OpenGL specification says:

    If a command that requires an enumerated value is passed a symbolic con-
    stant that is not one of those specified as allowable for that command, an
    INVALID_ENUM error is generated.

In the case of TexSubImage, the <target> parameter has a value that is not
allowed for the command, so INVALID_ENUM is the correct error.

In the case of TextureSubImage, it is the <texture> object that is not
valid for the command, so this makes INVALID_OPERATION the correct error.

> On Wed, Apr 22, 2015 at 11:25 AM, Arthur Huillet <arthur.huillet@free.fr> wrote:
> > From: Arthur Huillet <ahuillet@nvidia.com>
> >
> > Section 8.6 of the OpenGL 4.5 compatibility specification states:
> > "An INVALID_OPERATION error is generated by *TextureSubImage* if
> > the effective target of texture does not match the command, as shown in ta-
> > ble 8.23."
> >
> > Piglit was expecting INVALID_ENUM.
> >
> > Signed-off-by: Arthur Huillet <ahuillet@nvidia.com>
> > ---
> >  tests/spec/arb_direct_state_access/texture-errors.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/spec/arb_direct_state_access/texture-errors.c b/tests/spec/arb_direct_state_access/texture-errors.c
> > index c79fbc4..fa93ee4 100644
> > --- a/tests/spec/arb_direct_state_access/texture-errors.c
> > +++ b/tests/spec/arb_direct_state_access/texture-errors.c
> > @@ -87,18 +87,22 @@ test_pos_and_sizes(void)
> >         glCopyTextureSubImage2D(name, 2, 0, 0, 0, 0, 4, 4);
> >         pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> >
> > -       /* To test 1D and 3D entry points, let's try using the wrong functions. */
> > +       /* To test 1D and 3D entry points, let's try using the wrong functions.
> > +       Section 8.6 of the OpenGL 4.5 compatibility specification states:
> > +       "An INVALID_OPERATION error is generated by *TextureSubImage* if
> > +       the effective target of texture does not match the command, as shown
> > +       in table 8.23."*/
> >         glTextureSubImage1D(name, 0, 0, 4, GL_RGBA, GL_FLOAT, NULL);
> > -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> > +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> >
> >         glTextureSubImage3D(name, 0, 0, 0, 0, 4, 4, 4, GL_RGBA, GL_FLOAT, NULL);
> > -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> > +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> >
> >         glCopyTextureSubImage1D(name, 0, 0, 0, 0, 4);
> > -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> > +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> >
> >         glCopyTextureSubImage3D(name, 0, 0, 0, 0, 0, 0, 4, 4);
> > -       pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> > +       pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> >
> >         return pass;
> >  }
> > --
> > 2.3.5
> > _______________________________________________
> > Piglit mailing list
> > Piglit@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
On Thursday 23 April 2015, Arthur Huillet wrote:
> Hi,
> 
> On Thu, 23 Apr 2015 14:26:32 +0300
> Martin Peres <martin.peres@linux.intel.com> wrote:
> 
> > >>> Does the patch look OK to you?
> > >> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
> > >> Martin perhaps, who reviewed a lot of these) about whether this was
> > >> done on purpose or not, pending a spec fix.
> > 
> > I am not aware of any bug report filed by Laura on this. Given that she 
> > wrote this code in October last year, I am sure she would have received 
> > an answer from Khronos if she had filed a bug. This would suggest it is 
> > a bug in the test.
> > 
> > Let's wait for her comment to make sure of it.
> > 
> > Anyway, thanks for the patch Arthur! I know it can be frustrating to 
> > have to check every failing test while wondering whether this is a test 
> > or a driver bug. We should encourage developers writing piglit tests to 
> > try them on multiple drivers that already support the extension they are 
> > testing. This is however impossible to mandate.
> 
> Asking Piglit contributors to test their tests (...) on more than one implementation
> is certainly a good idea, but it's not always practical. I think it is easier to ask,
> for the tests for which it is appropriate (like the ones I fixed), to quote the part
> of the specification that they are testing. This also has the advantage of making 
> the reviewers' life easier - I don't think many people know by heart what error 
> is supposed to be returned by a particular error condition, so having a piece of 
> spec to check is helpful.
> I'll go a little further and suggest that in order to review a "GL_INVALID_*" 
> test properly, you *need* to look at the spec. So better quote it directly.
>  
> Here are more examples from DSA:
> 
> "bind-texture-unit.c:63:
> 
>     glGenTextures(1, &name);
>     glBindTextureUnit(0, name);
>     pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> 
> But here's the spec:
>     "An INVALID_OPERATION error is generated by BindTextureUnit if tex-
> ture is not zero or the name of an existing texture object."
> 
> So I believe INVALID_OPERATION is the correct thing to return (will send a patch
> when I can).
> 
> Then, bind-texture-unit.c:70:
> 
>     /* Texture unit doesn't exist */
> 
>     glDeleteTextures(1, &name);
>     glCreateTextures(GL_TEXTURE_2D, 1, &name);
>     glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &nunits);
>     glBindTextureUnit(nunits, name); /* Too High */
>     pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> 
> I haven't been able to find the relevant language to justify
> GL_INVALID_OPERATION here. (NVIDIA returns INVALID_VALUE)
> I won't send a patch since I can't back up my claim with spec."

I posted an analysis of these failures here a couple of weeks ago:

http://lists.freedesktop.org/archives/piglit/2015-April/015660.html

> > Do you try your tests against Intel's or AMD's driver for Windows? If 
> > so, why not start working on them in the open and push them to piglit 
> > since it now supports Windows?
> 
> I'm a Linux engineer and if I can avoid working on Windows, I will. :)
> 
>
> I think we already ask piglit developers to quote the spec at the 
> beginning of their tests. I certainly tried to do that on my tests and 
> only test the behaviour described in this test. If it has been done, 
> then I guess the review was a problem here. It would seem like multiple 
> months passed between the moment the patches were written and the moment 
> they got commited. I fear that no-one wanted to do a good review so it 
> just got pushed.
> 
Unfortunately this is a huge problem for piglit, I know many of the DSA
tests sat on the list for several weeks (6+ IIRC) and were just pushed
because no one would review them.

Dylan
Unfortunately I don't have everything in front of me right now, but I know
that Arthur and discussed some of the spec requirements off the mailing
list a month or so ago.  I had some detailed rationale there that could be
added to the tests.

Laura
On Apr 23, 2015 11:11 AM, "Dylan Baker" <baker.dylan.c@gmail.com> wrote:

> > I think we already ask piglit developers to quote the spec at the
> > beginning of their tests. I certainly tried to do that on my tests and
> > only test the behaviour described in this test. If it has been done,
> > then I guess the review was a problem here. It would seem like multiple
> > months passed between the moment the patches were written and the moment
> > they got commited. I fear that no-one wanted to do a good review so it
> > just got pushed.
> >
> Unfortunately this is a huge problem for piglit, I know many of the DSA
> tests sat on the list for several weeks (6+ IIRC) and were just pushed
> because no one would review them.
>
> Dylan
>
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
>