WebGL2 Add existing WebGL 1 extensions as WebGL 2 features

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

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

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla25
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
Add existing WebGL 1 extensions as WebGL 2 features

See https://wiki.mozilla.org/Platform/GFX/WebGL2
(Assignee)

Updated

6 years ago
Assignee: nobody → gabadie
Blocks: 889977
Depends on: 890311
(Assignee)

Updated

6 years ago
Blocks: 890926
(Assignee)

Comment 1

6 years ago
Created attachment 772220 [details] [diff] [review]
patch revision 1

Bring native support for WebGL_draw_buffers and OES_vertex_array_objects on WebGLContext2
Attachment #772220 - Flags: review?(jgilbert)
(Assignee)

Comment 2

6 years ago
Created attachment 772462 [details] [diff] [review]
patch revision 2

Add safety before enabling extensions.
Attachment #772220 - Attachment is obsolete: true
Attachment #772220 - Flags: review?(jgilbert)
Attachment #772462 - Flags: review?(jgilbert)
(Assignee)

Updated

6 years ago
Blocks: 892546
(Assignee)

Updated

6 years ago
No longer blocks: 892546
(Assignee)

Updated

6 years ago
Blocks: 892546
(Assignee)

Updated

6 years ago
Blocks: 891539
(Assignee)

Comment 3

6 years ago
Created attachment 774402 [details] [diff] [review]
patch revision 3
Attachment #772462 - Attachment is obsolete: true
Attachment #772462 - Flags: review?(jgilbert)
Attachment #774402 - Flags: review?(jgilbert)
(Assignee)

Updated

6 years ago
Blocks: 892978
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?
Attachment #774402 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
Jeff, you will find WebGL 2 patches order on https://wiki.mozilla.org/Platform/GFX/WebGL2#Landing_order_list
(Assignee)

Comment 8

6 years ago
Created attachment 775039 [details] [diff] [review]
path revision 4
Attachment #774402 - Attachment is obsolete: true
Attachment #775039 - Flags: review?(jgilbert)
(Assignee)

Comment 9

6 years ago
Created attachment 775837 [details] [diff] [review]
patch revision 5

Introduce WebGLContext::IsExtensionSupported(WebGLExtensionID ext)
Attachment #775039 - Attachment is obsolete: true
Attachment #775039 - Flags: review?(jgilbert)
Attachment #775837 - Flags: review?(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?
Attachment #775837 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 776117 [details] [diff] [review]
patch revision 6
Attachment #775837 - Attachment is obsolete: true
Attachment #776117 - Flags: review?(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?
Flags: needinfo?(gabadie)
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.
Attachment #776117 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 15

6 years ago
(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.
Flags: needinfo?(gabadie)
(Assignee)

Comment 16

6 years ago
Sorry : therefore all files that have to include WebGLVertexArray.h, have to include WebGLBuffer.h anyway.
(Assignee)

Comment 17

6 years ago
Created attachment 776762 [details] [diff] [review]
path revision 7
Attachment #776117 - Attachment is obsolete: true
Attachment #776762 - Flags: review?(jgilbert)
Attachment #776762 - Flags: review?(jgilbert) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da8602254226
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

6 years ago
No longer blocks: 892978
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.