Implement WebGL OES_vertex_array_object extension

RESOLVED FIXED in mozilla25

Status

()

Core
Canvas: WebGL
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgilbert, Assigned: James King)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jgilbert][lang=c++][games:p2] webgl-extension, URL)

Attachments

(4 attachments, 15 obsolete attachments)

9.75 KB, patch
jgilbert
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
19.25 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
54.70 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
907 bytes, patch
jgilbert
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
The extension draft is here:
http://www.khronos.org/registry/webgl/extensions/OES_vertex_array_object/

An overview of how WebGL extensions are added is here: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions
No longer blocks: 728319
Whiteboard: [mentor=jgilbert][lang=c++] → [mentor=jgilbert][lang=c++] webgl-extension

Comment 1

5 years ago
Started looking into bug and working on it.
Mark, still looking into this?  Let us know if you have any questions on where/how to get started.
Whiteboard: [mentor=jgilbert][lang=c++] webgl-extension → [mentor=jgilbert][lang=c++][games:p2] webgl-extension

Comment 3

5 years ago
Hi Vladimir, am still looking into this. Changed my developer laptop to a mac, am setting up the mozilla build environment on it. I would greatly appreciate any information and advice on the requirements for this task and/or any technical details that would help with implementing the required WebGL component. Thanks in advance :)

Comment 4

5 years ago
Hello all!

I'm really interested in the implementation of WebGL and I'd like to start on it by looking into this.

Mark: If you're still working on it I'll leave it alone but if not I'll take it on.
Hi Peter!
We just completely re-did the way that WebGL extensions are implemented. Consequently, documentation is currently outdated. I'll update this bug as soon as it's updated.
Done!
https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions is now up to date again, and describes the new way to expose WebGL extensions.

Comment 7

5 years ago
Hi Peter,
I have almost set up my mozilla dev environment in my new mac, and will start working on this soon. Will forward you the task if i am getting delayed or take more time :)
Note, if there is demand for more WebGL mentored bugs, always feel free to let me and jgilbert know, we're happy to file more. There is work that needs to be done.
Note: bug 738867 is another webgl extension that would be a good first bug/first extension.
Vlad: Jon Buckley and/or :dwarfcrank is already looking into this one.

Comment 11

4 years ago
I found a demo for this extension:
http://media.tojicode.com/webgl-samples/OES_vertex_array_object.html

More informations:
http://blog.tojicode.com/2012/10/oesvertexarrayobject-extension.html
This blogpost about running the Nebula3 game engine on the web mentions this extension,

http://flohofwoe.blogspot.de/2013/01/a-drakensang-online-map-viewer-in.html

He says that it would greatly reduce the number of GL calls in the demo he links to there, sounds like it should remove some of the frame stutter.
(Assignee)

Comment 13

4 years ago
Created attachment 735755 [details] [diff] [review]
WIP implementation of OES_vertex_array_object
Attachment #735755 - Flags: review?(jgilbert)
(Assignee)

Comment 14

4 years ago
Created attachment 735756 [details] [diff] [review]
WIP implementation of OES_vertex_array_object
(Assignee)

Updated

4 years ago
Attachment #735756 - Flags: review?(jgilbert)
Attachment #735756 - Attachment is patch: true
Hey James,

Thanks for the patches!  It's a good start, but unfortunately not complete :)

- It needs an implementation of the WebGLExtensionVertexArrayObjectOES class

- It's missing a bunch of integration points for VAOs in the WebGL code -- basically, all the security checks that track what the currently bound vertex attrib arrays are.  This might just be a matter of making sure that the contents of mAttribBuffers is kept up to date with the VAO (maybe just making a copy of it when the VAO is created would be enough, and updating it with any binding change?)

- (Ideally, but optinally) it could use a manual implementation of VAOs when the underlying GL extension isn't available.  This is something that we could emulate and still get a performance win, because we'd avoid the need to make a bunch of calls from JS for attrib setup.  This should probably be done as a followup patch to the initial one that implements/enables the VAO extension.

Also, it looks like you attached two separate patches that should be combined into one... not sure how you're generating your diffs, but worst case you can just cat them together into one file.

Thanks for taking this on!  You can find a bunch of us on irc.mozilla.org #gfx as well, if you have any questions that you'd like more real-time answers to.
(Reporter)

Comment 16

