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

[audioworklet] Bring Your Own Buffer style of memory management #2442

Open
joeberkovitz opened this issue Nov 7, 2017 · 23 comments
Open

[audioworklet] Bring Your Own Buffer style of memory management #2442

joeberkovitz opened this issue Nov 7, 2017 · 23 comments
Assignees
Labels
Priority: Urgent WG charter deliverables; "need to have". https://speced.github.io/spec-maintenance/about/

Comments

@joeberkovitz
Copy link
Contributor

Discussing with WebAssembly group, we've been talking about having an alternate approach to managing the buffers used to communicate data between an AudioWorkletProcessor and the rendering thread environment (input, output and parameters buffers). It would be better for WASM in many ways if we allowed the AWP code to respond to the needs of the environment by providing its own buffers, rather than having to work with JS buffers or views provided from outside. This would eliminate the number of copies and make for a very direct WebAssembly/Web Audio connection.

@mdjp mdjp transferred this issue from WebAudio/web-audio-api Sep 16, 2019
@hoch
Copy link
Member

hoch commented Sep 16, 2019

The conversation from TPAC:
We need to come up with the API design. For example, AudioWorkletProcessor's constructor can expose methods like registerInputBuffer() and registerOutputBuffer() so the user code pass the piece of WASM heap memory.

@svgeesus
Copy link
Contributor

@rtoy
Copy link
Member

rtoy commented Aug 13, 2020

Teleconf: Probably a duplicate of #2427, which has more detail on AudioWorklet and WASM.

@hoch
Copy link
Member

hoch commented Oct 14, 2020

The outcome from TPAC 2020:

This is not an exact duplicate of #2427. This can be applied to non-WASM use cases as well. With both improvements, the performance of Audio Worklet can be certainly improved.

What's being proposed is to call something like registerBuffer(wasmHeap) or registerBuffer(typedArray) at the node/processor registration procedure.

Also a callback function like ontopologychange is necessary, so the user code can reallocate memory when the incoming AudioNode configuration (e.g. channel count) is changed.

@padenot
Copy link
Member

padenot commented May 17, 2021

Discussed during the AudioWG online F2F, some pseudo-code that show how this could work (just a proposal, nothing concrete, drafted while talking during the call):

partial interface AudioWorkletProcessor {
    enum BufferType {
      INPUT = 1,
      OUTPUT = 2
    };
    // indicates that this memory will be used by the AudioWorklet -- don't touch it yourself
    // variant: register just the output, just the input.
    // This can be called in the ctor, or while process is running to provide the output from something already computed
    registerBuffers(ArrayBuffer memory, uint64_t offset, uint64_t maxLength, type = INPUT|OUTPUT);
 
    // Called when the input or output channel count changes for whatever reason
    // you can accept or deny the request. In case of error, `process` isn't called anymore
    channelTopologyChange = function(ArrayBuffer memory, uint64_t offset, uint64_t newMaxLength) {
        // return false; // reject the change request: audio will be down/up mixed to the previous topology
 
       // find some memory in already allocated space/allocate new memory
        
        return {
            memory: memory,
            offset: new_offset,
            maxLength: new_max_length
            type: INPUT
         };
    }
}

then the memory backing the arrays in process would be the memory registered above.

Being able to call registerBuffers from process will be very nice for be for developers using this pattern described by @hoch: buffers of the right size are prepared in advance (with a safety margin), and the worklet just uses one of them as output. The buffer could then be safely recycled in the next process call.

@mdjp
Copy link
Member

mdjp commented Jul 22, 2021

This would be a great issue to discuss across groups as TPAC 2021

@mdjp mdjp transferred this issue from WebAudio/web-audio-api-v2 Sep 29, 2021
@mdjp mdjp added the Priority: Urgent WG charter deliverables; "need to have". https://speced.github.io/spec-maintenance/about/ label Sep 29, 2021
@juj
Copy link

juj commented Nov 3, 2021

