775843 - Always BindAttribLocation something to attrib 0 if possible

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

Currently on all desktop GL platforms, there's a performance penalty when vertex attrib zero is not an enabled vertex attrib array. This can be for two reasons:

1:
The GLSL linker set an attrib's location to 0 when it is not an attrib array. (vertexAttrib[1234]f{v} instead of vertexAttribPointer)

2:
The GLSL linker did not assign an attrib to location 0.

We should try to assign *something* to attrib 0, in hopes that we can bind a vertex attrib array to 0.

Note that we won't be able to do anything if someone binds all their attribs to non-zero locations. We can't change the user's binding choices without breaking things, but we *can* try to prevent the linker from doing something silly.

Further, we could incorporate a heuristic where we try to check if one of the attribs is the vertex position attribute, which should always be an array. We can try to check if one of the attribs has a name containing 'position' or 'pos', and try binding that to 0. (if possible)

Comment 1

[Abhishek Potnis [:avp]]

Hi,

I would like to work on this bug, could you please guide me on getting started with it ? 

Thanks

Comment 2

[Kelsey Gilbert [:jgilbert]]

Looks like I got interrupted before posting this.

(In reply to Abhishek Potnis [:abhishekp] from comment #1)
> Hi,
> 
> I would like to work on this bug, could you please guide me on getting
> started with it ? 
> 
> Thanks

Sure. Roughly what we want to do is, right before WebGL's linkProgram, check through the program's vertex attribs and check if any of them are attrib arrays. If there is at least one vertex attrib array in a program, we want to use glBindAttribLocation to bind one of the vertex attrib arrays to index 0.

Note that if the user has already bound something to attrib index 0, there's nothing we can do. Also, we need to make sure that the attrib array we want to bind to 0 is has not already been bound by the user to some other index.

It is very important to understand the behavior of glBindAttribLocation here. glBindAttribLocation(program, index, name) is called on a program, and tells the driver that *if* it sees an attrib with the given name, to set its location to `index`. That is, we must call BindAttribLocation *before* LinkProgram. Calls after LinkProgram don't do anything until we call LinkProgram again.

Comment 3

[S Sujana]

Hello,

I would like to work on this bug too. I looked for calls to glLinkProgram, and found 2 in GLContext.cpp and 1 in GLContextUtils.cpp. In all 3 cases, the position attribute has already been bound to index 0. Could you tell me if I am looking in the right places?

Thanks

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to S Sujana from comment #3)
> Hello,
> 
> I would like to work on this bug too. I looked for calls to glLinkProgram,
> and found 2 in GLContext.cpp and 1 in GLContextUtils.cpp. In all 3 cases,
> the position attribute has already been bound to index 0. Could you tell me
> if I am looking in the right places?
> 
> Thanks

You want to look at WebGLContext::LinkProgram in WebGLContextGL.cpp.

Comment 5

[Erick Dransch [:edransch]]

There are a few points about this bug that I am wondering about:

It seems like our steps should be 

1) If something is already bound to index 0, we're done

2) Otherwise, for each shader, check each attribute:
    - If the attribute is an attrib array and the attribute is not already bound to an index: bind it to 0

We need functionality that will check (is something bound to index 0?) and (is a given attrib array bound to an index?).
However, since we need to do all of this processing pre-linking, we won't be able to use helpful functions like isAttribInUse or glGetAttribLocation (these are only available post-linking). 

Did I overlook functionality that will allow us to do these checks pre-linking? Or do we need to manually keep track of which attrib binding locations are in use by which attribs?

Comment 6

[Kelsey Gilbert [:jgilbert]]

We should have all the information we need from ANGLE, but we could also use the standard GL functions by running a second pass after we link for the first time:
LinkProgram()
if (FoundSomethingToBindTo0())
  LinkProgram()

Comment 7

[Rahul Gandhi]

Sir i want to take this bug i am well acquainted with the knowledge of c/c++ and would give my best effort to solve this bug.

Comment 8

[naxi246]

