Closed Bug 911394 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla28
Tracking Flags:
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)

webgl-simple-clipspace-strict.html
2.98 KB, text/html
Details
patch: Only do check for zero-divisor active attrib arrays on Instanced functions
7.86 KB, patch
bjacob
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
patch: Add tests for array-less drawing
5.42 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
testcase: standalone version of mochitest
12.24 KB, text/html
Details
patch: Mark ANGLE and Android as passing the drawElements tests, since their drivers seem to just handle it.
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?

    
  

          tracking-firefox26:
          --- → ?

    
    

    
  


  
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

          status-firefox26:
          fixed → verified

          status-firefox27:
          fixed → 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: