Bug 51725 – vertexAttribPointer should raise INVALID_OPERATION if stride/offset is not multiple of the type size

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

See WebGL spec section 6.3

[Zhenyao Mo]

Created attachment 77646 [details]
Patch

[Kenneth Russell]

Comment on attachment 77646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77646&action=review

The code changes look good and the test case certainly looks thorough, but the number of test cases being run are massive and I don't think all of the variants really increase the code coverage. In the WebGL test suite each pass line will act as an independent test case so this single patch will drastically increase the number of tests that we claim to run. I think you should look for ways to reduce the number of variants tested per type. A few related and unrelated comments.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:60
> +        "(" + count + ") " +

I don't think it's a good idea to include this count in the reported string. Adding or removing any test cases later will then cause huge spurious changes in the expected.txt file.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:86
> +      for (var off = 0; off < 4; ++off) {

The "4" here seems pretty arbitrary. Is there a strong reason for it?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:87
> +        for (var ss = 0; ss < info.bytesPerComponent; ++ss) {

How about a more descriptive name than "ss" like "offsetStep"?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:89
> +          for (var n = 0; n < 2; ++n) {

Testing both true and false values for the normalize argument doesn't really improve the test coverage and doubles the number of lines in the expected.txt file (making it harder to locate errors) so I'd suggest taking it out and just always passing down false.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:90
> +            for (var st = 0; st < bytesPerElement * 2; ++st) {

Just use "var stride = 0; ..." here. Also, is there a reason to test all of these variants up to stride = 2 * bytesPerElement? Does doing so really increase the test coverage?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:114
> +                  gl, gl.NO_ERROR, "at stide limit",

stide -> stride

[Zhenyao Mo]

I simply synced with khronos for the test.  Now I look more carefully into the test agree with you.  What should have happened is the tests checked into khronos should have been reviewed.

I'll get a patch for review for the khronos side test.  Once it's checked in, I'll do another sync here.

(In reply to comment #2)
> (From update of attachment 77646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77646&action=review
> 
> The code changes look good and the test case certainly looks thorough, but the number of test cases being run are massive and I don't think all of the variants really increase the code coverage. In the WebGL test suite each pass line will act as an independent test case so this single patch will drastically increase the number of tests that we claim to run. I think you should look for ways to reduce the number of variants tested per type. A few related and unrelated comments.
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:60
> > +        "(" + count + ") " +
> 
> I don't think it's a good idea to include this count in the reported string. Adding or removing any test cases later will then cause huge spurious changes in the expected.txt file.
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:86
> > +      for (var off = 0; off < 4; ++off) {
> 
> The "4" here seems pretty arbitrary. Is there a strong reason for it?
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:87
> > +        for (var ss = 0; ss < info.bytesPerComponent; ++ss) {
> 
> How about a more descriptive name than "ss" like "offsetStep"?
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:89
> > +          for (var n = 0; n < 2; ++n) {
> 
> Testing both true and false values for the normalize argument doesn't really improve the test coverage and doubles the number of lines in the expected.txt file (making it harder to locate errors) so I'd suggest taking it out and just always passing down false.
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:90
> > +            for (var st = 0; st < bytesPerElement * 2; ++st) {
> 
> Just use "var stride = 0; ..." here. Also, is there a reason to test all of these variants up to stride = 2 * bytesPerElement? Does doing so really increase the test coverage?
> 
> > LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:114
> > +                  gl, gl.NO_ERROR, "at stide limit",
> 
> stide -> stride

[Zhenyao Mo]

Created attachment 78082 [details]
Patch

[Zhenyao Mo]

(In reply to comment #4)
> Created an attachment (id=78082) [details]
> Patch

Difference from the previous patch:

1) Test was simplified, committed to khronos (reviewed by kbr and gman @google), and copied here.
2) No longer generate an error if stride < bytesPerElement.

[Kenneth Russell]

Comment on attachment 78082 [details]
Patch

Looks good.

[Zhenyao Mo]

Committed r75195: <http://trac.webkit.org/changeset/75195>