implement http://www.opengl.org/registry/specs/EXT/draw_range_elements.txt in WebGL 2

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.

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.

(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.

The symbol loading for GLES3 have to wait a full support of OpenGL version in GLContext.

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)

(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.

Here is the last patch made. Let put this bug for standby.

(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.