Closed Bug 982960 Opened 6 years ago Closed 6 years ago

Use DrawRangeElements, since we're already checking bounds

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(2 files, 3 obsolete files)

Since we already check that the bounds are correct, we should just use glDrawRangeElements for calls to webglDrawElements.
Attachment #8390185 - Flags: review?(dglastonbury)
Attachment #8390186 - Flags: review?(dglastonbury)
Attachment #8390185 - Attachment is obsolete: true
Attachment #8390185 - Flags: review?(dglastonbury)
Attachment #8390187 - Flags: review?(dglastonbury)
Alright, so I think the issue is that we're just expecting to be able to pull the global max here, but we don't over-validate and check the whole array if we only care about a small bit.
We should just ask for the max back from Validate, maybe.
Comment on attachment 8390187 [details] [diff] [review]
patch 1: Add GLContext support for draw_range_elements

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

::: gfx/gl/GLContext.h
@@ -2697,5 @@
>      // Store the current context when binding to thread local
>      // storage to support DebugMode on an arbitrary thread.
>      static unsigned sCurrentGLContextTLS;
>  #endif
> -    

Death to rogue ws

::: gfx/gl/GLContextFeatures.cpp
@@ +85,5 @@
>          }
>      },
>      {
> +        "draw_range_elements",
> +        200, // OpenGL version

Apparently EXT_draw_range_elements is available from 1.2, according to http://www.opengl.org/wiki/History_of_OpenGL
Attachment #8390187 - Flags: review?(dglastonbury) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Alright, so I think the issue is that we're just expecting to be able to
> pull the global max here, but we don't over-validate and check the whole
> array if we only care about a small bit.
> We should just ask for the max back from Validate, maybe.

Are you going to fix try and upload another patch?
Comment on attachment 8390186 [details] [diff] [review]
patch 2: Use draw_range_elements in WebGL

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

I understand what the patch is doing, but glDrawRangeElements looks really old and the rationale seems outdated with modern GPUs. Is this change really worth it the, albeit small, added complications?

::: content/canvas/src/WebGLContextVertices.cpp
@@ +694,5 @@
> +    MOZ_ASSERT(elemBuffer);
> +    size_t maxElem = elemBuffer->MaxElemValue(type);
> +
> +    GLuint maxElemUInt = (GLuint)maxElem;
> +    if ((size_t)maxElemUInt != maxElem)

Comment to explain what's going on here wouldn't go amiss.

::: content/canvas/src/WebGLElementArrayCache.cpp
@@ +562,5 @@
> +      return mUint32Tree->GlobalMaximum();
> +    default:
> +      break;
> +  }
> +  

To the god of ...
Attachment #8390186 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #7)
> (In reply to Jeff Gilbert [:jgilbert] from comment #5)
> > Alright, so I think the issue is that we're just expecting to be able to
> > pull the global max here, but we don't over-validate and check the whole
> > array if we only care about a small bit.
> > We should just ask for the max back from Validate, maybe.
> 
> Are you going to fix try and upload another patch?

Yeah, but relatively little is needed to fix this, so it's not a wasted review.
(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8390187 [details] [diff] [review]
> patch 1: Add GLContext support for draw_range_elements
> 
> Review of attachment 8390187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ -2697,5 @@
> >      // Store the current context when binding to thread local
> >      // storage to support DebugMode on an arbitrary thread.
> >      static unsigned sCurrentGLContextTLS;
> >  #endif
> > -    
> 
> Death to rogue ws
> 
> ::: gfx/gl/GLContextFeatures.cpp
> @@ +85,5 @@
> >          }
> >      },
> >      {
> > +        "draw_range_elements",
> > +        200, // OpenGL version
> 
> Apparently EXT_draw_range_elements is available from 1.2, according to
> http://www.opengl.org/wiki/History_of_OpenGL

That's a handy little page. I wouldn't have guessed it'd be that old, but cool. In practice, we don't run on anything less than 200, so it doesn't *really* matter, but it wouldn't hurt to get it right.
Attachment #8390186 - Attachment is obsolete: true
Attachment #8390955 - Flags: review?(dglastonbury)
Attachment #8390955 - Flags: review?(dglastonbury) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24f5c64eb243
https://hg.mozilla.org/mozilla-central/rev/233a33ca8ef8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This (specifically attachment 8390955 [details] [diff] [review]) broke my work in bug 917226, which we're planning to land asap.

I'll try to get a reduced test case, but glReadPixels doesn't seem to work anymore, which sounds really scary. Having preserveDrawingBuffer enabled doesn't seem to make a difference.

Jeff, in the meantime, does anything here look like it could affect glReadPixels?
Blocks: 917226
Flags: needinfo?(jgilbert)
I have found some simple steps to reproduce this:

1. Make sure the "devtools.chrome.enabled" pref is true (to enable js input in the Browser Console)
2. Go to [0]
3. Notice how a wireframe sphere is drawn; this is a very simple WebGL demo that uses glDrawElements
4. Open Browser Console (Accel+Shift+J, or Tools -> Web Developer -> Browser Console)
   --- Make sure you open the Browser Console, not the Web Console ---
5. Paste and run [1] in the Browser Console while [0] is the currently displayed content page

Notice how the sphere isn't drawn anymore, and these messages printed in the console:
> TRANSPARENT FOR glClear: true
> TRANSPARENT FOR glDrawElements: true

That means that glReadPixels returns transparent data after a call to glDrawElements which should've drawn things. This shouldn't happen. The expected messages are:
> TRANSPARENT FOR glClear: true
> TRANSPARENT FOR glDrawElements: false
...and the wireframe sphere should be correctly drawn.

Everything works ok on revision 24f5c64eb243.
Revision 233a33ca8ef8 seems to break things.

[0] https://dl.dropboxusercontent.com/u/2388316/Sgen/test/test_material_texture.html
[1] https://pastebin.mozilla.org/4628812

Jeff, please let me know if I can help with anything else. We really need to land bug 917226 asap, and this regression blocks it big time.
Since Jeff is probably on PTO, asking Dan for feedback as well.
Flags: needinfo?(dglastonbury)
Ah, ok, we're not blitting for DrawRangeElements.
Flags: needinfo?(jgilbert)
Attachment #8393756 - Flags: review?(dglastonbury)
Flags: needinfo?(dglastonbury)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 985685
This regression should really be a separate bug. I create bug 985685 for this.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #8393756 - Attachment is obsolete: true
Attachment #8393756 - Flags: review?(dglastonbury)
You need to log in before you can comment on or make changes to this bug.