Instanced Rendering See https://wiki.mozilla.org/Platform/GFX/WebGL2

patch revision 2

This fix patch also fix and re-enable a vertex array object.

(In reply to Guillaume Abadie from comment #3) > Created attachment 778458 [details] [diff] [review] > patch revision 3 > > This fix patch also fix and re-enable a vertex array object. Please split this into two patches (and a different bug) unless there's a compelling reason these can't be separated.

Comment on attachment 779200 [details] [diff] [review] patch revision 4 Review of attachment 779200 [details] [diff] [review]: ----------------------------------------------------------------- Needs the exts dealt with here to be fixed. ::: content/canvas/src/WebGLContext.h @@ +777,5 @@ > WebGLintptr byteOffset); > void Viewport(WebGLint x, WebGLint y, WebGLsizei width, WebGLsizei height); > > + // Vertices Feature (WebGLContextVertices.cpp) > + bool DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info); Why are these *_check functions public? ::: content/canvas/src/WebGLContextValidate.cpp @@ +1062,5 @@ > (!IsExtensionSupported(OES_vertex_array_object) || > !IsExtensionSupported(WEBGL_draw_buffers) || > !gl->IsExtensionSupported(gl::GLContext::EXT_gpu_shader4) || > + !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) || > + !gl->IsExtensionSupported(gl::GLContext::ARB_draw_instanced) Missing EXT_draw_instanced, as well as the other GLES exts I mentioned elsewhere. ::: content/canvas/src/WebGLContextVertices.cpp @@ +43,5 @@ > + if (!ValidateBuffers(&maxAllowedCount, info)) { > + return false; > + } > + > + CheckedUint32 checked_firstPlusCount = CheckedUint32(first) + count; You're going to have to rebase this on top of the change we're making here from Uint32 to Int32. @@ +87,5 @@ > + > + SetupContextLossTimer(); > + gl->fDrawArrays(mode, first, count); > + > + UndoFakeVertexAttrib0(); Everything this line and below in this function is repeated in the three other draw calls. Let's pull it out into a helper function. @@ +163,5 @@ > + } > + > + CheckedUint32 checked_byteCount; > + > + WebGLsizei first = 0; Why do we init `first` but not `checked_byteCount`? @@ +173,5 @@ > + return false; > + } > + first = byteOffset / 2; > + } > + else if (type == LOCAL_GL_UNSIGNED_BYTE) { Let's put this condition first, so that we're increasing from 1->2->4 in the cases we handle. @@ +207,5 @@ > + return false; > + } > + > + if (!mBoundVertexArray->mBoundElementArrayBuffer->ByteLength()) { > + ErrorInvalidOperation("%s: bound element array buffer doesn't have any data", info); We should probably just remove this case, since it's handled below. ::: gfx/gl/GLContext.cpp @@ +603,5 @@ > } > } > > + if (IsExtensionSupported(ARB_draw_instanced) || > + IsExtensionSupported(EXT_draw_instanced)) { Don't forget: www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt www.khronos.org/registry/gles/extensions/NV/NV_draw_instanced.txt ::: gfx/gl/GLContext.h @@ +2964,5 @@ > return ret; > } > > + // ARB_draw_instanced > + void GLAPIENTRY fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) Don't use GLAPIENTRY for these functions.

