Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused vertex attributes and gl.enable/disableVertexAttribArray? #2986

Closed
juj opened this issue Dec 9, 2019 · 6 comments
Closed

Unused vertex attributes and gl.enable/disableVertexAttribArray? #2986

juj opened this issue Dec 9, 2019 · 6 comments

Comments

@juj
Copy link
Contributor

juj commented Dec 9, 2019

Consider a simple vertex shader:

attribute vec3 pos;
attribute vec3 inColor;
varying vec3 outColor;

void main(void) {
	gl_Position = vec4(pos, 1.0);
	outColor = inColor;
}

Recall that in OpenGL ES 2.0 spec, if one wants to source pos attribute data using a constant vector, one should call

var posAttributeIndex = gl.getAttribLocation(shaderProgram, "pos");
gl.disableVertexAttribArray(posAttributeIndex);
gl.vertexAttrib3f(posAttributeIndex, 1.0, 2.0, 3.0);

Whereas if one wants to source pos attribute data using an array, one would call

var posAttributeIndex = gl.getAttribLocation(shaderProgram, "pos");
gl.enableVertexAttribArray(posAttributeIndex);
gl.bindBuffer(gl.ARRAY_BUFFER, vertexBufferObject);
gl.vertexAttribPointer(posAttributeIndex, 3, gl.FLOAT, false, 0, 0); 

In the second example, the call gl.vertexAttribPointer() effectively binds the VBO vertexBufferObject to the given attribute index posAttributeIndex.

As per spec, calling gl.disableVertexAttribArray(posAttributeIndex) will not undo this VBO binding made by gl.vertexAttribPointer(). Conversely, calling gl.enableVertexAttribArray(posAttributeIndex) will not undo the state call set by gl.vertexAttrib3f(posAttributeIndex, 1.f, 2.f, 3.f). That is, the attrib array enabled vs disabled bit is orthogonal to the constant attribute vector value and the VBO + attrib pointer state binding set by those calls. I.e. both set of state exist and persist on the GL server side, regardless of the enabled/disabled state of the vertex attrib array index.

Hence, if one renders with an attribute array enabled for attribute pos, the constant attribute vector state (set by gl.vertexAttrib3f) is simply ignored (but it exists and is still there). Conversely, if one renders with an attribute array disabled for pos, the attribute array state for pos (set by gl.vertexAttribPointer) is simply ignored (but it still exists and is there).

Makes sense.

Now, what should happen if

a) one enables attribute array for an attribute index not used by the vertex shader, and that index was populated with some VBO contents. (or, more likely, it was left over populated from a previous draw call)

b) one enables attribute array for an attribute index not used by the vertex shader, and that attribute index does not actually have an VBO bound to it?

A common sense approach could say:

If a vertex shader does not source an input from an attribute index X,
whatever state is set to that attribute index is simply ignored. I.e.
whether attribute array is enabled or disabled for that index is ignored,
and the attribute constant vector value for that attribute index is ignored,
and the attribute array data/VBO bound to that attribute index is ignored.

That seems like the most logical way to behave, after all, if a particular attribute index is not used by a vertex shader, then whatever state one sets there cannot have an effect to a draw call.

But testing this out in practice in Firefox and Chrome on Windows, that does not happen. For example enabling all attributes to be sourced from attribute arrays instead of attribute constant vectors, via calling

for(var i = 0; i < gl.getParameter(gl.MAX_VERTEX_ATTRIBS); ++i) {
   gl.enableVertexAttribArray(i);
}

results in both browsers complaining. Chrome gives INVALID_OPERATION: drawArrays: no buffer is bound to enabled attribute, and Firefox gives drawArrays: Vertex attrib array 2 is enabled but has no buffer bound.

Test it online

The error message in Chrome is (subtly) incorrect, and may be stating a misunderstanding of this functionality. Chrome is stating that no buffer is bound to an enabled attribute. However that is not the case, but in the demo page above, all enabled attributes (pos and inColor) do have a buffer bound to them. It is as if the error message of Chrome implementation has an impression that calling gl.enableVertexAttribArray(i) would enable an attribute at index i, whereas that is not correct. As per spec, the function gl.enableVertexAttribArray(i) does not enable an attribute at index i, but it enables the attribute at index i to be sourced via an attribute array buffer, instead of sourcing it via a constant vector from a call to gl.vertexAttrib*f().

