890379 - WebGL2 Add existing WebGL 1 extensions as WebGL 2 features

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Add existing WebGL 1 extensions as WebGL 2 features

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

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

Bring native support for WebGL_draw_buffers and OES_vertex_array_objects on WebGLContext2

Comment 2

[Guillaume Abadie]

Attachment: patch revision 2

Add safety before enabling extensions.

Comment 3

[Guillaume Abadie]

Attachment: patch revision 3

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 774402 [details] [diff] [review]
patch revision 3

Review of attachment 774402 [details] [diff] [review]:
-----------------------------------------------------------------

IsExtensionSupported needs to be made safe for calls with a null JS context.

::: content/canvas/src/WebGLContext.cpp
@@ +1123,5 @@
> +WebGLContext::EnableExtension(WebGLExtensionID ext)
> +{
> +    mExtensions.EnsureLengthAtLeast(ext + 1);
> +
> +    MOZ_ASSERT(mExtensions[ext] == nullptr);

It might be more clear to just assert `!IsExtensionEnabled(ext)`, maybe just additionally preceeding this assert.

@@ +1125,5 @@
> +    mExtensions.EnsureLengthAtLeast(ext + 1);
> +
> +    MOZ_ASSERT(mExtensions[ext] == nullptr);
> +
> +    WebGLExtensionBase *obj = nullptr;

Prefer 'T* foo' to 'T *foo'.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1049,5 @@
>          return false;
>      }
>  
> +    if (IsWebGL2() &&
> +        (!IsExtensionSupported(nullptr, OES_vertex_array_object) ||

I don't think IsExtensionSupported is safe to call with a null js context. It looks like it can be made safe, though.

::: content/canvas/src/WebGLVertexArray.h
@@ +6,5 @@
>  #ifndef WEBGLVERTEXARRAY_H_
>  #define WEBGLVERTEXARRAY_H_
>  
>  #include "WebGLObjectModel.h"
> +#include "WebGLBuffer.h"

Why has this been added?

Comment 5

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Comment on attachment 774402 [details] [diff] [review]
> patch revision 3
> 
> Review of attachment 774402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> IsExtensionSupported needs to be made safe for calls with a null JS context.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +1123,5 @@
> > +WebGLContext::EnableExtension(WebGLExtensionID ext)
> > +{
> > +    mExtensions.EnsureLengthAtLeast(ext + 1);
> > +
> > +    MOZ_ASSERT(mExtensions[ext] == nullptr);
> 
> It might be more clear to just assert `!IsExtensionEnabled(ext)`, maybe just
> additionally preceeding this assert.
> 
> @@ +1125,5 @@
> > +    mExtensions.EnsureLengthAtLeast(ext + 1);
> > +
> > +    MOZ_ASSERT(mExtensions[ext] == nullptr);
> > +
> > +    WebGLExtensionBase *obj = nullptr;
> 
> Prefer 'T* foo' to 'T *foo'.
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +1049,5 @@
> >          return false;
> >      }
> >  
> > +    if (IsWebGL2() &&
> > +        (!IsExtensionSupported(nullptr, OES_vertex_array_object) ||
> 
> I don't think IsExtensionSupported is safe to call with a null js context.
> It looks like it can be made safe, though.
> 
> ::: content/canvas/src/WebGLVertexArray.h
> @@ +6,5 @@
> >  #ifndef WEBGLVERTEXARRAY_H_
> >  #define WEBGLVERTEXARRAY_H_
> >  
> >  #include "WebGLObjectModel.h"
> > +#include "WebGLBuffer.h"
> 
> Why has this been added?

About IsExtensionSupported(nullptr, OES_vertex_array_object) : it is safe because the first parameter is actually not used at all.

Comment 6

[Guillaume Abadie]

and #include "WebGLBuffer.h" has been added because of the WebGLRefPtr<WebGLBuffer> mBoundElementArrayBuffer; at the genreation of bindings for WebGLRenderingContext2.webidl. It works in WebGLRenderingContext,webidl because its have all objects bindings, including WebGLBuffer which off course ask for including WebGLBuffer.h before WebGLVertexArray does for WebGLVertexArray.h.

Comment 7

[Guillaume Abadie]

Jeff, you will find WebGL 2 patches order on https://wiki.mozilla.org/Platform/GFX/WebGL2#Landing_order_list

Comment 8

[Guillaume Abadie]

Attachment: path revision 4

Comment 9

[Guillaume Abadie]

Attachment: patch revision 5

Introduce WebGLContext::IsExtensionSupported(WebGLExtensionID ext)

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 775837 [details] [diff] [review]
patch revision 5

Review of attachment 775837 [details] [diff] [review]:
-----------------------------------------------------------------

Fix the nits, and we're good.

::: content/canvas/src/WebGLContext.cpp
@@ +1127,5 @@
>      }
>  
>      // step 3: if the extension hadn't been previously been created, create it now, thus enabling it
> +    if (!IsExtensionEnabled(ext))
> +    {

'{' on same line as single-line conditionals.

::: content/canvas/src/WebGLContextFramebufferOperations.cpp
@@ +171,5 @@
> +
> +        MakeContextCurrent();
> +
> +        if (buffers[0] == LOCAL_GL_NONE) {
> +            const GLenum drawBufffersCommand = LOCAL_GL_NONE;

s/fff/ff/ :)

@@ +176,5 @@
> +            gl->fDrawBuffers(1, &drawBufffersCommand);
> +            return;
> +        }
> +        else if (buffers[0] == LOCAL_GL_BACK) {
> +            const GLenum drawBufffersCommand = LOCAL_GL_COLOR_ATTACHMENT0;

I believe this is not actually true for Win+ANGLE+D3D>10+no-aa, where we actually are rendering to an EGLSurface.
It will also not be true when we start using EGLStream.
It looks like this is just code movement, but let's address this in another bug. (now bug 894114)

Also s/fff/ff/.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1071,5 @@
>      mDefaultVertexArray->mAttribBuffers.SetLength(mGLMaxVertexAttribs);
>      mBoundVertexArray = mDefaultVertexArray;
>  
> +    if (IsWebGL2()) {
> +        EnableExtension(OES_vertex_array_object);

Assert that these return non-null.

::: content/canvas/src/WebGLVertexArray.h
@@ +6,5 @@
>  #ifndef WEBGLVERTEXARRAY_H_
>  #define WEBGLVERTEXARRAY_H_
>  
>  #include "WebGLObjectModel.h"
> +#include "WebGLBuffer.h"

Is this include necessary in this header, or can it be moved to the CPP file?

Comment 11

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 775837 [details] [diff] [review]
> patch revision 5
> 
> Review of attachment 775837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix the nits, and we're good.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +1127,5 @@
> >      }
> >  
> >      // step 3: if the extension hadn't been previously been created, create it now, thus enabling it
> > +    if (!IsExtensionEnabled(ext))
> > +    {
> 
> '{' on same line as single-line conditionals.
> 
> ::: content/canvas/src/WebGLContextFramebufferOperations.cpp
> @@ +171,5 @@
> > +
> > +        MakeContextCurrent();
> > +
> > +        if (buffers[0] == LOCAL_GL_NONE) {
> > +            const GLenum drawBufffersCommand = LOCAL_GL_NONE;
> 
> s/fff/ff/ :)
> 
> @@ +176,5 @@
> > +            gl->fDrawBuffers(1, &drawBufffersCommand);
> > +            return;
> > +        }
> > +        else if (buffers[0] == LOCAL_GL_BACK) {
> > +            const GLenum drawBufffersCommand = LOCAL_GL_COLOR_ATTACHMENT0;
> 
> I believe this is not actually true for Win+ANGLE+D3D>10+no-aa, where we
> actually are rendering to an EGLSurface.
> It will also not be true when we start using EGLStream.
> It looks like this is just code movement, but let's address this in another
> bug. (now bug 894114)
> 
> Also s/fff/ff/.
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +1071,5 @@
> >      mDefaultVertexArray->mAttribBuffers.SetLength(mGLMaxVertexAttribs);
> >      mBoundVertexArray = mDefaultVertexArray;
> >  
> > +    if (IsWebGL2()) {
> > +        EnableExtension(OES_vertex_array_object);
> 
> Assert that these return non-null.
> 
> ::: content/canvas/src/WebGLVertexArray.h
> @@ +6,5 @@
> >  #ifndef WEBGLVERTEXARRAY_H_
> >  #define WEBGLVERTEXARRAY_H_
> >  
> >  #include "WebGLObjectModel.h"
> > +#include "WebGLBuffer.h"
> 
> Is this include necessary in this header, or can it be moved to the CPP file?
and #include "WebGLBuffer.h" has been added because of the WebGLRefPtr<WebGLBuffer> mBoundElementArrayBuffer; at the generation of bindings for WebGLRenderingContext2.webidl. It works in WebGLRenderingContext.webidl because its have all objects bindings, including WebGLBuffer which off course ask for including WebGLBuffer.h before WebGLVertexArray does for WebGLVertexArray.h.

Comment 12

[Guillaume Abadie]

Attachment: patch revision 6

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #11)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > Is this include necessary in this header, or can it be moved to the CPP file?
> and #include "WebGLBuffer.h" has been added because of the
> WebGLRefPtr<WebGLBuffer> mBoundElementArrayBuffer; at the generation of
> bindings for WebGLRenderingContext2.webidl. It works in
> WebGLRenderingContext.webidl because its have all objects bindings,
> including WebGLBuffer which off course ask for including WebGLBuffer.h
> before WebGLVertexArray does for WebGLVertexArray.h.

Interesting. Generally, though, we can move this to the CPP file if we move the ctor and dtor definitions to the CPP file. Can you test this?

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776117 [details] [diff] [review]
patch revision 6

Review of attachment 776117 [details] [diff] [review]:
-----------------------------------------------------------------

Please try to get around adding includes to includes.

::: content/canvas/src/WebGLVertexArray.h
@@ +6,5 @@
>  #ifndef WEBGLVERTEXARRAY_H_
>  #define WEBGLVERTEXARRAY_H_
>  
>  #include "WebGLObjectModel.h"
> +#include "WebGLBuffer.h"

Try moving the ctor and dtor to the CPP to allow removing this include.

Comment 15

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 776117 [details] [diff] [review]
> patch revision 6
> 
> Review of attachment 776117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please try to get around adding includes to includes.
> 
> ::: content/canvas/src/WebGLVertexArray.h
> @@ +6,5 @@
> >  #ifndef WEBGLVERTEXARRAY_H_
> >  #define WEBGLVERTEXARRAY_H_
> >  
> >  #include "WebGLObjectModel.h"
> > +#include "WebGLBuffer.h"
> 
> Try moving the ctor and dtor to the CPP to allow removing this include.
I tried several things, and it still not working. The solution would be to say something like : I want WebGLRenderingContext2.h add WebGLBuffer.h, But in my mind, that make sence to let this include here. Because the thing is you can't include WebGLVertexArray.h without include WebGLBuffer.h before, therefore all file that have to include WebGLVertexArray.h, have to include WebGLVertexArray.h anyway. So trying removing include as much as possible is just painful and we don't save any compilation time. Maybe the solution would be pre-compiled headers.

Waiting for landing patch https://bugzilla.mozilla.org/show_bug.cgi?id=890311 before this one.

Comment 16

[Guillaume Abadie]

Sorry : therefore all files that have to include WebGLVertexArray.h, have to include WebGLBuffer.h anyway.

Comment 17

[Guillaume Abadie]

Attachment: path revision 7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da8602254226

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da8602254226