Closed Bug 893180 Opened 6 years ago Closed 6 years ago

WebGL2 gl.vertexAttribDivisor (GL_ARB_instanced_array)

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

Add DrawRangeElements (GL_ARB_instanced_array)

See https://wiki.mozilla.org/Platform/GFX/WebGL2
Assignee: nobody → gabadie
Blocks: webgl2
Depends on: 892546
Summary: WebGL2 DrawRangeElements (GL_ARB_instanced_array) → WebGL2 gl.vertexAttribDivisor (GL_ARB_instanced_array)
Depends on: 895552
No longer depends on: 895552
Attached patch patch revision 1 (obsolete) — Splinter Review
Attachment #781890 - Flags: review?(jgilbert)
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
Attachment #781890 - Flags: review?(jgilbert) → review-
(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
Attached patch patch revision 2 (obsolete) — Splinter Review
Attachment #781890 - Attachment is obsolete: true
Attachment #782001 - Flags: review?(jgilbert)
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.
Attachment #782001 - Flags: review?(jgilbert) → review-
(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.
Attached patch patch revision 3Splinter Review
Attachment #782001 - Attachment is obsolete: true
Attachment #782887 - Flags: review?(jgilbert)
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);
Attachment #782887 - Flags: review?(jgilbert) → review+
(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!
Blocks: 900767
Depends on: 902063
Target Milestone: --- → mozilla26
https://hg.mozilla.org/mozilla-central/rev/9c91adcdc328
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.