Closed Bug 890379 Opened 7 years ago Closed 7 years ago

WebGL2 Add existing WebGL 1 extensions as WebGL 2 features

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

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

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

Add existing WebGL 1 extensions as WebGL 2 features

See https://wiki.mozilla.org/Platform/GFX/WebGL2
Assignee: nobody → gabadie
Blocks: webgl2
Depends on: 890311
Blocks: 890926
Attached patch patch revision 1 (obsolete) — Splinter Review
Bring native support for WebGL_draw_buffers and OES_vertex_array_objects on WebGLContext2
Attachment #772220 - Flags: review?(jgilbert)
Attached patch patch revision 2 (obsolete) — Splinter Review
Add safety before enabling extensions.
Attachment #772220 - Attachment is obsolete: true
Attachment #772220 - Flags: review?(jgilbert)
Attachment #772462 - Flags: review?(jgilbert)
Blocks: 892546
No longer blocks: 892546
Blocks: 892546
Blocks: 891539
Attached patch patch revision 3 (obsolete) — Splinter Review
Attachment #772462 - Attachment is obsolete: true
Attachment #772462 - Flags: review?(jgilbert)
Attachment #774402 - Flags: review?(jgilbert)
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-
(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.
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.
Jeff, you will find WebGL 2 patches order on https://wiki.mozilla.org/Platform/GFX/WebGL2#Landing_order_list
Attached patch path revision 4 (obsolete) — Splinter Review
Attachment #774402 - Attachment is obsolete: true
Attachment #775039 - Flags: review?(jgilbert)
Attached patch patch revision 5 (obsolete) — Splinter Review
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+
(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.
Attached patch patch revision 6 (obsolete) — Splinter Review
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+
(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)
Sorry : therefore all files that have to include WebGLVertexArray.h, have to include WebGLBuffer.h anyway.
Attached patch path revision 7Splinter Review
Attachment #776117 - Attachment is obsolete: true
Attachment #776762 - Flags: review?(jgilbert)
Attachment #776762 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da8602254226
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
No longer blocks: 892978
You need to log in before you can comment on or make changes to this bug.