I did recently write an implementation that enables multithreaded Wasm code to define AudioWorkletProcessors and instantiate AudioWorkletNodes from Wasm. The branch can be found here:

https://github.com/juj/emscripten/compare/wasm_workers...juj:audio_worklets?expand=1

The intent is to bring the support in to core Emscripten in the near future.

The benefit of that branch with respect to this github issue is probably to concretely show what the JS<->Wasm marshalling copying overhead currently looks like, see the process function in src/library_webaudio.js.

In the implementation explicit care was taken to make the marshalling as "tight" as possible with minimized JS overhead (use Wasm thread stack space to avoid mallocs, minimize excess computation or abstractions in JS). So far we don't unfortunately have any data to know how much that marshalling performance overhead is in general.

For a small tone generator C program example of how one would use that API, see the end of the PR, tests/webaudio/tone_generator.c.

@Jonathhhan
Copy link

Jonathhhan commented Nov 15, 2021

I use Open Frameworks with Emscripten, and try to replace scriptprocessornode with audioworklet.
With sharedarraybuffer I can access the wasm memory from the main thread in the audioworklet. My problem is, that I cannot import a function (the audio callback) from the wasm Module in the main thread (it also needs to be in the main thread because it generates graphics too).

This is how I import the memory:

var outbufferArray = Module.HEAPF32.subarray(outbuffer >> 2, (outbuffer >> 2) + bufferSize * outputChannels); AUDIO.contexts[context_id].audioWorklet.addModule("bypass-processor.js").then(() => { const bypasser = new AudioWorkletNode(AUDIO.contexts[context_id], "bypass-processor", [1, 1, 2]); var worker = new Worker("wasm_worker.js"); bypasser.port.postMessage(outbufferArray) bypasser.port.onmessage = (e) => { dynCall("viiii", callback, [bufferSize, inputChannels, outputChannels, userData]); } bypasser.connect(AUDIO.contexts[context_id].destination) } );

This is the Audioworklet:

`
var counter = 0
class BypassProcessor extends AudioWorkletProcessor {
constructor() {
super()
this.port.onmessage = (e) => {
console.log(e.data, sampleRate)
this.data = e.data;
}
}

process(inputs, outputs) {

    counter = currentFrame / 128 % 64;
    if (counter == 0) {
        this.port.postMessage('pong')
    }
    const input = inputs[0];
    const output = outputs[0];
    for (let channel = 0; channel < 1; ++channel) {
        const outputChannel = output[channel];
            outputChannel.set(this.data.slice(counter * 128, counter * 128 + 128));
    }
    return true
}

}
registerProcessor('bypass-processor', BypassProcessor);
`

I think, my problem would be solved, if I could call dynCall("viiii", callback, [bufferSize, inputChannels, outputChannels, userData]); from the audioworklet. Calling the callback with port.postMessage is less reliable than audioscriptprocessor (but the sharedarraybuffer works well).

@juj
Copy link

juj commented Nov 16, 2021

@Jonathhhan check out the audio_worklets branch in my Emscripten Github repo.

In particular, the commit juj/emscripten@4aa53f4 . That achieves such a shared setup where the main wasm application can run on the main browser thread, and create wasm-based audioworklet nodes that call to a custom wasm function entry point for audio processing.

@Jonathhhan
Copy link

Jonathhhan commented Nov 17, 2021

@juj thank you. I will try that soon. Yesterday I had success with the solution from @tklajnscek emscripten-core/emscripten#12502 would be really great, if audioworklets will become a standard feature of Emscripten.

@hoch
Copy link
Member

hoch commented Nov 17, 2021

@juj Yes, that would be a huge improvement. Is a PR being reviewed somewhere? Is there something Audio WG can help/support?

@Jonathhhan
Copy link

Jonathhhan commented Nov 29, 2021

@juj I tried your tone_generator.c example like this: emcc -s MINIMAL_RUNTIME=1 -s AUDIO_WORKLET=1 -s WASM_WORKERS=1 -s WEBAUDIO_DEBUG=1 -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=$emscriptenGetAudioObject -o tone_generator.html tone_generator.c
It creates the audio worklet and compiles, but if I run it I get this error message:

