Closed Bug 911394 Opened 6 years ago Closed 6 years ago

Array-less drawing broken with "Error: WebGL: drawArrays: at least one vertex attribute divisor should be 0"

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 + verified
firefox27 --- verified
b2g-v1.2 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(5 files, 5 obsolete files)

2.98 KB, text/html
Details
7.86 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
5.42 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
12.24 KB, text/html
Details
3.43 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
Looks like a regression from instanced arrays.
Testcase displays points correctly in Chromium, and should still work in older versions of Firefox.
Assignee: nobody → jgilbert
Attachment #805653 - Flags: review?(bjacob)
Blocks: 917046
This one compiles and works.
I'll add a mochitest for this.
Attachment #805653 - Attachment is obsolete: true
Attachment #805653 - Flags: review?(bjacob)
Attachment #805704 - Flags: review?(bjacob)
In writing these tests, I found that we can't currently handle array-less drawing with drawElements. I've marked these tests as 'todo', and we'll handle this case in bug 917616.
Attachment #806351 - Flags: review?(bjacob)
Blocks: 917616
Forgot to qref.
Attachment #806351 - Attachment is obsolete: true
Attachment #806351 - Flags: review?(bjacob)
Attachment #806353 - Flags: review?(bjacob)
Attachment #805704 - Flags: review?(bjacob) → review+
Comment on attachment 806353 [details] [diff] [review]
patch: Add tests for array-less drawing

Review of attachment 806353 [details] [diff] [review]:
-----------------------------------------------------------------

An important comment: why not check getError?

::: content/canvas/test/webgl/non-conf-tests/test_no_arr_points.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Why is this file not a conformance test? Seems pretty conformous to me.

@@ +46,5 @@
> +    // Consider 'random'. Actually, ARMv6 fails, and ARMv7 succeeds, but we have
> +    // not been successful at determining this from JS. (see bug 917478)
> +    return;
> +  }
> +  

Empty lines in this file have a lot of whitespace.

@@ +90,5 @@
> +    // and so try to create a VBO of size INT32_MAX-1 to pretend that vert attrib 0 is an array.
> +    // (INT32_MAX-1 because we check that `first+count` is a valid GLsizei, which is int32_t)
> +    var UINT16_MAX  =         256*256-1;
> +    var INT32_MAX   = 128*256*256*256-1;
> +    var UINT32_MAX  = 256*256*256*256-1;

Why not... but hex is OK too!

@@ +107,5 @@
> +    }
> +    
> +    gl.clear(gl.COLOR_BUFFER_BIT);
> +    gl.drawArrays(gl.POINTS, 0, 1);
> +    ok(!isScreenBlack(), '[' + info + '] drawArrays should color pixels.');

Don't you also/rather want to check that !getError() ?
Attachment #806353 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #6)
> Comment on attachment 806353 [details] [diff] [review]
> patch: Add tests for array-less drawing
> 
> Review of attachment 806353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> An important comment: why not check getError?
> 
> ::: content/canvas/test/webgl/non-conf-tests/test_no_arr_points.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> Why is this file not a conformance test? Seems pretty conformous to me.
At some point, yes. When I talked to Ken about it, he didn't seem thrilled, though. I'd rather have it immediately than have to wait on it.
> 
> @@ +46,5 @@
> > +    // Consider 'random'. Actually, ARMv6 fails, and ARMv7 succeeds, but we have
> > +    // not been successful at determining this from JS. (see bug 917478)
> > +    return;
> > +  }
> > +  
> 
> Empty lines in this file have a lot of whitespace.
My bad, will fix.
> 
> @@ +90,5 @@
> > +    // and so try to create a VBO of size INT32_MAX-1 to pretend that vert attrib 0 is an array.
> > +    // (INT32_MAX-1 because we check that `first+count` is a valid GLsizei, which is int32_t)
> > +    var UINT16_MAX  =         256*256-1;
> > +    var INT32_MAX   = 128*256*256*256-1;
> > +    var UINT32_MAX  = 256*256*256*256-1;
> 
> Why not... but hex is OK too!
Hex is probably better.
> 
> @@ +107,5 @@
> > +    }
> > +    
> > +    gl.clear(gl.COLOR_BUFFER_BIT);
> > +    gl.drawArrays(gl.POINTS, 0, 1);
> > +    ok(!isScreenBlack(), '[' + info + '] drawArrays should color pixels.');
> 
> Don't you also/rather want to check that !getError() ?
We should probably also check getError, but we really do want to be sure that our bizarre draw requests aren't ignored, and that we actually do get pixels to the screen.
Tests are waiting for dependencies.
r+'d by bjacob
Attachment #806353 - Attachment is obsolete: true
Attachment #818748 - Flags: review+
Some cruft snuck into the last one.
Attachment #818748 - Attachment is obsolete: true
Attachment #818751 - Flags: review+
Comment on attachment 805704 [details] [diff] [review]
patch: Only do check for zero-divisor active attrib arrays on Instanced functions

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regressed by webgl instanced drawing
User impact if declined: Various webgl apps and basic tutorials will not function.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #805704 - Flags: approval-mozilla-aurora?
testcase: standalone version of the mochitest
Attachment #818770 - Attachment description: test_no_arr_points.solo.html → testcase: standalone version of mochitest
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/2def80d5a106 - we're still a few minutes away from knowing whether Windows got better from the second patch, but we already know that Android mochitest-gl remains as busted as before.
Jeff - is there a new patch coming for uplift that deals with the bustage in comment 17?
Flags: needinfo?(jgilbert)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #18)
> Jeff - is there a new patch coming for uplift that deals with the bustage in
> comment 17?