How can we check if vertex attrib is an array ?

I have written sample code for FoundSomethingToBindTo0 at this link :
http://pastebin.mozilla.org/2291282

Is it correct?

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to naxi246 from comment #8)
> How can we check if vertex attrib is an array ?
> 
> I have written sample code for FoundSomethingToBindTo0 at this link :
> http://pastebin.mozilla.org/2291282
> 
> Is it correct?

Sort of. You shouldn't do it by shader, but rather by program.

At draw time, `WebGLContext::mAttribBuffers[attribLoc].enabled` is whether or not an attrib slot is an array: `true` means it's an array, `false`, means non-array attrib. (We should change this at some point, but that can happen later)
What we want to do is make sure that when we hit DrawArrays/DrawElements, that `mAttribBuffers[0].enabled` is `true`, so hunt through the array (It's not technically dense, but MAX_VERTEX_ATTRIBS tends to max out at 16, sometimes 30) and find some attribIndex that represents an array.

This is a little tricky, because ultimately, we need the name of an attrib to call `glBindAttribLocation(program, 0, <name>); glLinkProgram(program);` on. The only way to get the name of an attrib from the location is to iterate through glGetActiveAttrib() to get a name, then call glGetAttribLocation(<name>) to get the location.

We don't want to do this every draw call, so we need to cache this mapping, probably in `WebGLProgram::UpdateInfo`. We want to be able to look up the name based on the location, as well as being able to iterate through the active locations.

This way, we can do:
int leastLocationThatIsAnArray = -1;
foreach pair<int, string> cur in WebGLProgram::ActiveAttribMap:
  if WebGLContext::mAttribBuffers[cur.first].enabled:
    if cur.first < leastLocationThatIsAnArray:
      leastLocationThatIsAnArray = cur.first;

if leastLocationThatIsAnArray > 0:
  glBindAttribLocation(program, 0, WebGLProgram::ActiveAttribMap.get(leastLocationThatIsAnArray));
  glLinkProgram(program);

For each program, we query the number of active attribs with glGetProgramiv(GL_ACTIVE_ATTRIBUTES).
Then, we loop over the active attribs[1], and get the `size` and `name` of the attrib via glGetActiveAttrib(). (Note that `name` can be up to glGetProgramiv(GL_ACTIVE_ATTRIBUTE_MAX_LENGTH) characters long!)

Now, for each active attrib, we get the actual attrib location with glGetAttribLocation(activeAttribName).

Comment 10

[Mina Almasry]

I'll take a shot at this, if that's okay. I'll be back or ping you on irc if I have questions. Thanks!

Comment 11

[Mina Almasry]

Attachment: Patch

Here is a patch that builds. I followed the pseudo code you put closely. Do you need a test for this and how do I write one?

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 793724 [details] [diff] [review]
Patch

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

This is the right idea, but seems more complicated than it needs to be. There's also a couple complications with the LinkProgramInternal approach.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2413,5 @@
> +  EnumerateHelper temp;
> +  temp.mAttribBuffers = &mBoundVertexArray->mAttribBuffers;
> +  temp.mLeastArrayLocation = leastArrayLocation;
> +
> +  program->mActiveAttribMap.EnumerateRead(FindLeastArrayLocation, &temp);

If we switch to std::map, this gets much more readable, since we'd just iterate to traverse the map.

@@ +2418,5 @@
> +
> +  if (temp.mLeastArrayLocation > 0) {
> +    gl->fBindAttribLocation(program->GLName(), 0,
> +                            program->mActiveAttribMap.Get(temp.mLeastArrayLocation)
> +                                   ->get());

Don't do this much code inline.

@@ +2419,5 @@
> +  if (temp.mLeastArrayLocation > 0) {
> +    gl->fBindAttribLocation(program->GLName(), 0,
> +                            program->mActiveAttribMap.Get(temp.mLeastArrayLocation)
> +                                   ->get());
> +    LinkProgramInternal(program);

We want to emit a warning here that we're relinking their program to make attrib0 an array.

We also don't want to blindly call an *Internal function, since we'd start triggering errors twice for failed links, and do generally more work than we need to.

We should just do this step at the end of LinkProgram, when we know we've successfully linked.

Further, we actually can't (or shouldn't) do this optimization if the user has deliberately BindAttribLocation'd a non-array attrib to location 0.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +85,5 @@
>              }
>          }
>      }
>  
> +    GLint numActiveAttrs;

