Fix API calls to WebGL where an optional size parameter is zero bytes#16837
Merged
juj merged 1 commit intoemscripten-core:mainfrom Apr 28, 2022
Merged
Fix API calls to WebGL where an optional size parameter is zero bytes#16837juj merged 1 commit intoemscripten-core:mainfrom
juj merged 1 commit intoemscripten-core:mainfrom
Conversation
…, which would upload the whole WebAssembly heap since WebGL has a different semantics when zero size is passed. Fixes emscripten-core#16799.
Collaborator
Author
|
We don't have tests for this, so recommend a careful review. |
kainino0x
approved these changes
Apr 28, 2022
Collaborator
kainino0x
left a comment
kainino0x
left a comment
There was a problem hiding this comment.
Nice catch, and good example of sentinel 0 values being a footgun. Glad we removed them in WebGPU.
sbc100
reviewed
Apr 28, 2022
| assert(GL.currentContext.version >= 2); | ||
| #endif | ||
| GLctx.uniform1iv(webglGetUniformLocation(location), HEAP32, value>>2, count); | ||
| count && GLctx.uniform1iv(webglGetUniformLocation(location), HEAP32, value>>2, count); |
Collaborator
There was a problem hiding this comment.
Is this just another way of writing if (count) .... but saving two bytes?
Seems a little odd to do the micro-optimization here but not globally across the codebase.
Collaborator
Author
There was a problem hiding this comment.
Is this just another way of writing if (count) .... but saving two bytes?
That's right.
Seems a little odd to do the micro-optimization here but not globally across the codebase.
Maybe. It is used consistently in WebGL library, and that is probably more on the hot code size path that some of the other libraries.
Collaborator
There was a problem hiding this comment.
Ah, ok. I didn't realize there was precedent for this.
Member
There was a problem hiding this comment.
I think closure does this? If not, it would be easy to add a js optimizer pass. I think depending on the optimizer for this kind of thing is probably best (keeps code more readable and the optimization applies to more places).
Collaborator
|
It seems odd that we don't have tests for any of this .. are they hard to write? |
Collaborator
Author
There are 26 API entry points that are affected, which is a lot of tests to write. Writing tests that check that the whole Wasm heap is not being serialized probably would require some kind of mocking/polyfilling on the ArrayBuffer. |
This was referenced Mar 20, 2024
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 20, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. For example the following line was unguarded: ``` var view = miniTempWebGLFloatBuffers[4 * count - 1]; ``` This recently resurfaced because I introduced `WEBGL_USE_GARBAGE_FREE_APIS` which is currently disabled for memories larger 2gb. This meant that users with large memories were forces onto the old path where the bug still existed. Rather than adding yet more `count &&` prefixes, this patch simply adds a single early return at the top of each function. Fixes: emscripten-core#21567
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 20, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. For example the following line was unguarded: ``` var view = miniTempWebGLFloatBuffers[4 * count - 1]; ``` This recently resurfaced because I introduced `WEBGL_USE_GARBAGE_FREE_APIS` which is currently disabled for memories larger 2gb. This meant that users with large memories were forces onto the old path where the bug still existed. Rather than adding yet more `count &&` prefixes, this patch simply adds a single early return at the top of each function. Fixes: emscripten-core#21567
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 20, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. For example the following line was unguarded: ``` var view = miniTempWebGLFloatBuffers[4 * count - 1]; ``` This recently resurfaced because I introduced `WEBGL_USE_GARBAGE_FREE_APIS` which is currently disabled for memories larger 2gb. This meant that users with large memories were forces onto the old path where the bug still existed. Rather than adding yet more `count &&` prefixes, this patch simply adds a single early return at the top of each function. Fixes: emscripten-core#21567
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 20, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. For example the following line was unguarded: ``` var view = miniTempWebGLFloatBuffers[4 * count - 1]; ``` This recently resurfaced because I introduced `WEBGL_USE_GARBAGE_FREE_APIS` which is currently disabled for memories larger 2gb. This meant that users with large memories were forces onto the old path where the bug still existed. Rather than adding yet more `count &&` prefixes, this patch simply adds a single early return at the top of each function. As part of this change I resurrected `test_webgl_draw_triangle_with_uniform_color.c` which has not actually be used since emscripten-core#20925. Fixes: emscripten-core#21567
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 21, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. As part of this change I resurrected `test_webgl_draw_triangle_with_uniform_color.c` which has not actually be used since emscripten-core#20925. Fixes: emscripten-core#21567
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Mar 21, 2024
The previous fix for this was in emscripten-core#16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. As part of this change I resurrected `test_webgl_draw_triangle_with_uniform_color.c` which has not actually be used since emscripten-core#20925. Fixes: emscripten-core#21567
sbc100
added a commit
that referenced
this pull request
Mar 26, 2024
The previous fix for this was in #16837, but I looks like that only covered the new "garbage-free" webgl2 path. When old webgl1 path was still using the zero count value. For example the following line was unguarded: ``` var view = miniTempWebGLFloatBuffers[4 * count - 1]; ``` This recently resurfaced because I introduced `WEBGL_USE_GARBAGE_FREE_APIS` which is currently disabled for memories larger 2gb. This meant that users with large memories were forces onto the old path where the bug still existed. Rather than adding yet more `count &&` prefixes, this patch simply adds a single early return at the top of each function. As part of this change I resurrected `test_webgl_draw_triangle_with_uniform_color.c` which has not actually be used since #20925. Fixes: #21567
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix API calls to WebGL where an optional size parameter is zero bytes, which would upload the whole WebAssembly heap since WebGL has a different semantics when zero size is passed. Fixes #16799.