4 years ago
Comment on attachment 735756 [details] [diff] [review]
WIP implementation of OES_vertex_array_object

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

This is going in the right direction, but you'll also need entries in GLContextSymbols.h, as well as the actual functions in GLContext.h. See what was done in the bug for adding ARB_sync support: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=739421&attachment=612737
Attachment #735756 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 17

4 years ago
Comment on attachment 735755 [details] [diff] [review]
WIP implementation of OES_vertex_array_object

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

Alright, we're on the right track, but I think we're missing an important file from this patch.

::: content/canvas/src/Makefile.in
@@ +53,5 @@
>  	WebGLExtensionLoseContext.cpp \
>  	WebGLExtensionStandardDerivatives.cpp \
>  	WebGLExtensionTextureFilterAnisotropic.cpp \
>  	WebGLExtensionTextureFloat.cpp \
> +	WebGLExtensionVertexArrayObjectOES.cpp \

This file wasn't included in this patch. Did you forget to `hg add` it? Also, drop the 'OES' from the end here, as well.

::: content/canvas/src/WebGLContext.cpp
@@ +950,5 @@
>          case OES_texture_float:
>              return gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float
>                                                            : GLContext::ARB_texture_float);
> +        case OES_vertex_array_object:
> +            return gl->IsExtensionSupported(GLContext::OES_vertex_array_object);

You should also check for the desktop GL extension:
http://www.opengl.org/registry/specs/ARB/vertex_array_object.txt

::: content/canvas/src/WebGLExtensions.h
@@ +128,5 @@
>  
>      DECL_WEBGL_EXTENSION_GOOP
>  };
>  
> +class WebGLExtensionVertexArrayObjectOES

Just 'WebGLExtensionVertexArrayObject', without the 'OES' suffix.

@@ +138,5 @@
> +
> +    WebGLExtensionVertexArrayObjectOES createVertexArrayOES();
> +    void deleteVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject);
> +    GLboolean isVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject);
> +    void bindVertexArrayOES(WebGLExtensionVertexArrayObjectOES arrayObject);

These functions all take (or return) `WebGLVertexArrayObjectOES`, not `WebGLExtensionVertexArrayObjectOES`.
Attachment #735755 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 18

4 years ago
To be clear, I really want two different patches:
One that just adds ARB/OES_vertex_array_object support to GLContext, and a second which does the actual webgl implementation.
(Reporter)

Comment 19

4 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> To be clear, I really want two different patches:
> One that just adds ARB/OES_vertex_array_object support to GLContext, and a
> second which does the actual webgl implementation.

To be double-clear, the way you're splitting it right now is exactly what I want.
(Assignee)

Comment 20

4 years ago
Created attachment 736288 [details] [diff] [review]
Added the implementation of WebGLExtensionVertexArrayObject

Forgot to add the (rather light) implementation I have so far.
Attachment #735755 - Attachment is obsolete: true
Attachment #735756 - Attachment is obsolete: true
Attachment #736288 - Flags: review?(jgilbert)
(Assignee)

Comment 21

4 years ago
Jeff, I will keep the patches separate going forward.
(Assignee)

Comment 22

4 years ago
Created attachment 736309 [details] [diff] [review]
Updated based on Jeff's review
Attachment #736288 - Attachment is obsolete: true
Attachment #736288 - Flags: review?(jgilbert)
Attachment #736309 - Flags: review?(jgilbert)
(Assignee)

Comment 23

4 years ago
Created attachment 736310 [details] [diff] [review]
Updated based on Jeff's review
Attachment #736310 - Flags: review?(jgilbert)
(Reporter)

Comment 24

4 years ago
Comment on attachment 736309 [details] [diff] [review]
Updated based on Jeff's review

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

::: gfx/gl/GLContext.cpp
@@ +61,5 @@
>      "GL_ARB_texture_float",
>      "GL_EXT_unpack_subimage",
>      "GL_OES_standard_derivatives",
> +    "GL_OES_vertex_array_object",
> +    "GL_ARB_vertex_array_object",

These need to go at the end of the list. This needs to match the enum in GLContext.h, as they are parallel structures.