Give these a default value. In the case of a GL error, GL will not set numActiveAttrs to anything, and we will act on uninitialized memory.

@@ +91,5 @@
> +
> +    GLint maxNameSize;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);
> +    uint32_t bufSize = maxNameSize + 1;

uint32_t->GLsizei

@@ +92,5 @@
> +    GLint maxNameSize;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);
> +    uint32_t bufSize = maxNameSize + 1;
> +    char attrName[bufSize];

I doubt that this compiles on all our platforms, but if it does, great.

@@ +95,5 @@
> +    uint32_t bufSize = maxNameSize + 1;
> +    char attrName[bufSize];
> +
> +    for (int32_t i = 0; i < numActiveAttrs; i++) {
> +      mContext->gl->fGetActiveAttrib(mGLName, i, bufSize, nullptr, nullptr, nullptr,

`length` is the only arg which can be ignored with `nullptr`. We need to pass junk vars to the others.

::: content/canvas/src/WebGLProgram.h
@@ +110,5 @@
>      NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>      NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLProgram)
>  
> +    // public post-link data
> +    nsClassHashtable<nsUint32HashKey, nsCString> mActiveAttribMap;

Let's just use std::map unless we have a pressing reason to use clunky ns* stuff.

Comment 13

[Mina Almasry]

Attachment: Patch

Here are the changes. Heh I thought the nsHashtables were clunky too, but I thought I have to use them.

Comment 14

[Mina Almasry]

Attachment: Patch

Sorry here are the actual changes.

Comment 15

[Mina Almasry]

Attachment: Patch

Sorry again... fixed indentation. Hopefully my last.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 793857 [details] [diff] [review]
Patch

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

Good, so let's just clean it up. :)
I can push it to Try.

::: content/canvas/src/WebGLContext.h
@@ +471,5 @@
>              return;
>          MakeContextCurrent();
>          gl->fLineWidth(width);
>      }
> +    bool BindArrayAttribToLocation0(WebGLProgram *program);

This needs to be private.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2278,5 @@
> +    int32_t leastArrayLocation = -1;
> +
> +    for (auto i = program->mActiveAttribMap.begin();
> +            i != program->mActiveAttribMap.end();
> +            i++) {

I don't think we can use auto yet. Pull out the now-huge itr decl to the line before. Also, prefer 'itr' to 'i' for iterators.

Indents for conditional continuation should be like this:
if (foo &&
    bar)
{
    bax();
}

@@ +2279,5 @@
> +
> +    for (auto i = program->mActiveAttribMap.begin();
> +            i != program->mActiveAttribMap.end();
> +            i++) {
> +        int32_t index(i->first);

Use '=' for assignment of primitive types.

@@ +2288,5 @@
> +    }
> +
> +    if (leastArrayLocation > 0) {
> +        nsCString& attrName = program->mActiveAttribMap.find(leastArrayLocation)->second;
> +        const char * attrNameCStr = attrName.get();

Star to the left, as 'char*'.

@@ +2332,5 @@
>          program->SetLinkStatus(false);
>          return;
>      }
>  
> +    bool updateInfoSucceeded;

Why is this way out here?

@@ +2349,5 @@
> +
> +        if (ok) {
> +            updateInfoSucceeded = program->UpdateInfo();
> +            program->SetLinkStatus(updateInfoSucceeded);
> +            if (BindArrayAttribToLocation0(program)) {

Newline between SetLinkStatus and the if, since they're not directly related.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +88,5 @@
>  
> +    GLint numActiveAttrs = 0;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &numActiveAttrs);
> +
> +    GLint maxNameSize = 127;

257 is better, since our limit for GLSL idents is 256.

::: content/canvas/src/WebGLProgram.h
@@ +110,5 @@
>      NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>      NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebGLProgram)
>  
> +    // public post-link data
> +    std::map<uint32_t, nsCString> mActiveAttribMap;

s/uint32_t/GLint/

Comment 17

[Mina Almasry]

Attachment: Patch v2

Comment 18

[Mina Almasry]

> Good, so let's just clean it up. :)

My pleasure :-)

