opengl support for memory64 #18744

PhilippMuigg · 2023-02-14T08:30:07Z

@sbc100, this is a second try at getting the opengl layer of emscripten to work when the MEMORY64 flag is used.
Since i worked on this as an employee of Siemens i was advised to add some copyright comments into file headers that were modified.

@juj i have incorporated some of the changes from #15433 into this pull request and i will wait for you finishing that up before considering merging.

emcc.py

src/library_egl.js

src/library_html5.js

sbc100 · 2023-05-13T02:18:01Z

src/library_html5.js

@@ -651,7 +657,7 @@ var LibraryHTML5 = {
      {{{ makeSetValue('JSEvents.wheelEvent', C_STRUCTS.EmscriptenWheelEvent.deltaY, 'wheelDeltaY', 'double') }}};
      {{{ makeSetValue('JSEvents.wheelEvent', C_STRUCTS.EmscriptenWheelEvent.deltaZ, '0 /* Not available */', 'double') }}};
      {{{ makeSetValue('JSEvents.wheelEvent', C_STRUCTS.EmscriptenWheelEvent.deltaMode, '0 /* DOM_DELTA_PIXEL */', 'i32') }}};
-      var shouldCancel = {{{ makeDynCall('iiii', 'callbackfunc') }}}( eventTypeId, JSEvents.wheelEvent, userData);
+      var shouldCancel = {{{ makeDynCall('iipp', 'callbackfunc') }}}( eventTypeId, JSEvents.wheelEvent, userData);


These changes look good. I wonder is we could figure out a test that uses these in test_browser.py and add also_with_wasm64 the that test? Then we could that this change in isolation?

It seems like the existing tests in test-windows test-mac and test-other are failing due to some of these changes. And right, i can take a quick look and see if this can be added to a separate commit/pull request.

sbc100 · 2023-05-13T02:18:48Z

src/library_strings.js

@@ -157,6 +157,9 @@ mergeInto(LibraryManager.library, {
 #if CAN_ADDRESS_2GB
    outIdx >>>= 0;
 #endif
+#if MEMORY64
+  outIdx = Number(outIdx);


This should not bee needed outIdx should already be a number by the time it gets here. Can you find the backtrace that leads here without outIdx being converted to an number already?

Looks like this is now also working without the explicit conversion. Will remove it.

sbc100 · 2023-05-13T02:21:36Z

src/parseTools.js

+  } else {
+    const mod = size == 1 ? '' : ('>>' + Math.log2(size));
+    return `HEAP${which}.subarray((${start})${mod}, (${end})${mod})`;
+  }


The last line of each branch here looks the same, right? Can we simplify here to avoid duplication? e.g.:

if (MEMORY64) { mod = .. } else { mod = .. } return `HEAP${which}.subarray((${start})${mod}, (${end})${mod})`;

Ok, will make this change.

pavshen · 2023-08-02T08:20:38Z

I checked this commit over tag 3.1.38 hoping it fixes WebGL problems in wasm32 with 4GB memory (as it incorporates changes from #15433). Vertex/index buffers and uncompressed textures work with pointers over 2GB, but compressed textures (DXT1/BC1) work only in WebGL 1 but don't work in WebGL 2, the following run-time error is shown:

Uncaught RangeError: Failed to execute 'compressedTexImage2D' on 'WebGL2RenderingContext': The ArrayBuffer/ArrayBufferView size exceeds the supported range.
    at _glCompressedTexImage2D

PhilippMuigg · 2023-08-02T11:16:39Z

I checked this commit over tag 3.1.38 hoping it fixes WebGL problems in wasm32 with 4GB memory (as it incorporates changes from #15433). Vertex/index buffers and uncompressed textures work with pointers over 2GB, but compressed textures (DXT1/BC1) work only in WebGL 1 but don't work in WebGL 2, the following run-time error is shown:
Uncaught RangeError: Failed to execute 'compressedTexImage2D' on 'WebGL2RenderingContext': The ArrayBuffer/ArrayBufferView size exceeds the supported range.
    at _glCompressedTexImage2D

The primary issue with these _glXTexImageX methods in WebGL 2 is that they directly access an ArrayBuffer. It looks like most browsers nowadays have limitations wrt the size of these buffers when passed into the corresponding webgl 2 functions. Thus my changes in this commit create a copy of the data that is being sent to WebGL 2. This is similar to how the WebGL 1 code path is working. Though please note, that most of the changes proposed here are only active when compiling to wasm64.

pavshen · 2023-08-02T13:36:10Z

Anyway, I found the same problem with _glCompressedTexSubImage2D. Both problems are solved by changing the ifinside the functions' implementations so that the code for wasm64 works also for wasm32 with > 2GB.

emcc.py

sbc100 · 2023-08-28T07:41:43Z

src/library_webgl2.js

@@ -630,23 +646,23 @@ var LibraryWebGL2 = {
    assert((value & 3) == 0, 'Pointer to integer data passed to glClearBufferiv must be aligned to four bytes!');
 #endif

-    GLctx.clearBufferiv(buffer, drawbuffer, HEAP32, value>>2);
+    GLctx.clearBufferiv(buffer, drawbuffer, HEAP32, {{{ ptrToIdx('value', 2) }}});


How about using the existing getHeapOffset('value', 'u32') here instead of this new macro?

In particular I don't love the 2 in the second argument to ptrToIdx. It seems very non-obvious compared to getHeapOffset('value', 'u32')

Agreed, since there is now a more standardized way to make this conversion i will use it (these index conversion macros are part of an old commit from @juj that i pulled in to get some of this to work which i did not want to change too much).

sbc100 · 2023-08-29T16:28:37Z

src/parseTools.js

+}
+
+// Browsers might not support passing BigInts to different API entry points that take in addresses, so convert those to Number()s before calling into the browser APIs.
+function idxToMemory53(heapIndex) {


I would hope this is not needed since all pointers should be converted to int53 numbers when they arrive from native code.

Ok, i will give removing this a try

sbc100 · 2023-10-16T12:13:29Z

src/library_webgl.js

@@ -376,7 +376,11 @@ var LibraryGL = {
      var source = '';
      for (var i = 0; i < count; ++i) {
        var len = length ? {{{ makeGetValue('length', 'i*4', 'i32') }}} : -1;
+#if MEMORY64
+        source += UTF8ToString({{{ makeGetValue('string', 'i*8', '*') }}}, len < 0 ? undefined : len);
+#else
        source += UTF8ToString({{{ makeGetValue('string', 'i*4', 'i32') }}}, len < 0 ? undefined : len);


Can you just use i*{POINTERSIZE} and * here, which should work in both cases I think?

thanks, will make that change.

sbc100 · 2023-10-16T12:13:54Z

src/library_webgl.js

@@ -1424,7 +1428,7 @@ var LibraryGL = {
  },

  glCompressedTexImage2D: (target, level, internalFormat, width, height, border, imageSize, data) => {
-#if MAX_WEBGL_VERSION >= 2
+#if MAX_WEBGL_VERSION >= 2 && !MEMORY64


I have not checked the newest versions of chrome/firefox, but up until recently calling webgl functions that directly accessed an array view via an offset did not work, when the array view was larger than 4GB (this is why i am using the fallback versions of these functions for which data has to be copied into a separate array). I will check, if this still holds true. If newer browser versions got rid of that limitation i can remove a lot of these checks.

If I am not mistaken, in wasm32 when the array view is larger than 2GB, there is the same problem.

sbc100 · 2023-10-16T12:14:31Z

src/library_webgl.js

+
+#if MEMORY64
+    {{{ makeSetValue('pointer', '0', 'GLctx.getVertexAttribOffset(index, pname)', 'i64') }}};
+#else
    {{{ makeSetValue('pointer', '0', 'GLctx.getVertexAttribOffset(index, pname)', 'i32') }}};


Can you just use '* here?

yes, that should work.

sbc100 · 2023-10-16T12:16:29Z

src/library_webgl.js

@@ -2320,17 +2338,17 @@ var LibraryGL = {
  glUniform1iv: (location, count, value) => {
 #if GL_ASSERTIONS
    GL.validateGLObjectID(GLctx.currentProgram.uniformLocsById, location, 'glUniform1iv', 'location');
-    assert((value & 3) == 0, 'Pointer to integer data passed to glUniform1iv must be aligned to four bytes!');
+    assert({{{ isPtrAligned('value', 4) }}}, 'Pointer to integer data passed to glUniform1iv must be aligned to four bytes!');


{POINTER_SIZE} bytes?

sbc100 · 2023-10-16T12:18:21Z

src/parseTools.js

@@ -920,6 +925,36 @@ function to64(x) {
  return `BigInt(${x})`;
 }

+// Generates an expression that tests whether a pointer (either a BigInt or a signed int32 JS Number) is aligned to the given byte multiple.
+function isPtrAligned(ptr, alignment) {


Do you need to second argument here? Shouldn't pointer always be aligned to pointer size? i.e. should alignment here not always be POINTER_SIZE?

This method is used in all the places where we previously did something like this

assert((value & 3) == 0, 'Pointer to integer data passed to glUniform2iv must be aligned to four bytes!');

So for example, if we want to pass int32 values to webGL (e.g. via glUniform2iv) then this data has to be aligned to 4 bytes because it is being passed via the HEAP32 array buffer view. To be honest, when i first saw this code i was a bit weary, since on the C/C++ side such guarantees are never required and i am pretty sure, that i could write some simple code which would break this assumption. But as far as i can tell, up until now, our production code has never caused any issues here.

As for whether the second parameter is required. I think currently we are always either using 4 byte alignment or 1 byte alignment (i.e. unaligned). So there is definitely space for improvement. Will take a closer look at this.

pavshen · 2023-10-16T12:58:34Z

I tested it above 3.1.47 on wasm32 with 4GB: WebGL2 still doesn't work.

PhilippMuigg · 2023-10-19T06:04:09Z

I tested it above 3.1.47 on wasm32 with 4GB: WebGL2 still doesn't work.

Right, so i am trying to keep the changes in this commit limited to the wasm64 compile target. The whole 2GB (and sometimes 4GB) limit on array views that are passed off to webgl2 functions seems to be something that probably has to be addressed by browser developers? In the long run i would also like to get rid of the workarounds used in my commit to actually use the faster webgl2 functions that do not involve copying all the data that is being passed in.

pavshen · 2023-10-19T14:12:21Z

By the way, there are some failures with WebGL1, too. Cannot you take into consideration the case of wasm32 >2GB as well? I suppose it requires similar fixes. I can help to test many different uses of WebGL 1/2 with my graphical engine. Thanks in advance.

sbc100 · 2024-03-28T23:09:18Z

I believe that all of these fixes have now landed as part of the sequence of changes that made all our tests pass in wasm64 and 2gb+ mode and 4gb+ mode. e.g. #21573, #21445, #21306, #21264, #21220.

My apologies for not landing this change and instead incrementally implementing it. Hopefully the end result is that all your code should now work. If you think there are any remaining wasm64 or 2gb+ issue please let me know.

PhilippMuigg · 2024-03-29T07:17:29Z

I believe that all of these fixes have now landed as part of the sequence of changes that made all our tests pass in wasm64 and 2gb+ mode and 4gb+ mode. e.g. #21573, #21445, #21306, #21264, #21220.

My apologies for not landing this change and instead incrementally implementing it. Hopefully the end result is that all your code should now work. If you think there are any remaining wasm64 or 2gb+ issue please let me know.

No worries, i am glad that i was able to point out issues that we ran into with this PR and i am excited to test this again.

pavshen · 2024-04-11T08:20:03Z

Next week, I am going to check 4GB WebGL2 with the newly released 3.1.57 (which should include all these fixes, am I right?) and I let you know if there are problems.

sbc100 · 2024-04-11T16:30:16Z

Thanks! I guess we can close this issue, and please open any new ones if you fine them. I'm pretty sure we have good test coverage at this point.

PhilippMuigg commented Feb 14, 2023

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

PhilippMuigg force-pushed the memory64-opengl branch from 7db7b6c to 85d3e5a Compare February 14, 2023 08:36

PhilippMuigg force-pushed the memory64-opengl branch 6 times, most recently from 4111e3f to a0f65ae Compare March 17, 2023 14:00

sbc100 reviewed Mar 17, 2023

View reviewed changes

src/library_egl.js Outdated Show resolved Hide resolved

PhilippMuigg force-pushed the memory64-opengl branch 2 times, most recently from 6084252 to 6d06901 Compare March 21, 2023 11:58

PhilippMuigg force-pushed the memory64-opengl branch 2 times, most recently from 68d6c7b to bc1a4fd Compare May 11, 2023 11:16

sbc100 reviewed May 13, 2023

View reviewed changes

sbc100 added the wasm64 label Jun 7, 2023

PhilippMuigg force-pushed the memory64-opengl branch from bc1a4fd to 4c3a66e Compare June 12, 2023 16:09

sbc100 mentioned this pull request Jul 12, 2023

How does emscripten generate 32-bit and 64-bit wasm #19833

Closed

pavshen mentioned this pull request Aug 16, 2023

Embind problems with val::as<T>() in wasm32 with MAXIMUM_MEMORY=4GB #19968

Closed

PhilippMuigg force-pushed the memory64-opengl branch 8 times, most recently from 68cf024 to 3ebab42 Compare August 29, 2023 13:31

sbc100 reviewed Aug 29, 2023

View reviewed changes

PhilippMuigg force-pushed the memory64-opengl branch 10 times, most recently from 80329b9 to 4801d2c Compare October 13, 2023 17:29

sbc100 reviewed Oct 16, 2023

View reviewed changes

PhilippMuigg force-pushed the memory64-opengl branch from 4801d2c to f15150e Compare October 17, 2023 09:09

opengl support for memory64

266116b

PhilippMuigg force-pushed the memory64-opengl branch from f15150e to 266116b Compare October 18, 2023 08:59

PhilippMuigg closed this Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opengl support for memory64 #18744

opengl support for memory64 #18744

PhilippMuigg commented Feb 14, 2023 •

edited

Loading

sbc100 May 13, 2023

PhilippMuigg Aug 29, 2023

sbc100 May 13, 2023

PhilippMuigg Jun 12, 2023

sbc100 May 13, 2023

PhilippMuigg Jun 12, 2023

pavshen commented Aug 2, 2023 •

edited

Loading

PhilippMuigg commented Aug 2, 2023

pavshen commented Aug 2, 2023 •

edited

Loading

sbc100 Aug 28, 2023

sbc100 Aug 28, 2023

PhilippMuigg Aug 30, 2023

sbc100 Aug 29, 2023

PhilippMuigg Aug 30, 2023

sbc100 Oct 16, 2023

PhilippMuigg Oct 18, 2023

sbc100 Oct 16, 2023

PhilippMuigg Oct 17, 2023 •

edited

Loading

pavshen Oct 17, 2023

sbc100 Oct 16, 2023

PhilippMuigg Oct 18, 2023

sbc100 Oct 16, 2023

sbc100 Oct 16, 2023

PhilippMuigg Oct 16, 2023

pavshen commented Oct 16, 2023

PhilippMuigg commented Oct 19, 2023

pavshen commented Oct 19, 2023 •

edited

Loading

sbc100 commented Mar 28, 2024

PhilippMuigg commented Mar 29, 2024

pavshen commented Apr 11, 2024

sbc100 commented Apr 11, 2024

opengl support for memory64 #18744

opengl support for memory64 #18744

Conversation

PhilippMuigg commented Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavshen commented Aug 2, 2023 • edited Loading

PhilippMuigg commented Aug 2, 2023

pavshen commented Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippMuigg Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavshen commented Oct 16, 2023

PhilippMuigg commented Oct 19, 2023

pavshen commented Oct 19, 2023 • edited Loading

sbc100 commented Mar 28, 2024

PhilippMuigg commented Mar 29, 2024

pavshen commented Apr 11, 2024

sbc100 commented Apr 11, 2024

PhilippMuigg commented Feb 14, 2023 •

edited

Loading

pavshen commented Aug 2, 2023 •

edited

Loading

pavshen commented Aug 2, 2023 •

edited

Loading

PhilippMuigg Oct 17, 2023 •

edited

Loading

pavshen commented Oct 19, 2023 •

edited

Loading