Bug 53940 – Implement the OES_vertex_array_object WebGL extension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Ben Vanik]

Initial extension spec work/conformance test/etc coming in patches via Khronos bug 428: http://www.khronos.org/bugzilla/show_bug.cgi?id=428

Once settled I will upload a patch containing an implementation of the extension for WebKit/OSX.

[Ben Vanik]

Created attachment 81566 [details]
Implementation of OES_vertex_array_object for WebKit/OSX

From the ChangeLog:

        Initial implementation of the OES_vertex_array_object extension adding the OESVertexArrayObject
        extension container and WebGLVertexArrayObjectOES VAO object. The extension is plumbed through
        the Extensions3D interface and implemented in the Extensions3DOpenGL (WebKit/OSX) version when
        it is available.
        Two big changes touching code outside of the extension:
        * Moved the typedefs at the top of GraphicsContext3D.h to GraphicsTypes3D.h (modeled after
          GraphicsTypes.h). They are not namespaced as they weren't before.
        * To make the code cleaner/clearer all vertex attribute state has been moved to the
          WebGLVertexArrayObjectOES type (class VertexAttribState) except for values which are still
          on the WebGLRenderingContext. A default VAO is now used to store the existing attribute
          states for when no other VAO is used. Code in WebGLRenderingContext dealing with buffers and
          vertex attributes now defers to or stores values in the bound array object.
        
        Tested against the WebGL conformance suite and the new
        oes-vertex-array-object test.

[Ben Vanik]

Created attachment 81568 [details]
Implementation of OES_vertex_array_object for WebKit/OSX (reupload)

Broke the first patch file, trying again

[Ben Vanik]

Created attachment 81569 [details]
Implementation of OES_vertex_array_object for WebKit/OSX (reupload)

One last try of this upload before I ask for help...

[Kenneth Russell]

Comment on attachment 81569 [details]
Implementation of OES_vertex_array_object for WebKit/OSX (reupload)

View in context: https://bugs.webkit.org/attachment.cgi?id=81569&action=review

Great work. The patch looks almost perfect. r- for a few relatively minor issues. It's generally difficult to land a patch via the commit queue that modifies project.pbxproj because there's a lot of churn in that file. Ping me when you're ready to upload a new patch and we can try to get it landed quickly.

> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

There are slight differences among the copyright notices in the new files. Could you please normalize them all to what's in Source/WebKit/LICENSE?

> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:65
> +    return o;

Prefer "return o.release();".

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1381
> +        if (loc >= 0 && loc < (int)m_maxVertexAttribs) {

Use static_cast<int>(...).

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:41
> +    static PassRefPtr<WebGLVertexArrayObjectOES> create(WebGLRenderingContext*, bool);

Recently there's been a lot of discussion on webkit-dev about preferring descriptive enums instead of bools. Please define an enum (perhaps something like "NormalVAO", "DefaultVAO"?) and pass it instead of the bool. You could give it a default value so most of the call sites don't have to supply it.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:44
> +    class VertexAttribState {

Make this a struct instead of a class, since everything's public. Everything else (in particular the constructor) can remain.

> Source/WebCore/platform/graphics/Extensions3D.h:116
> +    virtual Platform3DObject createVertexArrayOES() = 0;
> +    virtual void deleteVertexArrayOES(Platform3DObject) = 0;
> +    virtual GC3Dboolean isVertexArrayOES(Platform3DObject) = 0;
> +    virtual void bindVertexArrayOES(Platform3DObject) = 0;

You'll need to stub these out in Source/WebCore/platform/graphics/qt/Extensions3DQt.{cpp,h}. I'm not sure why the Qt EWS bot compiled this patch successfully -- maybe they're not currently compiling WebGL.

> Source/WebCore/platform/graphics/GraphicsTypes3D.h:26
> +#ifndef GraphicsTypes3D_h

This header needs to be added to a couple more of the build systems, in particular Source/WebCore/WebCore.pro and Source/WebCore/WebCore.gypi. Even though compilation didn't fail, the dependencies might not be correct.

[Ben Vanik]

Created attachment 82854 [details]
Revised implementation of OES_vertex_array_object

Changes from Ken's feedback

[Kenneth Russell]

Comment on attachment 82854 [details]
Revised implementation of OES_vertex_array_object

View in context: https://bugs.webkit.org/attachment.cgi?id=82854&action=review

Looks great. I'll help you land this by hand tomorrow morning so we can watch the bots. One tiny naming convention issue; not necessary to upload a new patch for it, but if you have time to that would be great.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:41
> +        kVaoTypeDefault,
> +        kVaoTypeUser,

WebKit style doesn't use the "k" for these, paradoxically -- so they should just be VaoTypeDefault and VaoTypeUser.

[Kenneth Russell]

Committed r79011: <http://trac.webkit.org/changeset/79011>