Fix calls to glCcompressedTexImage2D with null pixel data #22453
Conversation
| @@ -115,7 +115,12 @@ int main(int argc, char *argv[]) | |||
| while (level < 5) { | |||
| printf("uploading level %d: %d, %d\n", level, w, h); | |||
| assert(!glGetError()); | |||
| #if TEST_TEXSUBIMAGE | |||
| glCompressedTexImage2D(GL_TEXTURE_2D, level, GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, w, h, 0, w*h, NULL); | |||
There was a problem hiding this comment.
https://community.khronos.org/t/glcompressedteximage2d-and-null-data/41505
IIUC this link is saying you can call glTexImage2D with a null pointer to allocate the texture, then glCompressedTexSubImage2D to initialize it.
There was a problem hiding this comment.
According #19300, GLES users expect to be able to call glCompressedTexImage2D with the null final argument.
So I think the test code here should use what the existing GLES code expects to be able to do.
There was a problem hiding this comment.
IIUC this link is saying you can call
glTexImage2Dwith a null pointer to allocate the texture, thengl_Compressed_TexSubImage2Dto initialize it.
I am not sure if that is how the spec is supposed to work. Calling (Compressed)TexSubImage will not change a type of the texture that was previously created with a call to (Compressed)TexImage?
I tried with the following test code:
<!DOCTYPE html><html><body><script>
var gl = document.createElement("canvas").getContext('webgl');
gl.bindTexture(gl.TEXTURE_2D, gl.createTexture());
var dxt1 = gl.getExtension('WEBGL_compressed_texture_s3tc').COMPRESSED_RGB_S3TC_DXT1_EXT;
var w = 1024;
gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, w, w, 0, gl.RGBA, gl.UNSIGNED_BYTE, null);
var foo = new Uint8Array(w*w/2);
gl.compressedTexSubImage2D(gl.TEXTURE_2D, 0, 0, 0, w, w, dxt1, foo);
</script></body></html>which gives a warning in Firefox:
that suggests the same.
src/library_webgl.js
Outdated
| @@ -1520,6 +1520,11 @@ for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; | |||
| }, | |||
|
|
|||
| glCompressedTexImage2D: (target, level, internalFormat, width, height, border, imageSize, data) => { | |||
| // `data` may be null here, which means "allocate space but don't upload" in | |||
There was a problem hiding this comment.
My understanding of the conversation on the issue was that if data is null, that's literally just telling GL to read from the null pointer, it has no actual meaning. (glTexImage2D special-cases null, but glCompressedTexImage2D does not.) So seems to me like we should have a debug assert that data isn't null.
There was a problem hiding this comment.
IIUC, data is allowed to be null here (i.e. in the GLES C API), and the (in GLES) that means "just allocate some space". It certainly doesn't mean "read from address zero". since that would instantly crash, right?
I think its just an oversight in the GLES man page that it doesn't document the null special case. See #19300 (comment):
"
If the data argument of CompressedTexImage1D, CompressedTexImage2D, or CompressedTexImage3D is NULL, and the pixel unpack buffer object is zero, a texture image with unspecified image contents is created, just as when a NULL pointer is passed to TexImage1D, TexImage2D, or TexImage3D.
"
There was a problem hiding this comment.
In GLES3, data is allowed to be null. In GLES2 the specification missed clarifying whether data is allowed to be null or not. KhronosGroup/WebGL#3686
There was a problem hiding this comment.
Ahh, I didn't read far enough in the GLES3 spec because it was buried way at the bottom under the table!
There was a problem hiding this comment.
a little clarification:
| // `data` may be null here, which means "allocate space but don't upload" in | |
| // `data` may be null here, which means "allocate uninitialized space but don't upload" in |
|
@kenrussell, can you confirm this is the solution you recommend? |
src/library_webgl.js
Outdated
| @@ -1520,6 +1520,11 @@ for (/**@suppress{duplicate}*/var i = 0; i <= {{{ GL_POOL_TEMP_BUFFERS_SIZE }}}; | |||
| }, | |||
|
|
|||
| glCompressedTexImage2D: (target, level, internalFormat, width, height, border, imageSize, data) => { | |||
| // `data` may be null here, which means "allocate space but don't upload" in | |||
There was a problem hiding this comment.
a little clarification:
| // `data` may be null here, which means "allocate space but don't upload" in | |
| // `data` may be null here, which means "allocate uninitialized space but don't upload" in |
I believe this has always been an issue with emscripten's WebGL1 code which only recently became more prevalent with emscripten-core#21445. Fixes: emscripten-core#19300
sbc100
force-pushed
the
glCcompressedTexImage2D
branch
from
e85b9f8 to
2b6781e
Compare
I believe this has always been an issue with emscripten's WebGL1 code which only recently became more prevalent with #21445.
Fixes: #19300