WebGL2 draw_range_elements

RESOLVED DUPLICATE of bug 1202427

Status

()

Core
Canvas: WebGL
RESOLVED DUPLICATE of bug 1202427
5 years ago
a year ago

People

(Reporter: Guillaume Abadie, Assigned: Guillaume Abadie)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-next, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Updated

5 years ago
Blocks: 889977
(Assignee)

Comment 1

5 years ago
Created attachment 781901 [details] [diff] [review]
patch revision 1

After offline discussion with Vlad, our mind is to not verify the start and end parameter while we don't what would be the behavior that should happen.
Attachment #781901 - Flags: review?(jgilbert)
Comment on attachment 781901 [details] [diff] [review]
patch revision 1

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

Can't call DrawRangeElements with an unchecked lower bound, so just always send 0 as `start` until we fix our checks to cache the min value as well.

::: content/canvas/src/WebGLContextVertices.cpp
@@ +706,5 @@
> +     * Therefore we would need to decide what we should do in this case.
> +     */
> +
> +    SetupContextLossTimer();
> +    gl->fDrawRangeElements(mode, start, end, count, type, reinterpret_cast<GLvoid*>(byteOffset));

So, we can't actually allow DrawRangeElements with non-zero `start` until we track minimums. I have half a patch to do this, but haven't had the chance to finish it.

For now, just override `start` with zero.

::: gfx/gl/GLContext.cpp
@@ +666,5 @@
>                  mSymbols.fVertexAttribDivisor = nullptr;
>              }
>          }
>  
> +        if (IsExtensionSupported(EXT_draw_range_elements)) {

This is desktop-only, so we need to load this for GLES3, too.
Attachment #781901 - Flags: review?(jgilbert) → review-
(Assignee)

Updated

5 years ago
Depends on: 900101
(Assignee)

Updated

5 years ago
Depends on: 900113
(Assignee)

Comment 3

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 781901 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 781901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't call DrawRangeElements with an unchecked lower bound, so just always
> send 0 as `start` until we fix our checks to cache the min value as well.
> 
> ::: content/canvas/src/WebGLContextVertices.cpp
> @@ +706,5 @@
> > +     * Therefore we would need to decide what we should do in this case.
> > +     */
> > +
> > +    SetupContextLossTimer();
> > +    gl->fDrawRangeElements(mode, start, end, count, type, reinterpret_cast<GLvoid*>(byteOffset));
> 
> So, we can't actually allow DrawRangeElements with non-zero `start` until we
> track minimums. I have half a patch to do this, but haven't had the chance
> to finish it.
> 
> For now, just override `start` with zero.
Ok.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +666,5 @@
> >                  mSymbols.fVertexAttribDivisor = nullptr;
> >              }
> >          }
> >  
> > +        if (IsExtensionSupported(EXT_draw_range_elements)) {
> 
> This is desktop-only, so we need to load this for GLES3, too.
That is coming 900101 and mVersion stuff in GLContext.
(Assignee)

Comment 4

5 years ago
Created attachment 783864 [details] [diff] [review]
bug_898475_draw_range_elements.patch

The symbol loading for GLES3 have to wait a full support of OpenGL version in GLContext.
Attachment #781901 - Attachment is obsolete: true
Attachment #783864 - Flags: review?(jgilbert)
Comment on attachment 783864 [details] [diff] [review]
bug_898475_draw_range_elements.patch

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

Split adding webgl.drawRangeElements() out into a separate patch, so we can hold on to it in case the WG decides to keep it.

::: content/canvas/src/WebGLContextVertices.cpp
@@ +713,5 @@
> +     * the lowest.
> +     *
> +     * And the behavior if an index isn't in [start, end] is platform specific.
> +     * Therefore we would need to decide what we should do in this case.
> +     */

It's clear that in order to meet the WebGL security requirements, we have to throw an error in this case. Yes, this doesn't really make this function useful, since all WebGL DrawElements calls can be DrawRangeElements calls, given the checks we have to do.

The only question is whether or not we should even expose this redundant command. (Since we can't let `start` or `end` be too wide of a window, we therefore must know the ideal values for `start` and `end` that would minimize this window)

The only reason we'd keep this call is for completeness. Today, I happen to be leaning towards not taking this, though in the past, my opinion was that we might as well have it. It's honestly probably best to wait until the WG weighs in.

That said, the hint parameters added to getParameter() are absolutely worth taking.

::: gfx/gl/GLContext.h
@@ +3087,5 @@
>          AFTER_GL_CALL;
>      }
>  
> +    // EXT_draw_range_elements
> +    void GLAPIENTRY fDrawRangeElements(GLenum mode, GLuint start, GLuint end, GLsizei count, GLenum type, const GLvoid *indices)

No `GLAPIENTRY` in GLContext.h. (Yes there are others in here, but no, they shouldn't be in here)
Attachment #783864 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 6

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 783864 [details] [diff] [review]
> bug_898475_draw_range_elements.patch
> 
> Review of attachment 783864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Split adding webgl.drawRangeElements() out into a separate patch, so we can
> hold on to it in case the WG decides to keep it.
> 
> ::: content/canvas/src/WebGLContextVertices.cpp
> @@ +713,5 @@
> > +     * the lowest.
> > +     *
> > +     * And the behavior if an index isn't in [start, end] is platform specific.
> > +     * Therefore we would need to decide what we should do in this case.
> > +     */
> 
> It's clear that in order to meet the WebGL security requirements, we have to
> throw an error in this case. Yes, this doesn't really make this function
> useful, since all WebGL DrawElements calls can be DrawRangeElements calls,
> given the checks we have to do.
> 
> The only question is whether or not we should even expose this redundant
> command. (Since we can't let `start` or `end` be too wide of a window, we
> therefore must know the ideal values for `start` and `end` that would
> minimize this window)
> 
> The only reason we'd keep this call is for completeness. Today, I happen to
> be leaning towards not taking this, though in the past, my opinion was that
> we might as well have it. It's honestly probably best to wait until the WG
> weighs in.
> 
> That said, the hint parameters added to getParameter() are absolutely worth
> taking.
Ok.
> 
> ::: gfx/gl/GLContext.h
> @@ +3087,5 @@
> >          AFTER_GL_CALL;
> >      }
> >  
> > +    // EXT_draw_range_elements
> > +    void GLAPIENTRY fDrawRangeElements(GLenum mode, GLuint start, GLuint end, GLsizei count, GLenum type, const GLvoid *indices)
> 
> No `GLAPIENTRY` in GLContext.h. (Yes there are others in here, but no, they
> shouldn't be in here)
Oups... I have forgotten to fix that.
(Assignee)

Comment 7

5 years ago
Created attachment 786268 [details] [diff] [review]
patch for standby

Here is the last patch made. Let put this bug for standby.
Attachment #783864 - Attachment is obsolete: true
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> The only reason we'd keep this call is for completeness.

Can you elaborate? I thought that this method could be inherently more efficient than DrawElements.
Whiteboard: webgl-next
Keywords: dev-doc-needed
Blocks: 1266617
No longer blocks: 1266617
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1202427
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.