We don't need to uplift tests, so:
Yes, there will be a new patch that fixes the bustage.
No, there is nothing to uplift.
Flags: needinfo?(jgilbert)
Keywords: verifyme
(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> (In reply to lsblakk@mozilla.com [:lsblakk] from comment #18)
> > Jeff - is there a new patch coming for uplift that deals with the bustage in
> > comment 17?
> 
> We don't need to uplift tests, so:
> Yes, there will be a new patch that fixes the bustage.
> No, there is nothing to uplift.

This was worded poorly. There is nothing *that was bustage* that needs uplift. The root patch for this bug still needs uplift.
Comment on attachment 805704 [details] [diff] [review]
patch: Only do check for zero-divisor active attrib arrays on Instanced functions

[Triage Comment]
Thanks for clarifying, let's get this onto beta.
Attachment #805704 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Attachment #824157 - Flags: review?(bjacob) → review+
Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 (20131104182142)
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 (20131103004005)

The attached test cases work just like in Firefox 24.0 (unaffected by this bug).
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: ioana.budnar
Also verified on:
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Also verified on Mac OS X 10.8.4 and Windows 7 64bit on the same versions. The issue is also fixed on Mac. On Windows, the reporter's test case works fine, but the mochitest fails on all 3 versions (beta2, 11/03 aurora, 11/05 nightly) :

Test passed: `aPosition` should be valid.
Test passed: [no-attrib] drawArrays should color pixels.
Test passed: [no-attrib] drawArrays[huge first] should color pixels.
Test passed: [no-attrib] gl.getError should be 0, was 0x0.
Test FAILED: Todo: [no-attrib] drawElements[0] should color pixels.
Test FAILED: Todo: [no-attrib] drawElements[1] should color pixels.
Test FAILED: Todo: [no-attrib] drawElements[huge offset] should color pixels.
Test FAILED: Todo: [no-attrib] gl.getError should be 0, was 0x0.
Test passed: [one-attrib, no-array] drawArrays should color pixels.
Test passed: [one-attrib, no-array] drawArrays[huge first] should color pixels.
Test passed: [one-attrib, no-array] gl.getError should be 0, was 0x0.
Test FAILED: Todo: [one-attrib, no-array] drawElements[0] should color pixels.
Test FAILED: Todo: [one-attrib, no-array] drawElements[1] should color pixels.
Test FAILED: Todo: [one-attrib, no-array] drawElements[huge offset] should color pixels.
Test FAILED: Todo: [one-attrib, no-array] gl.getError should be 0, was 0x0.
Yeah, on some platforms (windows, some android?), the driver lets this stuff work, even though we don't do the correct setup, and the driver fails.

'unexpected pass' should largely be ignored.
See Also: → 944440
You need to log in before you can comment on or make changes to this bug.