Add DrawRangeElements (GL_ARB_instanced_array) See https://wiki.mozilla.org/Platform/GFX/WebGL2

Comment on attachment 781890 [details] [diff] [review] patch revision 1 Review of attachment 781890 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextVertices.cpp @@ +424,4 @@ > > bool WebGLContext::DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info) > { > if (first < 0 || count < 0) { I missed this previously, but we need to add primcount to these checks. Same thing for DrawElements. @@ +608,5 @@ > "size for given indices from the bound element array", info); > return false; > } > > + if (uint32_t(primcount) > mMaxFetchedInstances) { If primcount is always GLsizei, then mMaxFetchedInstances should be signed. If we did want to cast, we'd need to assert that primcount was non-negative. (We should just use the correct type though.) @@ +706,5 @@ > + MakeContextCurrent(); > + gl->fGetIntegerv(LOCAL_GL_CURRENT_PROGRAM, &currentProgram); > + > + NS_ASSERTION(GLuint(currentProgram) == mCurrentProgram->GLName(), > + "WebGL: current program doesn't agree with GL state"); Make this MOZ_ASSERT not NS_ASSERTION. @@ +709,5 @@ > + NS_ASSERTION(GLuint(currentProgram) == mCurrentProgram->GLName(), > + "WebGL: current program doesn't agree with GL state"); > + > + if (GLuint(currentProgram) != mCurrentProgram->GLName()) > + return false; Remove this DEBUG-only early-out. @@ +765,5 @@ > + > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > + maxVertices = checked_maxAllowedCount.value(); > + } > + else if (checked_maxAllowedCount.value() < maxInstances) { `} else if`! Please remember this! @@ +766,5 @@ > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > + maxVertices = checked_maxAllowedCount.value(); > + } > + else if (checked_maxAllowedCount.value() < maxInstances) { > + maxInstances = checked_maxAllowedCount.value(); `maxInstances` should be `maxAllowedCount / divisor` If divisor is high enough, we only need a vbo with one element in it. ::: content/canvas/src/WebGLVertexArray.h @@ +53,5 @@ > NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLVertexArray) > > + > + // ------------------------------------------------------------------------- > + // FUNCTION MEMBERS Member functions

(In reply to Jeff Gilbert [:jgilbert] from comment #2) > Comment on attachment 781890 [details] [diff] [review] > patch revision 1 > > Review of attachment 781890 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContextVertices.cpp > @@ +424,4 @@ > > > > bool WebGLContext::DrawArrays_check(WebGLint first, WebGLsizei count, WebGLsizei primcount, const char* info) > > { > > if (first < 0 || count < 0) { > > I missed this previously, but we need to add primcount to these checks. > Same thing for DrawElements. Oups ... > > @@ +608,5 @@ > > "size for given indices from the bound element array", info); > > return false; > > } > > > > + if (uint32_t(primcount) > mMaxFetchedInstances) { > > If primcount is always GLsizei, then mMaxFetchedInstances should be signed. > If we did want to cast, we'd need to assert that primcount was non-negative. > (We should just use the correct type though.) Well, mMaxFetchedInstances negative doesn't have sense, I would rather say that primcount be signed doesn't have any sense, but it's like that in specs ... > > @@ +706,5 @@ > > + MakeContextCurrent(); > > + gl->fGetIntegerv(LOCAL_GL_CURRENT_PROGRAM, &currentProgram); > > + > > + NS_ASSERTION(GLuint(currentProgram) == mCurrentProgram->GLName(), > > + "WebGL: current program doesn't agree with GL state"); > > Make this MOZ_ASSERT not NS_ASSERTION. I just move it from WebGLCOntextGL.cpp > > @@ +709,5 @@ > > + NS_ASSERTION(GLuint(currentProgram) == mCurrentProgram->GLName(), > > + "WebGL: current program doesn't agree with GL state"); > > + > > + if (GLuint(currentProgram) != mCurrentProgram->GLName()) > > + return false; > > Remove this DEBUG-only early-out. Ok. > > @@ +765,5 @@ > > + > > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > > + maxVertices = checked_maxAllowedCount.value(); > > + } > > + else if (checked_maxAllowedCount.value() < maxInstances) { > > `} else if`! > Please remember this! =) > > @@ +766,5 @@ > > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > > + maxVertices = checked_maxAllowedCount.value(); > > + } > > + else if (checked_maxAllowedCount.value() < maxInstances) { > > + maxInstances = checked_maxAllowedCount.value(); > > `maxInstances` should be `maxAllowedCount / divisor` > If divisor is high enough, we only need a vbo with one element in it. Oh it's true ! > > ::: content/canvas/src/WebGLVertexArray.h > @@ +53,5 @@ > > NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLVertexArray) > > > > + > > + // ------------------------------------------------------------------------- > > + // FUNCTION MEMBERS > > Member functions Fixed

Comment on attachment 782001 [details] [diff] [review] patch revision 2 Review of attachment 782001 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextVertices.cpp @@ +709,5 @@ > + > +bool > +WebGLContext::ValidateBuffersFetching(const char *info) > +{ > + if (mBuffersFetchingIsVerified) { Why did you remove the assert that used to be here? @@ +762,5 @@ > + > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > + maxVertices = checked_maxAllowedCount.value(); > + } > + else if ((checked_maxAllowedCount.value() / vd.divisor) < maxInstances) { `} else if` is the style, so please use it. If you want more whitespace, put a newline on the line before }. ::: gfx/gl/GLContext.cpp @@ +660,5 @@ > + if (!LoadSymbols(instancedArraySymbols, trygl, prefix)) { > + NS_ERROR("GL supports array instanced without supplying it function."); > + > + MarkExtensionUnsupported(ARB_instanced_arrays); > + MarkExtensionUnsupported(ANGLE_instanced_arrays); Don't forget NV_instanced_arrays! Let's actually assert `!IsExtensionSupported(XXX_instanced_arrays)` to be sure we get them all.

(In reply to Jeff Gilbert [:jgilbert] from comment #5) > Comment on attachment 782001 [details] [diff] [review] > patch revision 2 > > Review of attachment 782001 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContextVertices.cpp > @@ +709,5 @@ > > + > > +bool > > +WebGLContext::ValidateBuffersFetching(const char *info) > > +{ > > + if (mBuffersFetchingIsVerified) { > > Why did you remove the assert that used to be here? Miss understanding. > > @@ +762,5 @@ > > + > > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > > + maxVertices = checked_maxAllowedCount.value(); > > + } > > + else if ((checked_maxAllowedCount.value() / vd.divisor) < maxInstances) { > > `} else if` is the style, so please use it. > If you want more whitespace, put a newline on the line before }. Fixed. > > ::: gfx/gl/GLContext.cpp > @@ +660,5 @@ > > + if (!LoadSymbols(instancedArraySymbols, trygl, prefix)) { > > + NS_ERROR("GL supports array instanced without supplying it function."); > > + > > + MarkExtensionUnsupported(ARB_instanced_arrays); > > + MarkExtensionUnsupported(ANGLE_instanced_arrays); > > Don't forget NV_instanced_arrays! > Let's actually assert `!IsExtensionSupported(XXX_instanced_arrays)` to be > sure we get them all. Oups.

Comment on attachment 782887 [details] [diff] [review] patch revision 3 Review of attachment 782887 [details] [diff] [review]: ----------------------------------------------------------------- s/BuffersFetching/BufferFetching/ throughout the patch. ::: content/canvas/src/WebGLContext.h @@ +808,5 @@ > + void VertexAttrib2fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > + void VertexAttrib3fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > + void VertexAttrib4fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > + > + bool ValidateBuffersFetching(const char *info); s/BuffersFetching/BufferFetching/ ::: content/canvas/src/WebGLContextVertices.cpp @@ +411,5 @@ > + if ( !mBoundVertexArray->EnsureAttribIndex(index, "vertexAttribDivisor") ) { > + return; > + } > + > + WebGLVertexAttribData &vd = mBoundVertexArray->mAttribBuffers[index]; `&` to the left, against the type. @@ +770,5 @@ > + > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > + maxVertices = checked_maxAllowedCount.value(); > + } else if ((checked_maxAllowedCount.value() / vd.divisor) < maxInstances) { > + maxInstances = checked_maxAllowedCount.value() / vd.divisor; These conditions are probably actually clearer just as: if (vd.divisor == 0) maxVertices = min(maxVertices, checked_maxAllowedCount.value()); else maxInstances = min(maxInstances, checked_maxAllowedCount.value() / vd.divisor);

(In reply to Jeff Gilbert [:jgilbert] from comment #8) > Comment on attachment 782887 [details] [diff] [review] > patch revision 3 > > Review of attachment 782887 [details] [diff] [review]: > ----------------------------------------------------------------- > > s/BuffersFetching/BufferFetching/ throughout the patch. > > ::: content/canvas/src/WebGLContext.h > @@ +808,5 @@ > > + void VertexAttrib2fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > > + void VertexAttrib3fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > > + void VertexAttrib4fv_base(WebGLuint idx, uint32_t arrayLength, const WebGLfloat* ptr); > > + > > + bool ValidateBuffersFetching(const char *info); > > s/BuffersFetching/BufferFetching/ Fixed ! > > ::: content/canvas/src/WebGLContextVertices.cpp > @@ +411,5 @@ > > + if ( !mBoundVertexArray->EnsureAttribIndex(index, "vertexAttribDivisor") ) { > > + return; > > + } > > + > > + WebGLVertexAttribData &vd = mBoundVertexArray->mAttribBuffers[index]; > > `&` to the left, against the type. Fixed ! > > @@ +770,5 @@ > > + > > + if (vd.divisor == 0 && checked_maxAllowedCount.value() < maxVertices) { > > + maxVertices = checked_maxAllowedCount.value(); > > + } else if ((checked_maxAllowedCount.value() / vd.divisor) < maxInstances) { > > + maxInstances = checked_maxAllowedCount.value() / vd.divisor; > > These conditions are probably actually clearer just as: > if (vd.divisor == 0) > maxVertices = min(maxVertices, checked_maxAllowedCount.value()); > else > maxInstances = min(maxInstances, checked_maxAllowedCount.value() / > vd.divisor); Fixed ! Thanks or reviewing!