There is no function in GL to enable or disable an attribute, but the source attribute data specified by a vertex shader dictates which attribute indices are enabled or disabled.

The Firefox message is however more correct, although a bit useless in spirit. It says that vertex attrib array 2 is enabled but has no buffer bound. That is true, but, the vertex shader is not even using a vertex attribute at index 2, i.e. that attribute index is disabled - so why should the state vector of that vertex attribute index matter?

(one might argue that is because the validation/checking is static rather than dynamic, i.e. the code that checks the validity of the attribute indices cannot be enforced to need to know what attribute indices are actually in use by the current draw, but that seems a bit hand-wavy, even performance-wise?)

In practical scenarios, if a program only uses VBOs, it can be the case that unused attribute indices will have some unrelated VBOs bound to them from prior draws. If a vertex shader is not using some attribute indices, what are the performance characteristics of leaving bound VBOs to unused attribute indices? Common sense would say that since these attribute indices are not used by a vertex shader, whatever is set in them when drawing with that particular shader should just get ignored (both validation, but also performance wise) E.g. what happens if those unused VBOs are smaller than the size of the draw call? (common sense would say nothing should happen since this is ignored state anyways)

But it seems that implementations in Firefox and Chrome do not agree with the GLES2 spec interpretation of how glEnableVertexAttribArray() and glDisableVertexAttribArray() should work, and they both behave as if glDisableVertexAttribArray() disabled an attribute index (which it does not), and hence insist on validating the state of unused attribute indices. This is seen in for example bkaradzic/bgfx#1512 and bkaradzic/bgfx#1515.

The reason why this is interesting and important aspect is that repeatedly calling these two functions is very costly. In one WebAssembly+WebGL heavy page on Firefox there is a performance hit of around -10%, and in Chrome there is a performance hit of around -2.5% (profiled against an engine that implements a naive uncached algorithm to enable/disable attribute arrays: bkaradzic/bgfx#1960). In UE4 and Unity engines, I authored state-cached enable/disable attrib arrays using "pending" bitfields, which gives less perf overhead, but still noticeably high in profiles. For WebGL applications it would be beneficial for an application to be able to call

for(var i = 0; i < gl.getParameter(gl.MAX_VERTEX_ATTRIBS); ++i) {
   gl.enableVertexAttribArray(i);
}

up front to essentially say "in my application I will source all my vertex attributes from attribute arrays instead of constant attribute vectors". This would avoid needing to flip these bits back and forth to appease validation (so few engines even use the attribute constant vector feature).

After all, as a vertex shader already knows what attribute indices it uses, why should the CPU side need to repeat telling information to the GL that it already knows?

The GLES spec might be more clear if it instead would have had

enum VertexAttribSource {
  VertexAttribSourceFromConstantVector,
  VertexAttribSourceFromVertexArray
};
glSetVertexAttribSource(int index, VertexAttribSource source);

and then gl.enableVertexAttribArray(i) would be equivalent to calling glSetVertexAttribSource(i, VertexAttribSourceFromVertexArray), and gl.disableVertexAttribArray(i) would be equivalent to calling glSetVertexAttribSource(i, VertexAttribSourceFromConstantVector). That would have conveyed the meaning of this functionality better?

Does this interpretation seem appropriate? (or have I been grossly misinterpreting, greatest apologies if so?) Would it be possible for Chrome and Firefox to stop validating attribute indices that are not actually in use? (I imagine they currently loop through to validate all supported attribute indices on each draw call, which is not free either?)

@kainino0x
Copy link
Contributor

On first read I think you're probably right about this - in particular if native ES drivers exhibit the behavior you propose.

I dug up the Chrome change where these error messages were added, and it points to this spec section and conformance2/vertex_arrays/vertex-array-object.html. It's possible that Chrome and Firefox just have consistent but incorrect behavior because our tests test it.

It's likely we don't trust some drivers to behave correctly (not crash, mainly) when unused attributes are bound to arrays of the wrong size. But that state-cached pending bitfield method is something we could apply as a workaround under the hood in the browsers.

@juj
Copy link
Contributor Author

juj commented Dec 10, 2019

Thanks,

https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.6

answers question a) directly:

If a vertex attribute is enabled as an array, a buffer is bound to that attribute, but the attribute is not consumed by the current program, then regardless of the size of the bound buffer, it will not cause any error to be generated during a call to drawArrays or drawElements. 

That is, if attribute is set to be sourced via an array, and there is at least some VBO bound to it (rather than null), then no errors should happen. (The wording does not state that there could not occur a silly performance impact overhead from some kind of managing/bookkeeping for these unused bound attribute arrays - but specs rather specify anything about performance overhead, so wouldn't expect it to say anything)

That basically leaves question b) - letting applications render with an unused attribute set to source from array, and not having an array bound. As a workaround, applications can rely on binding dummy zero-sized arrays, e.g. at start of program:

gl.bindBuffer(gl.ARRAY_BUFFER, gl.createBuffer());
for(var i = 0; i < gl.getParameter(gl.MAX_VERTEX_ATTRIBS); ++i) {
  gl.enableVertexAttribArray(i);
  gl.vertexAttribPointer(i, 1, gl.FLOAT, false, 0, 0);
}

webgl_arrays_enabled_with_dummy_buffers_bound.html

That does work in Chrome and Firefox (although it does make me a bit nervous, since there are no performance guarantees on how browsers will treat such code)

Btw, there does also exist instancing attribute divisor state in WebGL 2. If not doing instancing, whatever divisor state is set to unused arrays should also be ignored. That seems to be the case:

webgl2_arrays_unused_attributes_enable_instancing.html

@kenrussell
Copy link
Member

In the underlying OpenGL specification, glVertexAttribPointer can cause the GL to fetch either client-side (CPU-side) or server-side (GPU-side) data for subsequent draw calls. If a buffer is bound at the time the call is made, it fetches GPU-side data for that attribute; otherwise, client-side. WebGL necessarily only allows operating upon GPU-side data.

In OpenGL, calling glEnableVertexAttribArray(true) when client-side data is being used for that attribute - including the case where the attribute is uninitialized, so is effectively pointing to address 0x0 - and then issuing a draw call will crash, even if that attribute isn't consumed by the program. At least, it used to on drivers that I tested on years ago. I'm not sure that WebGL implementations should guarantee that this scenario doesn't crash. We could try adding such code to the browser and seeing whether it impacts performance of common cases.

Binding zero-sized buffers to all vertex attributes as a workaround would be suboptimal. The browser has to explicitly disable those vertex attributes as arrays because some OpenGL implementations do fetch from those buffers even if they're unused by the current program. See http://crbug.com/756293 and http://crbug.com/740278 .

I think the best approach would be for the application layer to call enable/disableVertexAttribArray intelligently when switching programs. Is that a reasonable solution?

@greggman
Copy link
Contributor

greggman commented Jan 13, 2020

I'm a little confused. If you're worried about perf, why are you not using vertex arrays? There should be no thrashing of enabling or disabling attributes at render time just gl.bindVertexArray or the equiv. Vertex arrays are available on pretty much all devices. Those < 1% of devices for which it's not supported in WebGL1 are likely devices too slow to run your high per app though if you want to handle them there anyway there is a polyfill

Also, FYI, not sure it's still true and of course there are no guarantees but at one point Chrome tracked the highest attribute touched which means it knows up front that it only needs to check 0 to highest ever used attribute vs having to check 0 to maxNumAttributes. I know that of course a different impl could do different things, just pointing out that you probably don't want to go just bindly do something to all attributes.

@vvuk
Copy link

vvuk commented Jan 14, 2020

I vaguely remember writing that validation in the Firefox impl, and I think the reason was that pre-ANGLE, talking directly to a GL driver, there was a driver that actually caused a crash in that case (array enabled, no buffer bound, but unused). If I'm remembering all this right, the alternative was to bind a dummy buffer but that required a lot more tracking.

@kenrussell
Copy link
Member

@juj discussed this with Chrome's WebGL team during a recent meeting and agreed that this issue can be closed. The application layer should be upgraded to use VBOs, to efficiently disable unused vertex attributes as arrays, or some other solution. Please reopen this or file another issue if this turns out to be impractical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants