WebGL OES_vertex_array_object windows presence is low

RESOLVED FIXED in mozilla32

Status

()

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

People

(Reporter: Florian Bösch, Assigned: walter)

Tracking

unspecified
mozilla32
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Assignee: nobody → dglastonbury
Component: Untriaged → Canvas: WebGL
OS: Linux → Windows 7
Product: Firefox → Core
ANGLE doesn't export OES_vertex_array_object extension. I'm guessing that Chromium simulates the extension.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

4 years ago
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).
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

4 years ago
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.
Assignee: dglastonbury → wlitwinczyk
Giving this (maybe) to :walter, unless you still want this, Dan.
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8427339 [details] [diff] [review]
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.
Attachment #8427339 - Flags: review?(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.
Attachment #8427339 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 9

4 years ago
Created attachment 8428166 [details] [diff] [review]
Attempt #2

Note the comment in WebGLContextValidate.cpp
Attachment #8427339 - Attachment is obsolete: true
Attachment #8428166 - Flags: review?(jgilbert)
(Assignee)

Comment 10

4 years ago
Created attachment 8428170 [details] [diff] [review]
vert_array_obj_ext

Sorry, it's friday, forgot to check the build.
Attachment #8428166 - Attachment is obsolete: true
Attachment #8428166 - Flags: review?(jgilbert)
(Assignee)

Updated

4 years ago
Attachment #8428170 - Flags: review?(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.
Attachment #8428170 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 12

4 years ago
Created attachment 8429505 [details] [diff] [review]
vert_array_obj_ext
Attachment #8428170 - Attachment is obsolete: true
Attachment #8429505 - Flags: review?(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)
{}
Attachment #8429505 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 8429588 [details] [diff] [review]
vert_array_obj_ext
Attachment #8429505 - Attachment is obsolete: true
Attachment #8429588 - Flags: review?(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?
Attachment #8429588 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 16

4 years ago
Created attachment 8430357 [details] [diff] [review]
vert_array_obj_ext
Attachment #8429588 - Attachment is obsolete: true
Attachment #8430357 - Flags: review?(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.
Attachment #8430357 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8431843 [details] [diff] [review]
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
Attachment #8430357 - Attachment is obsolete: true
Attachment #8431843 - Flags: review?(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`?
Attachment #8431843 - Flags: review?(jgilbert) → review-
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().
(Assignee)

Comment 21

4 years ago
Created attachment 8434375 [details] [diff] [review]
vert_array_obj_ext

Try: https://tbpl.mozilla.org/?tree=Try&rev=efb0f9957909 (finally all green!)
Attachment #8431843 - Attachment is obsolete: true
Attachment #8434375 - Flags: review?(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.
Attachment #8434375 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 23

4 years ago
Created attachment 8434538 [details] [diff] [review]
vert_array_obj_ext
Attachment #8434375 - Attachment is obsolete: true
Attachment #8434538 - Flags: review?(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.
Attachment #8434538 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 8435051 [details] [diff] [review]
vert_array_obj_ext:

r=jgilbert carried forward
Attachment #8434538 - Attachment is obsolete: true
Attachment #8435051 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

4 years ago
Created attachment 8435365 [details] [diff] [review]
vert_obj

r=jgilbert carried forward, was missing author information
Attachment #8435051 - Attachment is obsolete: true
Attachment #8435365 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/63f80cbd7c35
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Updated

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