-
Notifications
You must be signed in to change notification settings - Fork 13
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
Work in progress for AudioWorkletNode via Worker thread #124
Conversation
env.run_script seems like it has no access to the global scope. It does not know registerProcessor and AudioWorkletProcessor. So maybe we should just call `eval`.
Cool! I will have a look soon |
Just made small change to remove the hardcoded path and have it work on my machine, this is really nice! Great work! |
TODO pass to the render call
[WARNING] make sure to check #124 (comment) to not reintroduce the |
I can advance on
next, can be done on JS side rather simply I think |
Thanks! Interesting stuff happening regarding the index.js. I will take care
Okay that would be helpful! |
For remaining
|
Other things I see that could be nice to have I think:
Then I start to be out of ideas too :) In any case, here are the latest wpt results:
Almost 80% of the tests (332)! |
Hey, I skimmed your commits and they look good. Nice to see progress on allocation free JS because it may be the garbage collector playing us parts in realtime safety. I have been benchmarking a bit and it's surprising to see the click actually happen inside the In any case, if I change
The load goes up from 11% CPU to 14% on my machine (because we removed some idling) but all the clicks/pop/underruns also seem to dissappear. Can you confirm? |
I just implemented the algorithm I described upper, would be nice to have your review when you find some time (no hurry :), should indeed help minimizing garbage collection I think
Actually, this is hard to tell because I do very rarely have clicks on my side now, but if it's better on your side then let's go that way! (maybe we should just make sure it doesn't create issue with the process exit, but has you had ideas to refactor anyway...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me, I have left some nitpicks.
However, the garbage collector stats before this change are equal to afterwards:
node --trace-gc examples/audio-worklet.mjs
gives e.g.
[46986:0x130028000] 5658 ms: Scavenge 5.9 (9.0) -> 4.3 (9.0) MB, 0.17 / 0.00 ms (average mu = 1.000, current mu = 1.000) task;
Meaning it reclaimed from 5.9 to 4.3 MB. More info: https://nodejs.org/en/learn/diagnostics/memory/using-gc-traces
After the change:
[47391:0x128028000] 5354 ms: Scavenge 5.9 (8.7) -> 4.3 (8.7) MB, 0.12 / 0.00 ms (average mu = 1.000, current mu = 1.000) task;
Not sure what to make from this..
|
||
for (output_number, output) in outputs.iter().enumerate() { | ||
let mut channels = js_outputs.get_element::<JsObject>(output_number as u32)?; | ||
if !check_same_io_layout(&js_outputs, outputs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in doubt if it was possible for output channel counts to change at runtime, but it is indeed possible:
https://webaudio.github.io/web-audio-api/#configuring-channels-with-audioworkletnodeoptions (note at item 4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup indeed, I think there might be an issue on the rust side on this point, the audioworkletnode-channel-count.https.html
wpt test is a good example of what is expected. The test is currently crashing for some reason I didn't really understand, but it seems the rs output length keeps to be 1 even when input is 3
let src = js_channel_buffer.as_ptr(); | ||
let dst = channel.as_ptr() as *mut f32; | ||
|
||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just safely use copy_from_slice
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first try but I did manage to find a way to iter_mut()
the &'static [&'static [&'static [f32]]]
output...
channels.delete_element(i)?; | ||
for (channel_number, channel) in input.iter().enumerate() { | ||
let js_channel = js_input.get_element::<JsTypedArray>(channel_number as u32)?; | ||
let mut js_channel_value = js_channel.into_value()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to check at some point if we don't allocate with some of these calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally except in rebuild_io_layout
there should be no allocation, but clearly needs to be confirmed
I'm just thinking we could also have a pool of preallocated Float32Array to mitigate further this possible issue
The GC pauses appear to be of the range 0.08 - 0.17 ms and seem not related to the excessive call durations I'm measuring.. So what is stalling my audio thread!? |
Hum, no clear idea neither... did you make it run for a long time ? I'm no expert but the few times I had to check such thing, the GC was called every 4-5 sec, so as our example is 8 sec we could just see the result of some initialisation stuff?
Yup weird, no idea neither... note that I didn't remove the |
Okay after loads of logging I have come to the surprising conclusion that the call So as I already mentioned changing the call to just I definitely want to look into this further, possible using some primitives from the |
Regarding
We could maybe add a method |
Cool, nice that you found the issue! I think we could start to consider merging this, and put all remaining ideas / improvements to a new issue. Is there anything you think we should really do before? |
Yeah sounds good! Perhaps only my final comment |
Yup sure, I will have a look |
…fore actual startRendering
Follow up in #127 |
\o/ great work! |
Awesome work! 🔥 |
Quite the milestone indeed :) Congrats to us. And also thanks @jampekka for chiming in on the exploratory phase. An extra pair of eyes is always very motivating. |
Please let me know when you have released this on npm. I think it is worth the announcement on the web audio slack. Also I would like to get some tips and tricks from Paul and Hoch in the WG afterwards |
Done [email protected] !
Yup, I will try to attend too next times |
Many many todo, but it works now for single-channel source nodes!
examples/audio-worklet.mjs
Relates to #28
Todo
this
send_recv_pair
comms channel from main to workersend_recv_pair2
comms channel from worker to main2728 segmentation fault node examples/audio-worklet.mjs
Result::unwrap()
on anErr
value: RecvError at Immediate.run_loop ([worker eval]:95:5)context.addModule
, only pass the name in theAudioWorkletNode
ctor)process
call: