Last Comment Bug 738869 - Implement WebGL OES_vertex_array_object extension
: Implement WebGL OES_vertex_array_object extension
Status: RESOLVED FIXED
[mentor=jgilbert][lang=c++][games:p2]...
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: mozilla25
Assigned To: James King
:
:
Mentors:
http://www.khronos.org/registry/webgl...
Depends on: 895010 912255
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-03-23 17:41 PDT by Jeff Gilbert [:jgilbert]
Modified: 2013-11-06 14:06 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP implementation of OES_vertex_array_object (5.14 KB, patch)
2013-04-10 07:16 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP implementation of OES_vertex_array_object (3.28 KB, patch)
2013-04-10 07:16 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
Added the implementation of WebGLExtensionVertexArrayObject (11.80 KB, patch)
2013-04-11 07:39 PDT, James King
no flags Details | Diff | Splinter Review
Updated based on Jeff's review (3.34 KB, patch)
2013-04-11 08:36 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
Updated based on Jeff's review (5.18 KB, patch)
2013-04-11 08:37 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP gfx/gl update for OES/ARB vertex array object (5.84 KB, patch)
2013-04-16 10:22 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject (8.58 KB, patch)
2013-04-16 10:23 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject (19.57 KB, patch)
2013-05-21 13:09 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP update to gl to implement OES_vertex_array_object (9.75 KB, patch)
2013-05-21 13:15 PDT, James King
jgilbert: review+
ryanvm: checkin+
Details | Diff | Splinter Review
WIP update following jgilbert's review (19.39 KB, patch)
2013-05-22 09:13 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
WIP update to canvas following jgilbert's review (19.25 KB, patch)
2013-05-24 07:24 PDT, James King
jgilbert: review-
Details | Diff | Splinter Review
james' patch, rebased, with minor tweaks to compile (20.51 KB, patch)
2013-06-27 10:42 PDT, Guillaume Abadie
no flags Details | Diff | Splinter Review
patch finishing the OES_vertex_array_object implementation (58.73 KB, patch)
2013-06-27 10:43 PDT, Guillaume Abadie
no flags Details | Diff | Splinter Review
single cobined patch (56.64 KB, patch)
2013-06-27 11:38 PDT, Guillaume Abadie
jacob.benoit.1: review-
Details | Diff | Splinter Review
patch for landing (55.93 KB, patch)
2013-06-27 12:29 PDT, Guillaume Abadie
jacob.benoit.1: review-
Details | Diff | Splinter Review
patch for final landing (61.33 KB, patch)
2013-06-27 13:47 PDT, Guillaume Abadie
no flags Details | Diff | Splinter Review
patch for final landing (54.88 KB, patch)
2013-06-27 13:48 PDT, Guillaume Abadie
no flags Details | Diff | Splinter Review
patch for final landing (tiny fix) (54.70 KB, patch)
2013-06-27 14:02 PDT, Guillaume Abadie
jacob.benoit.1: review+
Details | Diff | Splinter Review
patch: Add VAO tests to failing tests on android (907 bytes, patch)
2013-06-27 17:07 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review

Description Jeff Gilbert [:jgilbert] 2012-03-23 17:41:29 PDT
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
Comment 1 Mark 2012-05-29 21:01:38 PDT
Started looking into bug and working on it.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-09-06 11:49:07 PDT
Mark, still looking into this?  Let us know if you have any questions on where/how to get started.
Comment 3 Mark 2012-09-09 10:38:27 PDT
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 Peter Panaguiton 2012-10-10 15:51:16 PDT
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.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-10-10 19:32:25 PDT
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.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-10-10 20:30:17 PDT
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 Mark 2012-10-11 01:03:56 PDT
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 :)
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-10-11 06:48:08 PDT
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.
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-11 07:49:19 PDT
Note: bug 738867 is another webgl extension that would be a good first bug/first extension.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-10-11 08:27:04 PDT
Vlad: Jon Buckley and/or :dwarfcrank is already looking into this one.
Comment 12 Alon Zakai (:azakai) 2013-01-19 10:07:51 PST
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.
Comment 13 James King 2013-04-10 07:16:04 PDT
Created attachment 735755 [details] [diff] [review]
WIP implementation of OES_vertex_array_object
Comment 14 James King 2013-04-10 07:16:30 PDT
Created attachment 735756 [details] [diff] [review]
WIP implementation of OES_vertex_array_object
Comment 15 Vladimir Vukicevic [:vlad] [:vladv] 2013-04-10 08:04:53 PDT
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.
Comment 16 Jeff Gilbert [:jgilbert] 2013-04-10 15:40:28 PDT
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
Comment 17 Jeff Gilbert [:jgilbert] 2013-04-10 15:54:44 PDT
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`.
Comment 18 Jeff Gilbert [:jgilbert] 2013-04-10 15:55:38 PDT
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.
Comment 19 Jeff Gilbert [:jgilbert] 2013-04-10 15:56:10 PDT
(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.
Comment 20 James King 2013-04-11 07:39:37 PDT
Created attachment 736288 [details] [diff] [review]
Added the implementation of WebGLExtensionVertexArrayObject

Forgot to add the (rather light) implementation I have so far.
Comment 21 James King 2013-04-11 07:42:03 PDT
Jeff, I will keep the patches separate going forward.
Comment 22 James King 2013-04-11 08:36:05 PDT
Created attachment 736309 [details] [diff] [review]
Updated based on Jeff's review
Comment 23 James King 2013-04-11 08:37:12 PDT
Created attachment 736310 [details] [diff] [review]
Updated based on Jeff's review
Comment 24 Jeff Gilbert [:jgilbert] 2013-04-11 15:51:23 PDT
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.
Comment 25 Jeff Gilbert [:jgilbert] 2013-04-11 15:54:02 PDT
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`.
Comment 26 James King 2013-04-16 10:22:46 PDT
Created attachment 738080 [details] [diff] [review]
WIP gfx/gl update for OES/ARB vertex array object
Comment 27 James King 2013-04-16 10:23:53 PDT
Created attachment 738081 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject
Comment 28 Jeff Gilbert [:jgilbert] 2013-04-23 18:26:17 PDT
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.
Comment 29 Jeff Gilbert [:jgilbert] 2013-04-23 18:35:15 PDT
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.
Comment 30 Jeff Gilbert [:jgilbert] 2013-04-23 18:43:21 PDT
Also, check WebGLContext::CreateRenderbuffer and WebGLContext::DeleteRenderbuffer (etc) for a taste of how the implementation of CreateVertexArray (etc) should work.
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2013-04-29 13:18:34 PDT
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?
Comment 32 James King 2013-04-29 13:20:48 PDT
Thanks :bjacob;

It should be a GLuint and I'm in the process of following through with Jeff's comments.
Comment 33 James King 2013-05-21 13:09:51 PDT
Created attachment 752332 [details] [diff] [review]
WIP update to content/canvas/ add WebGLExtensionVertexArrayObject
Comment 34 James King 2013-05-21 13:13:49 PDT
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.
Comment 35 James King 2013-05-21 13:15:44 PDT
Created attachment 752337 [details] [diff] [review]
WIP update to gl to implement OES_vertex_array_object
Comment 36 Jeff Gilbert [:jgilbert] 2013-05-21 23:01:18 PDT
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.
Comment 37 James King 2013-05-22 09:13:27 PDT
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.
Comment 38 Jeff Gilbert [:jgilbert] 2013-05-22 23:56:43 PDT
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.
Comment 39 James King 2013-05-24 07:24:15 PDT
Created attachment 753779 [details] [diff] [review]
WIP update to canvas following jgilbert's review
Comment 40 Benoit Jacob [:bjacob] (mostly away) 2013-06-04 12:04:16 PDT
review ping!
Comment 41 Jeff Gilbert [:jgilbert] 2013-06-04 15:10:49 PDT
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.
Comment 42 James King 2013-06-04 19:26:50 PDT
I had a feeling it couldn't quite be that simple. I'll take a peek at this ASAP.
Comment 43 Jeff Gilbert [:jgilbert] 2013-06-13 15:24:37 PDT
Comment on attachment 752337 [details] [diff] [review]
WIP update to gl to implement OES_vertex_array_object

Let's at least take this patch.
Comment 44 Ryan VanderMeulen [:RyanVM] 2013-06-17 13:18:53 PDT
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!
Comment 45 Ryan VanderMeulen [:RyanVM] 2013-06-17 13:19:28 PDT
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
Comment 46 Ed Morley [:emorley] 2013-06-18 04:08:14 PDT
https://hg.mozilla.org/mozilla-central/rev/5a43a331c2cc
Comment 47 Benoit Jacob [:bjacob] (mostly away) 2013-06-20 12:17:03 PDT
Why [leave open] ?
Comment 48 Jeff Gilbert [:jgilbert] 2013-06-20 12:59:39 PDT
(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.
Comment 49 Benoit Jacob [:bjacob] (mostly away) 2013-06-21 15:16:43 PDT
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.
Comment 50 James King 2013-06-23 19:31:25 PDT
(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 Guillaume Abadie 2013-06-27 10:42:27 PDT
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.
Comment 52 Guillaume Abadie 2013-06-27 10:43:51 PDT
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
Comment 53 Benoit Jacob [:bjacob] (mostly away) 2013-06-27 11:05:03 PDT
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 Guillaume Abadie 2013-06-27 11:38:15 PDT
Created attachment 768477 [details] [diff] [review]
single cobined patch

here is the single unified patch
Comment 55 Benoit Jacob [:bjacob] (mostly away) 2013-06-27 12:01:58 PDT
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!
Comment 56 Guillaume Abadie 2013-06-27 12:29:48 PDT
Created attachment 768512 [details] [diff] [review]
patch for landing
Comment 57 Benoit Jacob [:bjacob] (mostly away) 2013-06-27 12:54:40 PDT
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.
Comment 58 Guillaume Abadie 2013-06-27 13:47:16 PDT
Created attachment 768549 [details] [diff] [review]
patch for final landing
Comment 59 Guillaume Abadie 2013-06-27 13:48:46 PDT
Created attachment 768551 [details] [diff] [review]
patch for final landing
Comment 60 Guillaume Abadie 2013-06-27 14:02:47 PDT
Created attachment 768562 [details] [diff] [review]
patch for final landing (tiny fix)
Comment 61 Benoit Jacob [:bjacob] (mostly away) 2013-06-27 14:09:56 PDT
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!
Comment 62 Jeff Gilbert [:jgilbert] 2013-06-27 17:07:07 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.