webgl: populate uniformLocsById on first glUniform#26846
Conversation
$webglGetUniformLocation reads program.uniformLocsById to resolve a uniform location integer to a WebGLUniformLocation. uniformLocsById is reset to 0 by glLinkProgram and lazily allocated by $webglPrepareUniformLocationsBeforeFirstUse, but that prepare call only ran on the glGetUniformLocation() path. As a result, calling glUniform*() on a freshly linked program before any glGetUniformLocation() — which is the intended usage with -sGL_EXPLICIT_UNIFORM_LOCATION and explicit layout(location=N) — silently no-opped: $webglGetUniformLocation read uniformLocsById[location] off the integer 0, returned undefined, and GLctx.uniform*() dropped the call with no error. Fix: call $webglPrepareUniformLocationsBeforeFirstUse(p) at the top of $webglGetUniformLocation, inside the GL_TRACK_ERRORS guard. The prepare call is idempotent (guards on !uniformLocsById), so the steady-state overhead is a single property read. Adds browser.test_webgl_uniform_before_get_location with two parameterizations (default and -sGL_ASSERTIONS).
| // `glGetUniformLocation()` silently no-ops: `glLinkProgram` resets | ||
| // `uniformLocsById` to 0 and only `$webglPrepareUniformLocationsBeforeFirstUse` | ||
| // refills it. The call below is idempotent (guards on `!uniformLocsById`). | ||
| webglPrepareUniformLocationsBeforeFirstUse(p); |
There was a problem hiding this comment.
Can you gate this block with
#if GL_EXPLICIT_UNIFORM_LOCATION
// ...
webglPrepareUniformLocationsBeforeFirstUse(p);
#endif
so that WebGL programs that do not use the explicit uniform location extension, won't need to take this micro-perf-hit.
There was a problem hiding this comment.
Makes sense, will do.
| emscripten_webgl_make_context_current(ctx); | ||
|
|
||
| const char* vs = "#version 300 es\n" | ||
| "layout(location = 0) uniform vec3 u_val;\n" |
There was a problem hiding this comment.
Maybe here we might want to test layout(location = 6) or something else arbitrary, since default 0 could theoretically pass by the virtue of "the only uniform gets assigned an ID 0" kind of a behavior?
There was a problem hiding this comment.
Makes sense, will do.
|
Thanks. Looks solid, one comment on the perf/code size aspect of the feature. |
|
@juj: addressed both comments, thanks for the review. |
|
@juj: any idea why the CI is failing? The tests pass when I try them locally via |
It is the you should see the failure. This error is a real issue with the ASSERTIONS logic. The assertion code is too early with respect to the lazily initialized uniforms code, when To fix, it's probably simplest to add the following diff: diff --git a/src/lib/libwebgl.js b/src/lib/libwebgl.js
index 549450fc92..a127f37d27 100644
--- a/src/lib/libwebgl.js
+++ b/src/lib/libwebgl.js
@@ -603,7 +603,9 @@ for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}};
#if GL_ASSERTIONS
validateGLObjectID: (objectHandleArray, objectID, callerFunctionName, objectReadableType) => {
- if (objectID != 0) {
+ // objectHandleArray may be uninitialized when GL uniforms are lazily initialized, and glUniform* is called
+ // for the first time before uniforms have been populated. So ignore this validation if the handle array is not present.
+ if (objectID != 0 && objectHandleArray) {
if (objectHandleArray[objectID] === null) {
err(`${callerFunctionName} called with an already deleted ${objectReadableType} ID ${objectID}!`);
} else if (!(objectID in objectHandleArray)) {because it looks like refactoring the assertions here so that all the |
Head branch was pushed to by a user without write access
|
@juj: I see, my apologies. I assumed that I applied your suggested diff to the PR. |
|
Great, thanks! |
PR created with AI assistance.
$webglGetUniformLocationreadsprogram.uniformLocsByIdto resolve a uniform location integer to aWebGLUniformLocation.uniformLocsByIdis reset to0byglLinkProgramand lazily allocated by$webglPrepareUniformLocationsBeforeFirstUse, but that prepare call only ran on theglGetUniformLocation()path. As a result, callingglUniform*()on a freshly linked program before anyglGetUniformLocation()— which is the intended usage with-sGL_EXPLICIT_UNIFORM_LOCATIONandexplicit layout(location=N)— silently no-opped:$webglGetUniformLocationreaduniformLocsById[location]off the integer0, returnedundefined, andGLctx.uniform*()dropped the call with no error.Fix: call
$webglPrepareUniformLocationsBeforeFirstUse(p)at the top of$webglGetUniformLocation, inside theGL_TRACK_ERRORSguard. The prepare call is idempotent (guards on!uniformLocsById), so the steady-state overhead is a single property read.Adds
browser.test_webgl_uniform_before_get_locationwith two parameterizations (default and-sGL_ASSERTIONS).Fixes: #26672