> I can push it to Try.

I can also push to try! https://tbpl.mozilla.org/?tree=Try&rev=ff4beb5e86ec

> Indents for conditional continuation should be like this:

Do you also want the braces after the for loop on the next line? like
for (something;
     somethingElse;
     somethingElseElse)
{

}

?

> > +    bool updateInfoSucceeded;
> 
> Why is this way out here?

It is used in the next if statement, so it needs to be in that scope too.

I've also added a couple of things you didn't ask for:

char attrName[bufSize] became:
char* attrName = (char*) malloc(bufSize * sizeof(char));
if (!attrName) return false;

Also, 
if (!numActiveAttrs) return false;

I assume if numActiveAttrs is still 0 after the call to fGetProgramiv, then the call has actually failed, and we should inform the caller. Or could numActiveAttrs be 0 and valid?

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Mina Almasry from comment #18)
> > Good, so let's just clean it up. :)
> 
> My pleasure :-)
> 
> > I can push it to Try.
> 
> I can also push to try! https://tbpl.mozilla.org/?tree=Try&rev=ff4beb5e86ec
Great! :)
> 
> > Indents for conditional continuation should be like this:
> 
> Do you also want the braces after the for loop on the next line? like
> for (something;
>      somethingElse;
>      somethingElseElse)
> {
> 
> }
> 
> ?
We're preferring dropping the open-brace to the next line for multi-line conditionals.
This is particularly helpful for readability surrounding if statements:
if (foo() &&
    bar() &&
    bax()) {
    qux();
}
vs:
if (foo() &&
    bar() &&
    bax())
{
    qux();
}
> 
> > > +    bool updateInfoSucceeded;
> > 
> > Why is this way out here?
> 
> It is used in the next if statement, so it needs to be in that scope too.
> 
> I've also added a couple of things you didn't ask for:
> 
> char attrName[bufSize] became:
> char* attrName = (char*) malloc(bufSize * sizeof(char));
> if (!attrName) return false;
Wrap this malloc as:
nsAutoArrayPtr<char> attrName = malloc();

> 
> Also, 
> if (!numActiveAttrs) return false;
> 
> I assume if numActiveAttrs is still 0 after the call to fGetProgramiv, then
> the call has actually failed, and we should inform the caller. Or could
> numActiveAttrs be 0 and valid?
Active attribs should be able to be zero. It's possible to render points using uniforms with no attribs. Once we get instanced rendering, a GLSL prim id would basically allow for useful no-attrib drawing.

Comment 20

[Mina Almasry]

Okay ignore the `if (!numActiveAttrs) return false;` in the review then, please. I'll come up with a patch without that and with nsAutoArrayPtr<char> attrName shortly.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 794446 [details] [diff] [review]
Patch v2

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

Just about there. Remember to try to use GLint and friends when we're going to be feeding the value into GL, so we can match the type.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +87,5 @@
>      }
>  
> +    GLint numActiveAttrs = 0;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &numActiveAttrs);
> +    if (!numActiveAttrs) {

Zero should be a valid number of active attribs.

@@ +95,5 @@
> +    GLint maxNameSize = 257;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);
> +    GLsizei bufSize = maxNameSize + 1;
> +    char* attrName = (char*) malloc(bufSize * sizeof(char));

Use nsAutoArrayPtr<char> to hold this to scope instead of doing deallocation manually.

