ircam-ismm / node-web-audio-api

Web Audio API implementation for Node.js
https://www.npmjs.com/package/node-web-audio-api
BSD 3-Clause "New" or "Revised" License
106 stars 14 forks source link

Feat - some ScriptProcessorNode tail #114

Closed b-ma closed 7 months ago

b-ma commented 7 months ago

First (...failed) attempt

To make it short

Most simplified example

relevant code is here: https://github.com/ircam-ismm/node-web-audio-api/compare/feat/script-processor?expand=1#diff-f2a3e90afd7d940e52b634347f9a1e12ee21402dafa586f92c960ad13fa052fcR146

In short:


node.set_onaudioprocess(move |e| {
    tsfn.call(Ok(e), ThreadsafeFunctionCallMode::Blocking);
});

Problem

error[E0521]: borrowed data escapes outside of closure
   --> src/script_processor_node.rs:171:17
    |
169 |             node.set_onaudioprocess(move |e| {
    |                                           -
    |                                           |
    |                                           `e` is a reference that is only valid in the closure body
    |                                           has type `&'1 mut AudioProcessingEvent`
170 |                 // let event = WebAudioEventType::from(e);
171 |                 tsfn.call(Ok(e), ThreadsafeFunctionCallMode::Blocking);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                 |
    |                 `e` escapes the closure body here
    |                 argument requires that `'1` must outlive `'static`

\o/

orottier commented 7 months ago

Right, the signature F: FnMut(&mut AudioProcessingEvent) is simply not compatible with the ThreadSafeFunction way of working.

Even if we would change it to F: FnMut(Arc<Mutex<AudioProcessingEvent>>) (so you can clone it out while still being able to mutate) it would not work because right after the event handler is run the buffer is shipped back to the render thread. So you will not be in time with the actual mutation.

Only solution I can come up with now is to provide a completely different (hidden) API. In which the AudioProcessingEvent contains a oneshot channel to the ScriptRenderer. I will have to sketch that out a bit before knowing it will really work.

b-ma commented 7 months ago

Right, the signature F: FnMut(&mut AudioProcessingEvent) is simply not compatible with the ThreadSafeFunction way of working.

Ok thanks, I can stop trying to imagine fancy workarounds :)

If ThreadsafeFunctionCallMode::Blocking does what it pretends, maybe the first solution F: FnMut(Arc<Mutex<AudioProcessingEvent>>) could work no? I will make a dirty hack to test it, I will let you know

orottier commented 7 months ago

I think we established earlier that Blocking does not execute immediately, but its worth the try. Im working on an implementation now that suits both rust and node, hooking into the Drop of the processing event. Stay tuned

b-ma commented 7 months ago

I think we established earlier that Blocking does not execute immediately, but its worth the try.

Right, seems I misunderstood the blocking thing, this seems to refer only to how the callback is added to the event queue. There is still more than one reference when a try to unwrap the Arc to its inner value...

Im working on an implementation now that suits both rust and node, hooking into the Drop of the processing event. Stay tuned

Ok cool, let me know!

orottier commented 7 months ago

Really cool you managed to get it to work. Quite the milestone to finally have custom JS audio processing now. I need a bit more time to look at all of it. I'm a bit worried about your new ThreadSafeFunction implementation, what are the exact changes and how do we keep it up to date over time? I will merge the new Rust API for the processing event. I think it's fine in the new way.

b-ma commented 7 months ago

I'm a bit worried about your new ThreadSafeFunction implementation, what are the exact changes and how do we keep it up to date over time?

Yup that may be an issue, but several mitigating points:

b-ma commented 7 months ago

Just to say, this will all be probably dropped and rewritten because it is based on dead branch

No idea yet on how to get rid of the alternative implementation though... However I think it's good anyway to put a nail into the guts because I'm pretty sure AudioWorklet (to work properly) will require importing and binding stuff like node.h, lib_uv.h or v8.h

orottier commented 7 months ago

Alright, let's go with this version for now then. I will release the new owned API today so this branch can be merged

orottier commented 7 months ago

v1.0.0-rc.6 has the API this PR requires.

b-ma commented 7 months ago

Closing in favor of #121