Kelsey Gilbert [:jgilbert] Reporter
	Description • 12 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

	Mark
	Comment 1 • 12 years ago

Started looking into bug and working on it.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 2 • 12 years ago

Mark, still looking into this?  Let us know if you have any questions on where/how to get started.

	Mark
	Comment 3 • 12 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 :)

	Peter Panaguiton
	Comment 4 • 12 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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 5 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 6 • 12 years ago

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.

	Mark
	Comment 7 • 12 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 :)

	Benoit Jacob [:bjacob] (mostly away)
	Comment 8 • 12 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 9 • 12 years ago

Note: bug 738867 is another webgl extension that would be a good first bug/first extension.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 10 • 12 years ago

Vlad: Jon Buckley and/or :dwarfcrank is already looking into this one.

	MrX1980
	Comment 11 • 12 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

	Alon Zakai (:azakai)
	Comment 12 • 11 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 15 • 11 years ago

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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 16 • 11 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

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 17 • 11 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`.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 18 • 11 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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 19 • 11 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.

	James King Assignee
	Comment 20 • 11 years ago

Forgot to add the (rather light) implementation I have so far.

	James King Assignee
	Comment 21 • 11 years ago

Jeff, I will keep the patches separate going forward.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 24 • 11 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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 25 • 11 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`.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 28 • 11 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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 29 • 11 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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 30 • 11 years ago

Also, check WebGLContext::CreateRenderbuffer and WebGLContext::DeleteRenderbuffer (etc) for a taste of how the implementation of CreateVertexArray (etc) should work.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 31 • 11 years ago

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?

	James King Assignee
	Comment 32 • 11 years ago

Thanks :bjacob;

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

	James King Assignee
	Comment 34 • 11 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.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 36 • 11 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.

	James King Assignee
	Comment 37 • 11 years ago

The linker still cannot find the missing JS Wrap symbols which will need fixing, but I've gone through and cleaned things up.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 38 • 11 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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 40 • 11 years ago

review ping!

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 41 • 11 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.

	James King Assignee
	Comment 42 • 11 years ago

I had a feeling it couldn't quite be that simple. I'll take a peek at this ASAP.

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 43 • 11 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 11 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 46 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5a43a331c2cc

	Benoit Jacob [:bjacob] (mostly away)
	Comment 47 • 11 years ago

Why [leave open] ?

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 48 • 11 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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 49 • 11 years ago

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.

	James King Assignee
	Comment 50 • 11 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.

	Guillaume Abadie
	Comment 51 • 11 years ago

james' patch rebased on top of WebGL_draw_buffers, and build system changes.

	Guillaume Abadie
	Comment 52 • 11 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 53 • 11 years ago

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.

	Guillaume Abadie
	Comment 54 • 11 years ago

here is the single unified patch

	Benoit Jacob [:bjacob] (mostly away)
	Comment 55 • 11 years ago

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!

	Benoit Jacob [:bjacob] (mostly away)
	Comment 57 • 11 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 61 • 11 years ago

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!

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 62 • 11 years ago

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)

	Kelsey Gilbert [:jgilbert] Reporter
	Comment 63 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 64 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/88b65229c78c
https://hg.mozilla.org/mozilla-central/rev/c5ccdfa3a392