@@ +534,5 @@
>                  mSymbols.fEGLImageTargetRenderbufferStorage = nullptr;
>              }
>          }
> +
> +        if (IsExtensionSupported(OES_vertex_array_object)) {

Needs to be checking for the desktop version, as well.

@@ +539,5 @@
> +            SymLoadStruct vaoSymbols[] = {
> +                { (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArray", nullptr } },
> +                { (PRFuncPtr*) &mSymbols.fGenVertexArrays, { "GenVertexArrays", nullptr } },
> +                { (PRFuncPtr*) &mSymbols.fBindVertexArray, { "BindVertexArray", nullptr } },
> +                { (PRFuncPtr*) &mSymbols.fDeleteVertexArrays, { "DeleteVertexAraays", nullptr } },

Should be "DeleteVertexArrays".
Also all of these functions we're querying for should be suffixed by "OES". (Check the GL/GLES spec)

You also need to include the changes in GLContextSymbols.h. Again, see the ARB_sync patch. This patch here should be basically the same thing, except for a different extension.
Attachment #736309 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 25

4 years ago
Comment on attachment 736310 [details] [diff] [review]
Updated based on Jeff's review

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

::: content/canvas/src/Makefile.in
@@ +53,5 @@
>  	WebGLExtensionLoseContext.cpp \
>  	WebGLExtensionStandardDerivatives.cpp \
>  	WebGLExtensionTextureFilterAnisotropic.cpp \
>  	WebGLExtensionTextureFloat.cpp \
> +	WebGLExtensionVertexArrayObjectOES.cpp \

This filename shouldn't have OES at the end.
Also you need to include this file in this patch. If you're using HG, use `hg add content/canvas/src/WebGLExtensionVertexArrayObject.cpp`.
Attachment #736310 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 26

4 years ago
Created attachment 738080 [details] [diff] [review]
WIP gfx/gl update for OES/ARB vertex array object
Attachment #736309 - Attachment is obsolete: true
Attachment #738080 - Flags: review?(jgilbert)
(Assignee)

Comment 27

4 years ago
Created attachment 738081 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject
Attachment #736310 - Attachment is obsolete: true
Attachment #738081 - Flags: review?(jgilbert)
(Reporter)

Comment 28

4 years ago
Comment on attachment 738080 [details] [diff] [review]
WIP gfx/gl update for OES/ARB vertex array object

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

These file are almost good, but I still don't see the changes we need done to GLDefs.h and GLContextSymbols.h. Without changes to GLContextSymbols.h, this won't even compile. Please include the changes to these two extra files in the next review request.

::: gfx/gl/GLContext.cpp
@@ +536,5 @@
>          }
> +
> +        if (IsExtensionSupported(OES_vertex_array_object)) {
> +            SymLoadStruct vaoSymbols[] = {
> +                { (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArrayOES", nullptr } },

We don't need to split these two extensions. They're the same, they just have different symbol names that we want to coalesce into one standard set.

This line should be:
{ (PRFuncPtr*) &mSymbols.fIsVertexArray, { "IsVertexArray", "IsVertexArrayOES", nullptr } },

This way it first tries the un-suffixed version, then the suffixed version, then fails if neither is found. This will cover both the OES and ARB extensions.

@@ +546,5 @@
> +
> +            if (!LoadSymbols(&vaoSymbols[0], trygl, prefix)) {
> +                NS_ERROR("GL supports OES_vertex_array_object without supplying its functions.");
> +
> +                MarkExtensionUnsupported(OES_vertex_array_object);

Just unconditionally mark both as unsupported, as we combine these two branches.

@@ +552,5 @@
> +                mSymbols.fGenVertexArrays = nullptr;
> +                mSymbols.fBindVertexArray = nullptr;
> +                mSymbols.fDeleteVertexArrays = nullptr;
> +            }
> +        } else if (IsExtensionSupported(ARB_vertex_array_object)) {

Don't split the handling of the ARB and OES extension. See other comments.

::: gfx/gl/GLContext.h
@@ +2866,5 @@
>          mSymbols.fEGLImageTargetRenderbufferStorage(target, image);
>          AFTER_GL_CALL;
>      }
>  
> +    void GLAPIENTRY fBindVertexArray(GLuint array)

Take out 'GLAPIENTRY'. It should just be ancient cruft which needs to be removed.
Attachment #738080 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 29

4 years ago
Comment on attachment 738081 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject

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

Great, we're getting there.

::: content/canvas/src/WebGLExtensionVertexArrayObject.cpp
@@ +17,5 @@
> +WebGLExtensionVertexArrayObjectOES::~WebGLExtensionVertexArrayObjectOES()
> +{
> +}
> +
> +WebGLVertexArrayObject WebGLExtensionVertexArrayObjectOES::CreateVertexArrayOES()

You will need to create a new WebGLVertexArrayObject here. (along with creating its class definition in its own pair of header/source files, similar to WebGLRenderbuffer, and other first-class webgl objects)

@@ +26,5 @@
> +
> +    return array;
> +}
> +
> +void WebGLExtensionVertexArrayObjectOES::DeleteVertexArrayOES(WebGLVertexArrayObject array)

For the rest of these, you'll need to accept a pointer to the `array` object, and do the `mContext->gl->` calls based on the data it holds. Basically, the `WebGLVertexArrayObject` object will be a wrapper around the `GLuint` handle used internally by normal OpenGL.

::: content/canvas/src/WebGLExtensions.h
@@ +135,5 @@
> +public:
> +    WebGLExtensionVertexArrayObject(WebGLContext*);
> +    virtual ~WebGLExtensionVertexArrayObject();
> +
> +    WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint

This should return an `already_AddRefed<WebGLVertexArrayObject>`.

@@ +137,5 @@
> +    virtual ~WebGLExtensionVertexArrayObject();
> +
> +    WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint
> +    void DeleteVertexArrayOES(WebGLVertexArrayObject array);
> +    GLboolean IsVertexArrayOES(WebGLVertexArrayObject array);

This should just return `bool`, not `GLboolean`.

@@ +138,5 @@
> +
> +    WebGLVertexArrayObject CreateVertexArrayOES(); // WebGLVertexArrayObject should be a typedef for GLuint
> +    void DeleteVertexArrayOES(WebGLVertexArrayObject array);
> +    GLboolean IsVertexArrayOES(WebGLVertexArrayObject array);
> +    void BindVertexArrayOES(WebGLVertexArrayObject array);

These three need the type of `array` to be `WebGLVertexArrayObject*`.

::: content/canvas/src/WebGLObjectModel.h
@@ +19,5 @@
>  typedef uint32_t WebGLuint;
>  typedef float WebGLfloat;
>  typedef float WebGLclampf;
>  typedef bool WebGLboolean;
> +typedef unsigned int WebGLVertexArrayObject;

WebGLVertexArrayObject needs to be a full object type, like WebGLTexture.
Attachment #738081 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 30

4 years ago
Also, check WebGLContext::CreateRenderbuffer and WebGLContext::DeleteRenderbuffer (etc) for a taste of how the implementation of CreateVertexArray (etc) should work.
Just a drive-by question. Why this typedef?

+typedef unsigned int WebGLVertexArrayObject;

Is this a OpenGL unsigned integer name like for other OpenGL objects? If yes, then 1) why does this use its own typedef whereas for textures/buffers/etc we just use GLuint like the OpenGL API itself does; and 2) if this is a generic OpenGL thing then why does this have WebGL in its name?
(Assignee)

Comment 32

4 years ago
Thanks :bjacob;

It should be a GLuint and I'm in the process of following through with Jeff's comments.
(Assignee)

Comment 33

4 years ago
Created attachment 752332 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject
Attachment #752332 - Attachment is patch: true
(Assignee)

Comment 34

4 years ago
Comment on attachment 752332 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject

Just another WIP update to show progress on the patch.

I think it's just missing the JS wrappers but would like someone to take a peek at it and make sure I've got the implementation going in the right direction.
Attachment #752332 - Flags: review?(jgilbert)
(Assignee)

Updated

4 years ago
Attachment #738081 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Created attachment 752337 [details] [diff] [review]
WIP update to gl to implement OES_vertex_array_object
Attachment #738080 - Attachment is obsolete: true
Attachment #752337 - Flags: review?(jgilbert)
(Reporter)

Updated

4 years ago
Attachment #752337 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 36

4 years ago
Comment on attachment 752332 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject

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

This is close.

::: content/canvas/src/WebGLContextGL.cpp
@@ +216,5 @@
> +    
> +    if (!ValidateObjectAllowDeletedOrNull("bindVertexArrayObject", array))
> +        return;
> +
> +    if (array and array->IsDeleted())

`and`? Too much python? :P

@@ +220,5 @@
> +    if (array and array->IsDeleted())
> +        return;
> +
> +    if (array)
> +        array->SetHasEverBeenBound(true);

I would prefer to do this after we bind, not before.

@@ +3032,5 @@
> +    if (!IsContextStable())
> +        return false;
> +
> +    return ValidateObjectAllowDeleted("isVertexArrayObject", array) &&
> +        !array->IsDeleted() &&

Indent continued lines to match the beginning of `ValidateObje...`.

@@ +4263,5 @@
> +WebGLContext::CreateVertexArray()
> +{
> +    if (!IsContextStable())
> +        return nullptr;
> +    nsRefPtr<WebGLVertexArrayObject> globj = new WebGLVertexArrayObject(this);

Newline after the return.

::: content/canvas/src/WebGLExtensionVertexArrayObject.cpp
@@ +26,5 @@
> +
> +void WebGLExtensionVertexArrayObject::DeleteVertexArray(WebGLVertexArrayObject* array)
> +{
> +    if (!array)
> +      return;

Handle this in WebGLContext.

@@ +35,5 @@
> +
> +bool WebGLExtensionVertexArrayObject::IsVertexArray(WebGLVertexArrayObject* array)
> +{
> +    if (!array)
> +      return false;

Handle this in WebGLContext.

@@ +49,5 @@
> +
> +    if (!array->HasEverBeenBound()) {
> +        //  mContext->MakeContextCurrent();
> +        mContext->BindVertexArray(array);
> +        array->SetHasEverBeenBound(true);

These functions should be very minimal pass-throughs to the functions in WebGLContext.

::: content/canvas/src/WebGLVertexArrayObject.h
@@ +44,5 @@
> +    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLVertexArrayObject)
> +
> +protected:
> +    WebGLuint mGLName;
> +    bool mHasEverBeenBound;

These need to be initialized in the constructor's initializer list.
Attachment #752332 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 37

4 years ago
Created attachment 752791 [details] [diff] [review]
WIP update following jgilbert's review

The linker still cannot find the missing JS Wrap symbols which will need fixing, but I've gone through and cleaned things up.
Attachment #752332 - Attachment is obsolete: true
Attachment #752791 - Flags: review?(jgilbert)
(Reporter)

Comment 38

4 years ago
Comment on attachment 752791 [details] [diff] [review]
WIP update following jgilbert's review

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

Almost!

::: content/canvas/src/WebGLContextGL.cpp
@@ +210,5 @@
>  
>  void
> +WebGLContext::BindVertexArray(WebGLVertexArrayObject *array)
> +{
> +    if (!array)

We need to allow null here, since that's how we unbind VAOs without binding a new one.

@@ +228,5 @@
> +    WebGLuint vertexarrayobjectname = array ? array->GLName() : 0;
> +    gl->fBindVertexArray(vertexarrayobjectname);
> +
> +    mBoundVertexArrayObject = array;
> +    array->SetHasEverBeenBound(true);

Don't call against `array` unless it's non-null.

@@ +1102,5 @@
> +
> +    if (!IsContextStable())
> +        return;
> +
> +    if (!ValidateObjectAllowDeletedOrNull("deleteVertexArrayObject", array))

`array` can't be null here.

@@ +1105,5 @@
> +
> +    if (!ValidateObjectAllowDeletedOrNull("deleteVertexArrayObject", array))
> +        return;
> +
> +    if (!array || array->IsDeleted())

`array` can't be null here.
Attachment #752791 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 39

4 years ago
Created attachment 753779 [details] [diff] [review]
WIP update to canvas following jgilbert's review
Attachment #752791 - Attachment is obsolete: true
Attachment #753779 - Flags: review?(jgilbert)
review ping!
Flags: needinfo?(jgilbert)
(Reporter)

Comment 41

4 years ago
Comment on attachment 753779 [details] [diff] [review]
WIP update to canvas following jgilbert's review

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

Ok, bad news. There's actually a decent amount of work still to do.

This patch is almost correct for just adding raw support to WebGL, but we still need to tie it to all the state tracking we do. I detailed what needs to happen in the comment attached to `BindVertexArray`.

::: content/canvas/src/WebGLContextGL.cpp
@@ +208,5 @@
>      mBoundRenderbuffer = wrb;
>  }
>  
>  void
> +WebGLContext::BindVertexArray(WebGLVertexArrayObject *array)

This is actually going to have to be more involved than I thought.

When we switch VAOs, we would need to update `mBoundArrayBuffer` and `mBoundElementArrayBuffer`, based on the new vao. (think of vao 0 as the 'default vao') We would also need to update all our info about vertex attrib arrays.

Instead, what we should do, is put all this info on the `WebGLVertexArrayObject` object, and indirect into the currently bound one when we need such info. The list of state that VAOs store is enumerated in the extension.

@@ +217,5 @@
> +    if (!ValidateObjectAllowDeletedOrNull("bindVertexArrayObject", array))
> +        return;
> +
> +    if (array && array->IsDeleted())
> +        return;

Deleted should emit an INVALID_OPERATION.

@@ +226,5 @@
> +        WebGLuint vertexarrayobjectname = array ? array->GLName() : 0;
> +        gl->fBindVertexArray(vertexarrayobjectname);
> +
> +        mBoundVertexArrayObject = array;
> +        array->SetHasEverBeenBound(true);

This is the only line that should be inside this `if`. All other lines should be run unconditionally.

@@ +3028,5 @@
> +{
> +    if (!array)
> +        return false;
> +
> +    if (!IsContextStable())

`IsContextStable` should always come first.
Attachment #753779 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 42

4 years ago
I had a feeling it couldn't quite be that simple. I'll take a peek at this ASAP.
(Reporter)

Updated

4 years ago
Flags: needinfo?(jgilbert)
(Reporter)

Comment 43

4 years ago
Comment on attachment 752337 [details] [diff] [review]
WIP update to gl to implement OES_vertex_array_object

Let's at least take this patch.
Attachment #752337 - Flags: checkin?(jgilbert)
Assignee: nobody → james
Status: NEW → ASSIGNED
Comment on attachment 752337 [details] [diff] [review]
WIP update to gl to implement OES_vertex_array_object

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a43a331c2cc

To make life easier for those checking in on your behalf, please make sure that future patches follow the guidelines below. Thanks!
Attachment #752337 - Flags: checkin?(jgilbert) → checkin+
And by link below, I meant this of course :P
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Whiteboard: [mentor=jgilbert][lang=c++][games:p2] webgl-extension → [mentor=jgilbert][lang=c++][games:p2][leave open] webgl-extension
https://hg.mozilla.org/mozilla-central/rev/5a43a331c2cc
Why [leave open] ?
(Reporter)

Comment 48

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #47)
> Why [leave open] ?

Because the patch we landed only adds support for this extension to GLContext, not WebGLContext.
Alright, so let me first provide some context: Guillaume Abadie is going to work on starting our WebGL 2 implementation, and our first priority there is the subset of WebGL 2 (i.e. of ES 3.0) that is already specced as WebGL 1 extensions. That makes VAO's a particularly high priority.

Given that, Guillaume had a look at the work already done here today, and it seems like a lot of great work is already done, in the patch already reviewed above (not to mention the GLContext part that already landed), and just some works remains to be done to fix remaining issues.

So: James, if you already happen to have the rest done, that's great, otherwise, no worries, Guillaume can give you a hand. That could take the form of landing your patch as is, together with a separate fixup patch that he would write soon, so that credit is given where credit is due. I hope that sounds fair & a good use of the great work already done here.
(Assignee)

Comment 50

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #49)
> Alright, so let me first provide some context: Guillaume Abadie is going to
> work on starting our WebGL 2 implementation, and our first priority there is
> the subset of WebGL 2 (i.e. of ES 3.0) that is already specced as WebGL 1
> extensions. That makes VAO's a particularly high priority.
> 
> Given that, Guillaume had a look at the work already done here today, and it
> seems like a lot of great work is already done, in the patch already
> reviewed above (not to mention the GLContext part that already landed), and
> just some works remains to be done to fix remaining issues.
> 
> So: James, if you already happen to have the rest done, that's great,
> otherwise, no worries, Guillaume can give you a hand. That could take the
> form of landing your patch as is, together with a separate fixup patch that
> he would write soon, so that credit is given where credit is due. I hope
> that sounds fair & a good use of the great work already done here.

I'd be happy t work on it and finish it with Guillaume if that is alright.

Comment 51

4 years ago
Created attachment 768427 [details] [diff] [review]
james' patch, rebased, with minor tweaks to compile

james' patch rebased on top of WebGL_draw_buffers, and build system changes.
Attachment #768427 - Flags: review?(bjacob)

Comment 52

4 years ago
Created attachment 768428 [details] [diff] [review]
patch finishing the OES_vertex_array_object implementation

renaming: WebGLVertexArrayObject.h/.cpp -> WebGLVertexArray.h/.cpp
renaming: WebGLContext::mBoundElementArrayBuffer -> WebGLVertexArray::mBoundElementArrayBuffer
renaming: WebGLContext::mAttribBuffers -> WebGLVertexArray::mAttribBuffers

add: WebGLContext::mDefaultVertexArray
add: WebGLExtensionVertexArray::IsSupported(this);
add: .cpp files in moz.config
add: JS bindings
add: WebIDL
add: APPLE_vertex_array_object support
add: #define LOCAL_GL_VERTEX_ARRAY_BINDING
add: guaranteed without regressions on WebGL conformance 1.0.1 and 1.0.2
add: all tests passed on Push to Try https://tbpl.mozilla.org/?tree=Try&rev=a92be182914b

bug fix: GetVertexAttrib had wrong return with LOCAL_GL_VERTEX_ATTRIB_ARRAY_NORMALIZED
bug fix: GLContext was looking for symbole glBindVertexArrays instead of glBindVertexArray
bug fix: on OES_vertex_array_object's WebGL Conformance test : isVertexArrayOES() is not specified
Attachment #768428 - Flags: review?(bjacob)
Hm, actually attachment 753779 [details] [diff] [review] and the rebased version (attachment 768427 [details] [diff] [review]) are very far from even compiling, so let's give up on the idea of keeping separate patches --- sorry. Let's do a single combined patch.

Comment 54

4 years ago
Created attachment 768477 [details] [diff] [review]
single cobined patch

here is the single unified patch
Attachment #768427 - Attachment is obsolete: true
Attachment #768428 - Attachment is obsolete: true
Attachment #768427 - Flags: review?(bjacob)
Attachment #768428 - Flags: review?(bjacob)
Attachment #768477 - Flags: review?(bjacob)
Comment on attachment 768477 [details] [diff] [review]
single cobined patch

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

Use MOZ_ASSERT instead of NS_ASSERTION to enforce internal c++ code contracts (crashes debug builds).

No need to MOZ_ASSERT that RefPtr's are non-null before dereferencing them: they already check that e.g. in operator->().

::: content/canvas/src/WebGLExtensionVertexArray.cpp
@@ +57,5 @@
> +    if (gl->IsExtensionSupported(gl::GLContext::APPLE_vertex_array_object)) {
> +        return true;
> +    }
> +
> +    return false;

the two last if's and this return can be simplified into just:

    return a || b;

::: content/canvas/src/WebGLVertexArray.cpp
@@ +10,5 @@
> +#include "nsContentUtils.h"
> +
> +using namespace mozilla;
> +
> +JSObject* 

trailing \w

@@ +42,5 @@
> +    if (index >= WebGLuint(mContext->mGLMaxVertexAttribs)) {
> +        if (index == WebGLuint(-1)) {
> +            mContext->ErrorInvalidValue("%s: index -1 is invalid. That probably comes from a getAttribLocation() call, "
> +                              "where this return value -1 means that the passed name didn't correspond to an active attribute in "
> +                              "the specified program.", info);

You can un-indent the two previous lines a bit (code moved from elsewhere).

::: gfx/gl/GLContext.h
@@ +1026,5 @@
>          OES_element_index_uint,
>          OES_vertex_array_object,
>          ARB_vertex_array_object,
>          ARB_draw_buffers,
> +        APPLE_vertex_array_object,

Move this up. Current order doesn't match!
Attachment #768477 - Flags: review?(bjacob) → review-

Comment 56

4 years ago
Created attachment 768512 [details] [diff] [review]
patch for landing
Attachment #768477 - Attachment is obsolete: true
Attachment #768512 - Flags: review?(bjacob)
Comment on attachment 768512 [details] [diff] [review]
patch for landing

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

::: content/canvas/src/WebGLContext.cpp
@@ +266,5 @@
> +     where the gl context is lost, so we unref its mBoundElementArrayBuffer
> +     manualy
> +     */
> +    mDefaultVertexArray->mBoundElementArrayBuffer = nullptr;
> +    mDefaultVertexArray->mAttribBuffers.Clear();

I would much rather just mDefaultVertexArray = nullptr;  , please explain precisely why we couldn't do this.

::: content/canvas/src/WebGLContextGL.cpp
@@ +255,5 @@
> +        gl->fBindVertexArray(array->GLName());
> +
> +        array->SetHasEverBeenBound(true);
> +
> +        mBoundVertexArray = array;

Too many blank lines above.

::: content/canvas/src/WebGLExtensionVertexArray.cpp
@@ +14,5 @@
> +WebGLExtensionVertexArray::WebGLExtensionVertexArray(WebGLContext* context)
> +  : WebGLExtensionBase(context)
> +{
> +    MOZ_ASSERT(IsSupported(context), "should not construct WebGLExtensionVertexArray :"
> +                                       "OES_vertex_array_object unsuported.");

Un-indent

@@ +43,5 @@
> +}
> +
> +bool WebGLExtensionVertexArray::IsSupported(const WebGLContext* context)
> +{
> +    gl::GLContext * gl = context->GL();

One space too many.

@@ +50,5 @@
> +        return gl->IsExtensionSupported(gl::GLContext::OES_vertex_array_object);
> +    }
> +
> +    return (gl->IsExtensionSupported(gl::GLContext::ARB_vertex_array_object) ||
> +            gl->IsExtensionSupported(gl::GLContext::APPLE_vertex_array_object));

Redundant (...)

::: content/canvas/test/webgl/conformance/extensions/oes-vertex-array-object.html
@@ +148,5 @@
>      ext.bindVertexArrayOES(null);
>      shouldBeTrue("ext.isVertexArrayOES(vao)");
>      
> +    /*
> +     * Issue found in the conformance test. The public webgl mailing list has been notified about it.

The tests have already been fixed upstream.

::: gfx/gl/GLContext.cpp
@@ +576,5 @@
> +            }
> +        }
> +        else if (IsExtensionSupported(APPLE_vertex_array_object)) {
> +            /*
> +             * separated call for LoadSymbols with APPLE_vertex_array_object because of

separate call to  .... to work around a driver bug: the IsVertexArray symbol (without suffix) can be present but unusable.
Attachment #768512 - Flags: review?(bjacob) → review-

Comment 58

4 years ago
Created attachment 768549 [details] [diff] [review]
patch for final landing
Attachment #768512 - Attachment is obsolete: true
Attachment #768549 - Flags: review?(bjacob)

Comment 59

4 years ago
Created attachment 768551 [details] [diff] [review]
patch for final landing
Attachment #768549 - Attachment is obsolete: true
Attachment #768549 - Flags: review?(bjacob)
Attachment #768551 - Flags: review?(bjacob)

Comment 60

4 years ago
Created attachment 768562 [details] [diff] [review]
patch for final landing (tiny fix)
Attachment #768551 - Attachment is obsolete: true
Attachment #768551 - Flags: review?(bjacob)
Attachment #768562 - Flags: review?(bjacob)
Attachment #768562 - Flags: review?(bjacob) → review+
Pushed with this identity (crediting both authors) and commit msg:

# User Guillaume Abadie <gabadie@mozilla.com> and James King <james@agentultra.com>
bug 738869 - implement OES_vertex_array_object webgl extension - r=bjacob

https://hg.mozilla.org/integration/mozilla-inbound/rev/88b65229c78c

Thanks James & Guillaume!
Whiteboard: [mentor=jgilbert][lang=c++][games:p2][leave open] webgl-extension → [mentor=jgilbert][lang=c++][games:p2] webgl-extension
Target Milestone: --- → mozilla25
(Reporter)

Comment 62

4 years ago
Created attachment 768661 [details] [diff] [review]
patch: Add VAO tests to failing tests on android

For the record, WebGL tests are only m1 on desktop. On Android, they're in m-gl. (We don't have them on B2G :C)
Attachment #768661 - Flags: review+
(Reporter)

Comment 63

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ccdfa3a392
https://hg.mozilla.org/mozilla-central/rev/88b65229c78c
https://hg.mozilla.org/mozilla-central/rev/c5ccdfa3a392
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 895010

Updated

4 years ago
Depends on: 912255

Updated

4 years ago
Blocks: 710398
You need to log in before you can comment on or make changes to this bug.