@@ +96,5 @@
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);
> +    GLsizei bufSize = maxNameSize + 1;
> +    char* attrName = (char*) malloc(bufSize * sizeof(char));
> +    if (!attrName) {

Malloc is infallible, and we're not asking for enough memory to worry about doing a fallible allocation. Just assume this succeeded.

@@ +102,5 @@
> +    }
> +
> +    GLint dummySize;
> +    GLenum dummyType;
> +    for (int32_t i = 0; i < numActiveAttrs; i++) {

Stick to GL's types when we're going to be using it with GL, instead of using stdint types.

@@ +106,5 @@
> +    for (int32_t i = 0; i < numActiveAttrs; i++) {
> +        mContext->gl->fGetActiveAttrib(mGLName, i, bufSize, nullptr, &dummySize,
> +                                       &dummyType, attrName);
> +        GLint attrLoc = mContext->gl->fGetAttribLocation(mGLName, attrName);
> +        mActiveAttribMap.insert(std::make_pair(attrLoc, attrName));

We need to clear mActiveAttribMap before we fill it in this loop.

Comment 22

[Mina Almasry]

Attachment: bindattr.patch

Here are the changes. Try push: https://tbpl.mozilla.org/?tree=Try&rev=fad9ad2f1e19

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 794882 [details] [diff] [review]
bindattr.patch

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

This is fine if you want to leave it as is.

If you want to take my recommendation, just ask for review on that new revision, but let's not carry the r+ forward without checking it.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +90,5 @@
> +
> +    GLint numActiveAttrs = 0;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &numActiveAttrs);
> +
> +    GLint maxNameSize = 257;

256 is the max size of an ident, so we can use that as the default, and make a note that it comes from the WebGL spec section 6.18.
If fact, we could just use this unconditionally, since we don't really care if we temporarily allocate a quarter kilobyte.

@@ +92,5 @@
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &numActiveAttrs);
> +
> +    GLint maxNameSize = 257;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);

We could query this, and assert that it's <= 256 if we wanted in DEBUG mode, but as stated above, I now realize this shouldn't be strictly necessary.

@@ +94,5 @@
> +    GLint maxNameSize = 257;
> +    mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH,
> +                                &maxNameSize);
> +    GLsizei bufSize = maxNameSize + 1;
> +    nsAutoArrayPtr<char> attrName((char*) malloc(bufSize * sizeof(char)));

Sorry for the churn, but let's just use:
char attrName[256+1]; // GLSL tokens are at most 256 chars, as per WebGL spec section 6.18.

Comment 24

[Mina Almasry]

Attachment: Patch v3

here is the change. I need a review anyway because I had to make small changes to make it compile on all platforms too.

Comment 25

[Mina Almasry]

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

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 795090 [details] [diff] [review]
Patch v3

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

It would be nice if you fixed these nits, but this is good.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2274,5 @@
> +    if (mBoundVertexArray->mAttribBuffers[0].enabled) {
> +        return false;
> +    }
> +
> +    int32_t leastArrayLocation = -1;

To match the type in mActiveAttribMap, this should be GLint.

@@ +2339,1 @@
>      GLint ok;

Let's be safe and give these defaults of `false` and `0`.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +99,5 @@
> +    GLenum dummyType;
> +    for (GLint i = 0; i < numActiveAttrs; i++) {
> +        mContext->gl->fGetActiveAttrib(mGLName, i, 257, nullptr, &dummySize,
> +                                       &dummyType, attrName);
> +        GLint attrLoc = mContext->gl->fGetAttribLocation(mGLName, attrName);

We should assert that attrLoc is >=0.

Comment 27

[Mina Almasry]

Attachment: Patch v4

nits addressed.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 795648 [details] [diff] [review]
Patch v4

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

This is fine.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2280,5 @@
> +    std::map<GLint, nsCString>::iterator itr;
> +    for (itr = program->mActiveAttribMap.begin();
> +         itr != program->mActiveAttribMap.end();
> +         itr++) {
> +        int32_t index = itr->first;

Type of `itr->first` is GLint.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00210be9d9eb

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00210be9d9eb