892546 - WebGL2 Instanced Rendering

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Instanced Rendering

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

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

Comment 2

[Guillaume Abadie]

Attachment: bug_892546_instanced_rendering.patch

patch revision 2

Comment 3

[Guillaume Abadie]

Attachment: patch revision 3

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

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

[Guillaume Abadie]

Attachment: patch revision 4

Comment 6

[Kelsey Gilbert [:jgilbert]]

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.

Comment 7

[Guillaume Abadie]

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

Comment 8

[Guillaume Abadie]

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

Comment 9

[Kelsey Gilbert [:jgilbert]]

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

Comment 10

[Kelsey Gilbert [:jgilbert]]

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

Comment 11

[Guillaume Abadie]

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

Comment 12

[Kelsey Gilbert [:jgilbert]]

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.

Comment 13

[Guillaume Abadie]

(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)
???

Comment 14

[Kelsey Gilbert [:jgilbert]]

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

Comment 15

[Guillaume Abadie]

With pleasure !

Comment 16

[Guillaume Abadie]

Attachment: patch revision 5

Comment 17

[Kelsey Gilbert [:jgilbert]]

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.

Comment 18

[Guillaume Abadie]

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

Comment 19

[Guillaume Abadie]

Also waiting for lending 890277.

Comment 20

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/639f164cc46e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/639f164cc46e

Comment 22

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext#Drawing_buffers