Skip to content

Fix a crash in miniTempWebGL*Buffers optimization #22130

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Fix a crash in miniTempWebGL*Buffers optimization #22130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
520be16
Fix a crash in miniTempWebGL*Buffers optimization
kainino0x Jun 22, 2024
b31a7cf
add second comment
kainino0x Jun 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/library_webgl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ var LibraryGL = {

$miniTempWebGLFloatBuffers: [],
$miniTempWebGLFloatBuffers__postset: `var miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}});
for (/**@suppress{duplicate}*/var i = 0; i < {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) {
// Create GL_POOL_TEMP_BUFFERS_SIZE+1 temporary buffers, for uploads of size 0 through GL_POOL_TEMP_BUFFERS_SIZE inclusive
for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was the one who broke this in #21573?

Could we instead update all the other places where GL_POOL_TEMP_BUFFERS_SIZE to use less than? e.g. all the places there we currently do if (count <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this but decided it didn't make as much sense. Then the top end would be unused for vec2/vec3/vec4 - the maximum values that hit the optimization would be:

  • 287 floats
  • 143 vec2s (286 floats)
  • 95 vec3s (285 floats)
  • 71 vec4s (284 floats)

Also
miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}});
would change to
miniTempWebGLFloatBuffersStorage = new Float32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}} - 1);

miniTempWebGLFloatBuffers[i] = miniTempWebGLFloatBuffersStorage.subarray(0, i);
}`,

$miniTempWebGLIntBuffers: [],
$miniTempWebGLIntBuffers__postset: `var miniTempWebGLIntBuffersStorage = new Int32Array({{{ GL_POOL_TEMP_BUFFERS_SIZE }}});
for (/**@suppress{duplicate}*/var i = 0; i < {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) {
// Create GL_POOL_TEMP_BUFFERS_SIZE+1 temporary buffers, for uploads of size 0 through GL_POOL_TEMP_BUFFERS_SIZE inclusive
for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; ++i) {
miniTempWebGLIntBuffers[i] = miniTempWebGLIntBuffersStorage.subarray(0, i);
}`,

Expand Down
46 changes: 45 additions & 1 deletion test/browser/webgl_draw_triangle_with_uniform_color.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,56 @@ int main() {
glEnableVertexAttribArray(0);
glEnableVertexAttribArray(1);

// Spot-test for miniTempWebGLFloatBuffers optimization in library_webgl.js.
// Note there WILL be GL errors from this, but the test doesn't check for them
// so it won't cause it to fail.
#define GL_POOL_TEMP_BUFFERS_SIZE 288
{
GLfloat fdata[GL_POOL_TEMP_BUFFERS_SIZE + 4] = {};
// Just under the optimization limit (should use unoptimized codepath)
glUniform4fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4 - 1, fdata);
glUniform2fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2 - 1, fdata);
glUniform1fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE - 1, fdata);
// Just at the optimization limit (should use optimized codepath)
glUniform4fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4, fdata);
glUniform2fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2, fdata);
glUniform1fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE, fdata);
// Just over the optimization limit (should use optimized codepath)
glUniform4fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4 + 1, fdata);
glUniform2fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2 + 1, fdata);
glUniform1fv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE + 1, fdata);

GLint idata[GL_POOL_TEMP_BUFFERS_SIZE + 4] = {};
// Just under the optimization limit (should use unoptimized codepath)
glUniform4iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4 - 1, idata);
glUniform2iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2 - 1, idata);
glUniform1iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE - 1, idata);
// Just at the optimization limit (should use optimized codepath)
glUniform4iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4, idata);
glUniform2iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2, idata);
glUniform1iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE, idata);
// Just over the optimization limit (should use optimized codepath)
glUniform4iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 4 + 1, idata);
glUniform2iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE / 2 + 1, idata);
glUniform1iv(glGetUniformLocation(program, "color2"), GL_POOL_TEMP_BUFFERS_SIZE + 1, idata);
}

// Actual upload for the rest of the test (overwrites the previous one).
float color2[4] = { 0.0f, 1.f, 0.0f, 1.0f };
glUniform4fv(glGetUniformLocation(program, "color2"), 1, color2);

// Test that passing zero for the size paramater does not cause error
// https://github.com/emscripten-core/emscripten/issues/21567
glUniform4fv(glGetUniformLocation(program, "color2"), 0, color2);
// (These are zero-sized, so shouldn't overwrite anything.)
{
GLfloat fdata[4] = {};
glUniform4fv(glGetUniformLocation(program, "color2"), 0, fdata);
glUniform4fv(glGetUniformLocation(program, "color2"), 0, NULL);

GLint idata[4] = {};
glUniform4iv(glGetUniformLocation(program, "color2"), 0, idata);
glUniform4iv(glGetUniformLocation(program, "color2"), 0, NULL);
}

glClearColor(0.3f,0.3f,0.3f,1);
glClear(GL_COLOR_BUFFER_BIT);
Expand Down
Loading