Skip to content
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.

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

Fix audio worklet + memory64 #22731

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 14, 2024

There are a two different fixes here, neither of which are needed except in the nightly version of chrome.

  1. In the instantiateWasm override used in audio worklets return the result of receiveInstance rather then directly returning the wasm exports. This means that the resulting object has the needed wrappers applied. This was causing an exception even on older version of chrome, but the tests still passed regardless. With newer versions of chrome the exception seems to cause the worklet creation to fail. (see Ensure exports are correctly handled when instantiateWasm is present #18711)
  2. Using BigInt when indexing the wasm table. I would have prefered to use toIndexType here but that doesn't work in audio_worklet.js.

There are a two different fixes here, neither of which are needed
except in the nightly version of chrome.

1. In the `instantiateWasm` override used in audio worklets return
   the result of `receiveInstance` rather then directly returning the
   wasm exports.  This means that the resulting object has the needed
   wrappers applied.  This was causing an exception even on older
   version of chrome, but the tests still passed regardless.  With newer
   versions of chrome the exception seems to cause the worklet creation
   to fail.
2. Using BigInt when indexing the wasm table.  I would have prefered to
   use `toIndexType` here but that doesn't work in `audio_worklet.js`.
@sbc100 sbc100 requested review from juj and kripken October 14, 2024 19:15
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 14, 2024

This fixes the current CI breakage, which I believe is due to a change in chrome, not a change in emscripten. Since its only effects wasm64 I think its fine to just fix emscripten here.

@sbc100 sbc100 merged commit 8bf26bf into emscripten-core:main Oct 14, 2024
28 checks passed
@sbc100 sbc100 deleted the audio_worklet_wasm64 branch October 14, 2024 21:50
@juj
Copy link
Collaborator

juj commented Oct 15, 2024

There is still more to do to make AudioWorklets MEMORY64 compliant, it needs to write pointers in HEAPU32/HEAPU64 heap depending on memory mode. One could do the trick to bump the static memory size to 4GB so that all user allocations occur at >4GB mark to verify.

However definitely recommend letting cwoffenden's PR #22681 to land first to avoid any merge conflicts on his part.

@cwoffenden
Copy link
Contributor

However definitely recommend letting cwoffenden's PR #22681 to land first to avoid any merge conflicts on his part.

From looking at this, to do wasm64 properly, it would need the makeGetValue helpers fixing to work with the way HEAP32, etc., is exposed in the worklet (I tried it already, since it allows SAFE_HEAP, and it fails).

@juj
Copy link
Collaborator

juj commented Oct 15, 2024

In wasm_webgpu library I use the following kinds of helpers written ad-hoc inside the JS file:

https://github.com/juj/wasm_webgpu/blob/2e4664bee7079f69d0ce41bed43beae0c4de4bf8/lib/lib_webgpu.js#L26-L75

Something like that could be used to ease converting between Numbers and BigInts. In PR #19289 I proposed to add that type of API into Emscripten directly, though we did not find a consensus back then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants