966998 - WebGL OES_vertex_array_object windows presence is low

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Florian Bösch]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.77 Safari/537.36

Steps to reproduce:

Consulting data from http://webglstats.com/ Firefox on Windows only has the OES_vertex_array_object for 0.1% of visitors.

Chrome has it for 99.4% of visitors on Windows, and both firefox and chrome show substantial percentages for it on OSX and Linux.

This might indicate a bug, malconfiguration or outdated Angle version in Firefox for Windows.

Comment 1

[u480271]

ANGLE doesn't export OES_vertex_array_object extension. I'm guessing that Chromium simulates the extension.

Comment 2

[Andre Weissflog]

Sorry for intruding the thread, I just stumbled across the same problem, started a new engine (https://github.com/floooh/oryol) and thought that vertex_array_extension would be widespread enough that I wouldn't need to implement a fallback, only to discover that FF on Windows doesn't expose it (only when the "use native GL" flag is set). While the extension itself isn't very well designed IMHO, it still can reduce calls into WebGL drastically (in may case up to 11 calls replaced by 1 call when switchting geometry), which in WebGL (and the emscripten GL wrapper) is much more important then native GL, since there's more overhead per call, so supporting it in WebGL would be a good thing IMHO (along with any other extension which reduces GL calls, e.g. for hardware instancing).

Comment 3

[u480271]

Of course.

We're using ANGLE for windows which doesn't export the required extension. I've investigated Chromium and they simulate the required ext in their WebGL implementation. We just need to schedule time to do the work.

Comment 4

[Andre Weissflog]

That sounds great! In the meantime I implemented the fallback path, the Oryol samples should now also work in Windows on FF: http://floooh.github.io/oryol, the plan is to make this list of samples much longer, each testing/demonstrating a small set of features, so that it will be easier to pinpoint the reason why a specific problems occurs on a specific browser.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Giving this (maybe) to :walter, unless you still want this, Dan.

Comment 6

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Giving this (maybe) to :walter, unless you still want this, Dan.

Nah, I haven't had time. Chrome simulates vertex_array_object. We should do the same. Would be an interesting "meaty" project for :walter.

Comment 7

[Walter Litwinczyk [:walter]]

Attachment: First attempt at simulating VAOs

First attempt at emulating VAOs. I'm not too clear on how mDefaultVertexArray is being used in the code, as in just a sentinel or something more. Also wasn't positive on what it should mean when you unbind the emulated VAO.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8427339 [details] [diff] [review]
First attempt at simulating VAOs

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

Cool, this is the right idea.

::: content/canvas/src/WebGLContextVertexArray.cpp
@@ +42,4 @@
>          mBoundVertexArray = array;
>      }
>      else {
> +        array->UnBindVertexArray();

`array` is null here.

@@ +56,5 @@
> +    nsRefPtr<WebGLVertexArray> globj;
> +    if (gl->IsSupported(gl::GLFeature::vertex_array_object)) {
> +        globj = new WebGLVertexArrayGL(this);
> +    }
> +    else {

`} else {` on one line. Fix the one above this, too, if you would.

::: content/canvas/src/WebGLVertexArrayFake.cpp
@@ +16,5 @@
> +    // Go through and re-bind all buffers and setup all
> +    // vertex attribute pointers
> +    gl::GLContext* gl = mContext->gl;
> +    for (size_t i = 0; i < mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData &vd(mAttribs[i]);

& to the left, and prefer operator= instead of the copy-constructor.

@@ +18,5 @@
> +    gl::GLContext* gl = mContext->gl;
> +    for (size_t i = 0; i < mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData &vd(mAttribs[i]);
> +
> +        if (vd.enabled) {

I think we need to reset this data anyways, even if this is disabled, since the next command might be to re-enable this.

@@ +22,5 @@
> +        if (vd.enabled) {
> +            mContext->BindBuffer(vd.buf->Target(), vd.buf);
> +
> +            gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized,
> +                    vd.stride, reinterpret_cast<void*>(vd.byteOffset));

We also need to reset the non-pointer data with vertexAttrib4f/fv

::: content/canvas/src/WebGLVertexArrayGL.cpp
@@ +8,5 @@
> +#include "WebGLVertexArrayGL.h"
> +#include "GLContext.h"
> +
> +void
> +WebGLVertexArrayGL::Delete() {

Format as:
returntype
class::name()
{
  code
}

@@ +29,5 @@
> +    mContext->gl->fBindVertexArray(mGLName);
> +}
> +
> +void
> +WebGLVertexArrayGL::UnBindVertexArray()

I don't think we should bother with unbinding anymore. We should just 'bind' 'vao 0'.

::: content/canvas/src/WebGLVertexArrayGL.h
@@ +26,5 @@
> +
> +    // -------------------------------------------------------------------------
> +    // CONSTRUCTOR & DESTRUCTOR
> +
> +    WebGLVertexArrayGL(WebGLContext *context) :

Star to the left.

Comment 9

[Walter Litwinczyk [:walter]]

Attachment: Attempt #2

Note the comment in WebGLContextValidate.cpp

Comment 10

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Sorry, it's friday, forgot to check the build.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8428170 [details] [diff] [review]
vert_array_obj_ext

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

::: content/canvas/src/WebGLVertexArray.h
@@ +38,5 @@
> +    // -------------------------------------------------------------------------
> +    //
> +
> +    virtual void BindVertexArray() {
> +        SetHasEverBeenBound(true);

To avoid forgetting to call this when we override this function, we should do:
void BindVertexArray() {
  SetHasEverBeenBound(true);
  BindVertexArrayImpl();
}

virtual void BindVertexArrayImpl() = 0;

@@ +83,5 @@
>  
>      // -------------------------------------------------------------------------
>      // MEMBERS
>  
>      GLuint mGLName;

Remove this member from the base class, since it only applies to the GL one.

::: content/canvas/src/WebGLVertexArrayFake.cpp
@@ +18,5 @@
> +    gl::GLContext* gl = mContext->gl;
> +    for (size_t i = 0; i < mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData& vd = mAttribs[i];
> +
> +        mContext->BindBuffer(vd.buf->Target(), vd.buf);

You're going to need to restore the previous buffer binding state after you call VertexAttribPointer.

::: content/canvas/src/WebGLVertexArrayFake.h
@@ +12,5 @@
> +#include "WebGLVertexAttribData.h"
> +
> +#include "nsWrapperCache.h"
> +
> +#include "mozilla/LinkedList.h"

A bunch of these headers seem unneeded. Remove all the ones you can.

@@ +41,5 @@
> +
> +
> +// -----------------------------------------------------------------------------
> +// PRIVATE
> +private:

Remove unused boilerplate.

::: content/canvas/src/WebGLVertexArrayGL.h
@@ +27,5 @@
> +    // -------------------------------------------------------------------------
> +    // CONSTRUCTOR & DESTRUCTOR
> +
> +    WebGLVertexArrayGL(WebGLContext* context) :
> +        WebGLVertexArray(context)

Initializer lists are like:
Class(args)
  : var0(foo)
  , var1(foo)
{
    code
}

@@ +42,5 @@
> +    void GenVertexArray() MOZ_OVERRIDE;
> +
> +// -----------------------------------------------------------------------------
> +// PRIVATE
> +private:

Don't leave this if we don't have anything here. We don't really want to boilerplating.

Comment 12

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8429505 [details] [diff] [review]
vert_array_obj_ext

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

Let's fix up Delete like BindVertexArray, and we can r+ this.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1834,5 @@
> +    //        Is the mDefaultVertexArray ever used, or only checked?
> +    if (gl->IsSupported(gl::GLFeature::vertex_array_object)) {
> +        mDefaultVertexArray = new WebGLVertexArrayGL(this);
> +    } else {
> +        mDefaultVertexArray = new WebGLVertexArrayFake(this);

Better than this might be having a:
static WebGLVertexArray::Create(this) {
  if (ext supported)
    return gl
  else
    return emulated
}

::: content/canvas/src/WebGLVertexArrayFake.h
@@ +17,5 @@
> +// PUBLIC
> +public:
> +
> +    // -------------------------------------------------------------------------
> +    // CONSTRUCTOR & DESTRUCTOR

For classes this small, we really don't need any of this boilerplate. The boilerplate is like 30% of this class's lines.

::: content/canvas/src/WebGLVertexArrayGL.cpp
@@ +15,5 @@
> +        mContext->gl->fDeleteVertexArrays(1, &mGLName);
> +        LinkedListElement<WebGLVertexArray>::removeFrom(mContext->mVertexArrays);
> +    }
> +
> +    WebGLVertexArray::Delete();

Like BindVertexArray, having to remember to call this is risky, so change Delete to have a DeleteImpl, too.

@@ +21,5 @@
> +
> +void
> +WebGLVertexArrayGL::BindVertexArrayImpl()
> +{
> +    WebGLVertexArray::BindVertexArray();

Remove this line.

::: content/canvas/src/WebGLVertexArrayGL.h
@@ +20,5 @@
> +    // -------------------------------------------------------------------------
> +    // CONSTRUCTOR & DESTRUCTOR
> +
> +    WebGLVertexArrayGL(WebGLContext* context) :
> +        WebGLVertexArray(context)

Move : to next line, like:
Class(args)
  : mFoo(foo)
{}

Comment 14

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8429588 [details] [diff] [review]
vert_array_obj_ext

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

R- unless I'm wrong about the removal from mVertexArrays.

::: content/canvas/src/WebGLVertexArrayFake.cpp
@@ +10,5 @@
> +{
> +    // Go through and re-bind all buffers and setup all
> +    // vertex attribute pointers
> +    gl::GLContext* gl = mContext->gl;
> +    WebGLRefPtr<WebGLBuffer> mPrevBuffer = mContext->mBoundArrayBuffer;

'm' prefix is for members of non-POD classes. s/mPrevBuffer/prevBuffer/

@@ +17,5 @@
> +
> +        mContext->BindBuffer(vd.buf->Target(), vd.buf);
> +
> +        gl->fVertexAttribPointer(i, vd.size, vd.type, vd.normalized,
> +                vd.stride, reinterpret_cast<void*>(vd.byteOffset));

Indent to match the (

::: content/canvas/src/WebGLVertexArrayGL.cpp
@@ +12,5 @@
> +        mBoundElementArrayBuffer = nullptr;
> +
> +        mContext->MakeContextCurrent();
> +        mContext->gl->fDeleteVertexArrays(1, &mGLName);
> +        LinkedListElement<WebGLVertexArray>::removeFrom(mContext->mVertexArrays);

Surely removing this from the list of vertex arrays should be in the base class, not just GL?

Comment 16

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8430357 [details] [diff] [review]
vert_array_obj_ext

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

Alright, great. Let's see what Try thinks.

Comment 18

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Made some changes because of try submission. These are the new try results:

https://tbpl.mozilla.org/?tree=Try&rev=da13d658c808

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8431843 [details] [diff] [review]
vert_array_obj_ext

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

::: content/canvas/src/WebGLVertexArrayFake.cpp
@@ +18,5 @@
> +    WebGLRefPtr<WebGLBuffer> prevBuffer = mContext->mBoundArrayBuffer;
> +    for (size_t i = 0; i < mAttribs.Length(); ++i) {
> +        const WebGLVertexAttribData& vd = mAttribs[i];
> +
> +        mContext->BindBuffer(vd.buf->Target(), vd.buf);

This is unchanged. Did you forget to `hg qref`?

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8431843 [details] [diff] [review]
vert_array_obj_ext

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

::: content/canvas/src/WebGLContextVertexArray.cpp
@@ +42,1 @@
>          gl->fBindVertexArray(0);

This should be mDefaultVAO->Bind().

Comment 21

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Try: https://tbpl.mozilla.org/?tree=Try&rev=efb0f9957909 (finally all green!)

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8434375 [details] [diff] [review]
vert_array_obj_ext

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

Cool, thanks.

::: content/canvas/src/WebGLVertexArray.cpp
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "WebGLContext.h"
>  #include "WebGLBuffer.h"
>  #include "WebGLVertexArray.h"

Always include the header for a file first, in the CPP. Move "WebGLVertexArray.h" to the top here. (and generally leave a blank line between it and the includes that are needed internally for this CPP.

::: content/canvas/src/WebGLVertexArray.h
@@ +84,5 @@
>  
>      GLuint mGLName;
>      bool mHasEverBeenBound;
>      nsTArray<WebGLVertexAttribData> mAttribs;
>      WebGLRefPtr<WebGLBuffer> mBoundElementArrayBuffer;

s/mBoundElementArrayBuffer/mElementArrayBuffer/, since I feel like otherwise it might cause confusion as to why we 'bind' the 'boundElementArrayBuffer', since it should be already bound, no? (Not necessarily, because it's the VAO index buffer, not necessarily the currently bound index buffer for the context (which may or may not have VAO support))

::: content/canvas/src/WebGLVertexArrayFake.h
@@ +14,5 @@
> +    : public WebGLVertexArray
> +{
> +public:
> +
> +    void BindVertexArrayImpl() MOZ_OVERRIDE;

We don't need a blank line between `public:` and function decls.

@@ +29,5 @@
> +        DeleteOnce();
> +    }
> +
> +    friend class WebGLVertexArray;
> +

Unnecessary blank line.

Comment 23

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8434538 [details] [diff] [review]
vert_array_obj_ext

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

::: content/canvas/src/WebGLVertexArrayFake.h
@@ +15,5 @@
> +{
> +public:
> +    void BindVertexArrayImpl() MOZ_OVERRIDE;
> +    void DeleteImpl() MOZ_OVERRIDE { };
> +    void GenVertexArray() MOZ_OVERRIDE { };

Please mark these not just MOZ_OVERRIDE, but also virtual.
Yes, it's implied, but since there's a difference, let's be explicit. The fewer transformations we have to hold in our head between code-on-the-page and what-the-code-actually-means, the better.

@@ +18,5 @@
> +    void DeleteImpl() MOZ_OVERRIDE { };
> +    void GenVertexArray() MOZ_OVERRIDE { };
> +
> +private:
> +

You don't need a blank line after 'private:' either. :P

::: content/canvas/src/WebGLVertexArrayGL.cpp
@@ +12,5 @@
> +
> +void
> +WebGLVertexArrayGL::DeleteImpl()
> +{
> +    if (mGLName != 0) {

MOZ_ASSERT(mGLName);
I don't see how this should ever be zero, anymore.

Comment 25

[Walter Litwinczyk [:walter]]

Attachment: vert_array_obj_ext:

r=jgilbert carried forward

Comment 26

[Walter Litwinczyk [:walter]]

Attachment: vert_obj

r=jgilbert carried forward, was missing author information

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63f80cbd7c35

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/63f80cbd7c35