(In reply to Jeff Gilbert [:jgilbert] from comment #6) > Comment on attachment 779200 [details] [diff] [review] > patch revision 4 > > Review of attachment 779200 [details] [diff] [review]: > ----------------------------------------------------------------- > > Needs the exts dealt with here to be fixed. > > ::: content/canvas/src/WebGLContext.h > @@ +777,5 @@ > > WebGLintptr byteOffset); > > void Viewport(WebGLint x, WebGLint y, WebGLsizei width, WebGLsizei height); > > > > + // Vertices Feature (WebGLContextVertices.cpp) > > + bool DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info); > > Why are these *_check functions public? Oups ... > > ::: content/canvas/src/WebGLContextValidate.cpp > @@ +1062,5 @@ > > (!IsExtensionSupported(OES_vertex_array_object) || > > !IsExtensionSupported(WEBGL_draw_buffers) || > > !gl->IsExtensionSupported(gl::GLContext::EXT_gpu_shader4) || > > + !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) || > > + !gl->IsExtensionSupported(gl::GLContext::ARB_draw_instanced) > > Missing EXT_draw_instanced, as well as the other GLES exts I mentioned > elsewhere. No because I fake MarkExtensionSupported(ARB_draw_instanced) if IsExtensionSupported(EXT_draw_instanced) because there are same. GLContext.cpp:623 > > ::: content/canvas/src/WebGLContextVertices.cpp > @@ +43,5 @@ > > + if (!ValidateBuffers(&maxAllowedCount, info)) { > > + return false; > > + } > > + > > + CheckedUint32 checked_firstPlusCount = CheckedUint32(first) + count; > > You're going to have to rebase this on top of the change we're making here > from Uint32 to Int32. Yes I know the patch is being reviewed by bjacob on https://bugzilla.mozilla.org/show_bug.cgi?id=896601 . And I will also have some other collisions. =) > > @@ +87,5 @@ > > + > > + SetupContextLossTimer(); > > + gl->fDrawArrays(mode, first, count); > > + > > + UndoFakeVertexAttrib0(); > > Everything this line and below in this function is repeated in the three > other draw calls. Let's pull it out into a helper function. I agree. > > @@ +163,5 @@ > > + } > > + > > + CheckedUint32 checked_byteCount; > > + > > + WebGLsizei first = 0; > > Why do we init `first` but not `checked_byteCount`? This was in the previous code I just moved from WebGLContextGL.cpp > > @@ +173,5 @@ > > + return false; > > + } > > + first = byteOffset / 2; > > + } > > + else if (type == LOCAL_GL_UNSIGNED_BYTE) { > > Let's put this condition first, so that we're increasing from 1->2->4 in the > cases we handle. This was in the previous code I just moved from WebGLContextGL.cpp > > @@ +207,5 @@ > > + return false; > > + } > > + > > + if (!mBoundVertexArray->mBoundElementArrayBuffer->ByteLength()) { > > + ErrorInvalidOperation("%s: bound element array buffer doesn't have any data", info); > > We should probably just remove this case, since it's handled below. This was in the previous code I just moved from WebGLContextGL.cpp > > ::: gfx/gl/GLContext.cpp > @@ +603,5 @@ > > } > > } > > > > + if (IsExtensionSupported(ARB_draw_instanced) || > > + IsExtensionSupported(EXT_draw_instanced)) { > > Don't forget: > www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt > www.khronos.org/registry/gles/extensions/NV/NV_draw_instanced.txt Sure. > > ::: gfx/gl/GLContext.h > @@ +2964,5 @@ > > return ret; > > } > > > > + // ARB_draw_instanced > > + void GLAPIENTRY fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) > > Don't use GLAPIENTRY for these functions. Why ? I don't understand because all other GL functions in GLContext.cpp are using GLAPIENTRY ...

(In reply to Guillaume Abadie from comment #7) > (In reply to Jeff Gilbert [:jgilbert] from comment #6) > > Comment on attachment 779200 [details] [diff] [review] > > patch revision 4 > > > > Review of attachment 779200 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Needs the exts dealt with here to be fixed. > > > > Don't forget: > > www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt > > www.khronos.org/registry/gles/extensions/NV/NV_draw_instanced.txt > Sure. Sorry, I have to disagree for NV_draw_instanced because it's only bringing VertexAttribDivisor, and it's coming with https://bugzilla.mozilla.org/show_bug.cgi?id=893180. Also, ANGLE_instanced_arrays is coming with https://bugzilla.mozilla.org/show_bug.cgi?id=893180 because they merge *_draw_isntanced and *_instanced_arrays.

(In reply to Guillaume Abadie from comment #8) > (In reply to Guillaume Abadie from comment #7) > > (In reply to Jeff Gilbert [:jgilbert] from comment #6) > > > Comment on attachment 779200 [details] [diff] [review] > > > patch revision 4 > > > > > > Review of attachment 779200 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Needs the exts dealt with here to be fixed. > > > > > > Don't forget: > > > www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt > > > www.khronos.org/registry/gles/extensions/NV/NV_draw_instanced.txt > > Sure. > Sorry, I have to disagree for NV_draw_instanced because it's only bringing > VertexAttribDivisor, and it's coming with > https://bugzilla.mozilla.org/show_bug.cgi?id=893180. > Also, ANGLE_instanced_arrays is coming with > https://bugzilla.mozilla.org/show_bug.cgi?id=893180 because they merge > *_draw_isntanced and *_instanced_arrays. NV_draw_instanced does the opposite: It's only the Draw*Instanced calls. ANGLE_instanced_arrays applies to this bug because it includes the functions in question too.

(In reply to Guillaume Abadie from comment #7) > No because I fake MarkExtensionSupported(ARB_draw_instanced) if > IsExtensionSupported(EXT_draw_instanced) because there are same. > GLContext.cpp:623 Well, we don't need to track either, so let's just drop these references. > > ::: gfx/gl/GLContext.h > > @@ +2964,5 @@ > > > return ret; > > > } > > > > > > + // ARB_draw_instanced > > > + void GLAPIENTRY fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) > > > > Don't use GLAPIENTRY for these functions. > Why ? I don't understand because all other GL functions in GLContext.cpp are > using GLAPIENTRY ... Only the ones at the bottom do, but they shouldn't. I had a bug and a patch to remove this at one point. I can't find it at the moment though. Just remove them from here. They only need to be in GLContextSymbols, if anywhere.

(In reply to Jeff Gilbert [:jgilbert] from comment #10) > (In reply to Guillaume Abadie from comment #7) > > No because I fake MarkExtensionSupported(ARB_draw_instanced) if > > IsExtensionSupported(EXT_draw_instanced) because there are same. > > GLContext.cpp:623 > Well, we don't need to track either, so let's just drop these references. I think what I did here is the best solution, because a driver might support EXT_draw_instanced instead of ARB_draw_instanced, but the specifications of thoses extensions are exactly same. Therefore with this faking, we just need to check ARB_draw_instanced, regardless if it is the really the ARB one or the EXT one. > > > > ::: gfx/gl/GLContext.h > > > @@ +2964,5 @@ > > > > return ret; > > > > } > > > > > > > > + // ARB_draw_instanced > > > > + void GLAPIENTRY fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) > > > > > > Don't use GLAPIENTRY for these functions. > > Why ? I don't understand because all other GL functions in GLContext.cpp are > > using GLAPIENTRY ... > Only the ones at the bottom do, but they shouldn't. I had a bug and a patch > to remove this at one point. I can't find it at the moment though. Just > remove them from here. They only need to be in GLContextSymbols, if anywhere. It's true.

We want the extension support API to be very explicit. If you want to make it easier to check if we support an ARB_draw_instanced equivalent, (and we probably should!) just add a helper function to GLContext like SupportsDrawInstanced.

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > We want the extension support API to be very explicit. If you want to make > it easier to check if we support an ARB_draw_instanced equivalent, (and we > probably should!) just add a helper function to GLContext like > SupportsDrawInstanced. We could generalize that to others GL extensions so ... What about SupportsXXX_draw_instanced to keep the _draw_instanced sufix like extensions do. Or maybe add a : enum GLExtensionPackages { XXX_instanced_array, ExtensionPackages_Max }; bool IsExtensionSupported(GLExtensionPackages aKnownExtensionPackage) ???

(In reply to Guillaume Abadie from comment #13) > (In reply to Jeff Gilbert [:jgilbert] from comment #12) > > We want the extension support API to be very explicit. If you want to make > > it easier to check if we support an ARB_draw_instanced equivalent, (and we > > probably should!) just add a helper function to GLContext like > > SupportsDrawInstanced. > We could generalize that to others GL extensions so ... > What about SupportsXXX_draw_instanced to keep the _draw_instanced sufix like > extensions do. > Or maybe add a : > enum GLExtensionPackages { > XXX_instanced_array, > ExtensionPackages_Max > }; > bool IsExtensionSupported(GLExtensionPackages aKnownExtensionPackage) > ??? This is an interesting idea. File a bug? We already have to do this for a number of other extension sets.

With pleasure !

Comment on attachment 779960 [details] [diff] [review] patch revision 5 Review of attachment 779960 [details] [diff] [review]: ----------------------------------------------------------------- R+, just makes sure we get those ASSERT_SYMBOL_PRESENT checks in there in the right place. (Before the GL calls) ::: content/canvas/src/WebGLContext.h @@ +789,5 @@ > +private: > + bool DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info); > + bool DrawElements_check(WebGLsizei count, WebGLenum type, WebGLintptr byteOffset, > + WebGLsizei primcount, const char* info); > + void Draw_finish(); s/finish/cleanup/ Finish has such a specific meaning in GL it's best not to confuse it if we can avoid it. ::: gfx/gl/GLContext.h @@ +3039,5 @@ > + // ARB_draw_instanced > + void fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) > + { > + BEFORE_GL_CALL; > + mSymbols.fDrawArraysInstanced(mode, first, count, primcount); Since this is an extension function, add an ASSERT_SYMBOL_PRESENT here. @@ +3046,5 @@ > + > + void fDrawElementsInstanced(GLenum mode, GLsizei count, GLenum type, const GLvoid* indices, GLsizei primcount) > + { > + BEFORE_GL_CALL; > + mSymbols.fDrawElementsInstanced(mode, count, type, indices, primcount); And here. ::: gfx/gl/GLLibraryLoader.h @@ +26,5 @@ > > typedef PRFuncPtr (GLAPIENTRY * PlatformLookupFunction) (const char *); > > enum { > + MAX_SYMBOL_NAMES = 6, Hah, neat.

(In reply to Jeff Gilbert [:jgilbert] from comment #17) > Comment on attachment 779960 [details] [diff] [review] > patch revision 5 > > Review of attachment 779960 [details] [diff] [review]: > ----------------------------------------------------------------- > > R+, just makes sure we get those ASSERT_SYMBOL_PRESENT checks in there in > the right place. (Before the GL calls) > > ::: content/canvas/src/WebGLContext.h > @@ +789,5 @@ > > +private: > > + bool DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info); > > + bool DrawElements_check(WebGLsizei count, WebGLenum type, WebGLintptr byteOffset, > > + WebGLsizei primcount, const char* info); > > + void Draw_finish(); > > s/finish/cleanup/ > Finish has such a specific meaning in GL it's best not to confuse it if we > can avoid it. Fixed. > > ::: gfx/gl/GLContext.h > @@ +3039,5 @@ > > + // ARB_draw_instanced > > + void fDrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei primcount) > > + { > > + BEFORE_GL_CALL; > > + mSymbols.fDrawArraysInstanced(mode, first, count, primcount); > > Since this is an extension function, add an ASSERT_SYMBOL_PRESENT here. Fixed. > > @@ +3046,5 @@ > > + > > + void fDrawElementsInstanced(GLenum mode, GLsizei count, GLenum type, const GLvoid* indices, GLsizei primcount) > > + { > > + BEFORE_GL_CALL; > > + mSymbols.fDrawElementsInstanced(mode, count, type, indices, primcount); > > And here. Fixed. > > ::: gfx/gl/GLLibraryLoader.h > @@ +26,5 @@ > > > > typedef PRFuncPtr (GLAPIENTRY * PlatformLookupFunction) (const char *); > > > > enum { > > + MAX_SYMBOL_NAMES = 6, > > Hah, neat. Waiting for 896601 and 896814 landed, to fix collision. Thank you Jeff !

Also waiting for lending 890277.