Uncaught ReferenceError: emscriptenGetAudioObject is not defined
    at 1056 (343b50ff-7dba-4c7e-9fa8-85dade93c8ff:490:46)
    at _emscripten_asm_const_int (343b50ff-7dba-4c7e-9fa8-85dade93c8ff:584:31)
    at ba64fc02:0x639
    at MessagePort._EmAudioDispatchProcessorCallback (343b50ff-7dba-4c7e-9fa8-85dade93c8ff:664:30)
1056 @ 343b50ff-7dba-4c7e-9fa8-85dade93c8ff:490
_emscripten_asm_const_int @ 343b50ff-7dba-4c7e-9fa8-85dade93c8ff:584
$func9 @ ba64fc02:0x639
_EmAudioDispatchProcessorCallback @ 343b50ff-7dba-4c7e-9fa8-85dade93c8ff:664

Did I make mistake with the flags?

@juj
Copy link

juj commented Nov 30, 2021

No, that does look good. Tried it now and I don't get any issue. I am on commit

commit 4aa53f43af938d5d55f4ce54b0612387cbcffd21 (HEAD -> audio_worklets, juj/audio_worklets)
Author: Jukka Jylänki <jujjyl@gmail.com>
Date:   Sun Oct 24 18:47:25 2021 +0300

   Add first implementation of Wasm Audio Worklets, based on Wasm Workers.

and build with

E:\code\emsdk\emscripten\main>emcc -s MINIMAL_RUNTIME=1 -s AUDIO_WORKLET=1 -s WASM_WORKERS=1 -s WEBAUDIO_DEBUG=1 -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=$emscriptenGetAudioObject -o tone_generator.html tests\webaudio\tone_generator.c

which generates

11/30/2021  02:00 PM             6,732 tone_generator.aw.js
11/30/2021  02:00 PM             1,393 tone_generator.html
11/30/2021  02:00 PM            77,520 tone_generator.js
11/30/2021  02:00 PM             3,182 tone_generator.wasm
11/30/2021  02:00 PM               756 tone_generator.ww.js

Then running on an ad hoc web server on localhost I do get the tone generator running.

The error you print seems as if the command line directive -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=$emscriptenGetAudioObject did not work for some reason. The generated file tone_generator.js should have the following in it:

  function emscriptenGetAudioObject(objectHandle) {
      return EmAudio[objectHandle];
    }

Zipped the build here, which does work for me on Windows in Chrome and Firefox
tone_generator.zip

Also pushed a commit to the branch that improves some assertions and error checks for e.g. the scenario when the browser does not support AudioWorklets at all.

@juj
Copy link

juj commented Nov 30, 2021

@juj Yes, that would be a huge improvement. Is a PR being reviewed somewhere? Is there something Audio WG can help/support?

There is no PR yet, I am working on the prerequisite wasm workers PR first to build the necessary scaffolding to get audio worklets in the tree.

@Jonathhhan
Copy link

Jonathhhan commented Nov 30, 2021

@juj thanks. It works, if I paste:

  function emscriptenGetAudioObject(objectHandle) {
      return EmAudio[objectHandle];
    }

manually.

@juj
Copy link

juj commented Nov 30, 2021

oh wait, this is probably a Windows vs macOS/Linux build issue: you must be on a Unix-like OS with a bash-like shell?

There

-s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=$emscriptenGetAudioObject

will cause $emscriptenGetAudioObject to look up a variable, so it gets expanded to an empty string.

If you build by passing either

-s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=\$emscriptenGetAudioObject

or

-s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=$$emscriptenGetAudioObject

does that work? (not sure which of those escapes a $ character on cmdline, quick google search didn't find a hit either)

@Jonathhhan
Copy link

Jonathhhan commented Nov 30, 2021

@juj thanks a lot. -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=\$emscriptenGetAudioObject actually works.
Also already tried to implement it into Open Frameworks, but then I realized it is not possible to use WASM_WORKERS and USE_PTHREADS at the same time yet. And I need USE_PTHREADS for sharedArrayBuffer as far as I know to share the memory with the AudioWorklet?
Edit: Now I think I understand better. WASM_WORKERS are an optional replacement for USE_PTHREADS and they are not implemented in the master branch yet...

@meditating-monkey
Copy link

Any updates on this registerBuffer() method?

@guest271314

This comment was marked as off-topic.

@hoch
Copy link
Member

hoch commented Sep 13, 2023

2023 TPAC Audio WG Discussion:
padenot@ will write a document that explains the proposal shown in this comment. The WG will contact developers who made significant contributions to the similar project (Emscripten) to gather feedback.

@juj
Copy link

juj commented Sep 13, 2023

I now only finally paused to think about @padenot 's code snippet above (#2442 (comment)). Not sure if the thinking has evolved since it has been a long time, but I was puzzled over the channelTopologyChange element there.

My understanding is that channelTopologyChange would be an event that would be raised when the memory requirements of the inputs and outputs would change so that the previous memory area passed to registerBuffers would no longer work?

And the handler of that channelTopologyChange callback would then have the responsibility of providing a new subarray location in the ArrayBuffer where the interaction would take place?

It looks like the proposed handler there returns the new subarray location. This seems to be duplicating the behavior that a call to registerBuffers also achieves, so I wonder if it would make sense to spec the handler of channelTopologyChange to need to call registerBuffers again in order to configure the new subarray region? That way the code flow would be uniform and there wouldn't exist two different looking mechanisms in the API to specify the subarray information.

I.e. something like

    enum BufferType {
      INPUT = 1,
      OUTPUT = 2
    };
    // indicates that this memory will be used by the AudioWorklet -- don't touch it yourself
    // variant: register just the output, just the input.
    // This can be called in the ctor, or while process is running to provide the output from something already computed
    registerBuffers(ArrayBuffer memory, uint64_t offset, uint64_t maxLength, type = INPUT|OUTPUT);
 
    // Called when the input or output channel count changes for whatever reason
    // you can accept or deny the request. In case of error, `process` isn't called anymore
    channelTopologyChange = function(ArrayBuffer memory, uint64_t offset, uint64_t newMaxLength) {
       // find some memory in already allocated space/allocate new memory, and update previous registration
       registerBuffers(memory, new_offset, new_max_length, INPUT);
    }
}

This way the code would look similar to first allocation, and subsequent reallocations.

One note is that there might be a big challenge to users with respect to multithreading safe memory allocation inside channelTopologyChange handler. In Emscripten that is no problem, we have well crafted multithreading safe allocators that are able to find memory allocations while other code is running. But I think it might make sense to explicitly write the spec with a wording that does not require the user to have such a capability.

That could be achieved by making sure that registerBuffers will allow "over-allocation", i.e. that the user could pass buffer sizes there that are well bigger than what the node currently needs for input&output. Then if user knows the maximum size needed, they can statically allocate that at startup, and pass it to registerBuffers, and not need to worry about resizes down the line. I am not sure if the above proposal already had that idea in mind (what should/would happen if maxLength was passed to be larger than what is used by the node), though that is something that will be a useful guarantee to have.

@hoch
Copy link
Member

hoch commented Sep 14, 2023

@juj Thanks so much for detailed comment! We'll review and respond soon.

@padenot
Copy link
Member

padenot commented Sep 20, 2023

@juj, I agree with everything you said in your comment: the API proposal and the caveat about this potentially requiring a multi-thread aware allocator (if the heap is shared between multiple threads, that is).

I had over-allocation in mind when proposing the above, because this is typically how this can be/is solved in native. Sometimes you allocate a bit more upfront, or also in other cases when it goes from stereo to mono you don't free the second buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Urgent WG charter deliverables; "need to have". https://speced.github.io/spec-maintenance/about/
Projects
None yet
Development

No branches or pull requests

10 participants