-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
AudioStreamCallback::_mix()
runs on separate thread + causes inevitable panic
#938
Comments
Releated to the issues in #713, in particular #709 (although that one got a tailored fix, which seems to not apply here).
This was "working" as a result of us not checking it. But Godot calling into Rust code through different threads can easily introduce unsoundness (one can bypass In fact If you have insights into how concretely Godot's multithreaded audio/video classes concretely interact with Rust, that would be very valuable... Maybe we can also think about specific patterns to make such interactions safer 🤔 |
What I do not understand is that there is no Godot binding activity happening from the audio thread. Adding some Example with debug statementsuse godot::classes::native::AudioFrame;
use godot::classes::{AudioServer, AudioStreamPlayback, IAudioStream, IAudioStreamPlayback, Os};
use godot::prelude::*;
#[derive(GodotClass)]
#[class(base=Node)]
pub struct Demo {
audio_player: Gd<AudioStreamPlayer>,
}
#[godot_api]
impl INode for Demo {
fn init(base: Base<Self::Base>) -> Self {
println!(
"Demo::init is running on thread {}",
Os::singleton().get_thread_caller_id()
);
let mut audio_player = AudioStreamPlayer::new_alloc();
audio_player.set_stream(Gd::<CustomAudioStream>::from_init_fn(|_| {
CustomAudioStream::new()
}));
base.to_gd().add_child(audio_player.clone());
Self { audio_player }
}
fn ready(&mut self) {
println!(
"Demo::ready is running on thread {}",
Os::singleton().get_thread_caller_id()
);
self.audio_player.play();
}
}
// CustomAudioStream
#[derive(GodotClass)]
#[class(base=AudioStream, no_init)]
pub struct CustomAudioStream {}
#[godot_api]
impl IAudioStream for CustomAudioStream {
fn instantiate_playback(&self) -> Option<Gd<AudioStreamPlayback>> {
println!(
"CustomAudioStream::instantiate_playback is running on thread {}",
Os::singleton().get_thread_caller_id()
);
Some(
Gd::<CustomAudioStreamPlayback>::from_init_fn(|_base| {
CustomAudioStreamPlayback::new(Sequencer {
sample_rate: AudioServer::singleton().get_mix_rate(),
sample_index: 0,
})
})
.upcast(),
)
}
}
impl CustomAudioStream {
pub fn new() -> Self {
println!(
"CustomAudioStream::new is running on thread {}",
Os::singleton().get_thread_caller_id()
);
Self {}
}
}
// CustomAudioStreamPlayback
#[derive(GodotClass)]
#[class(base=AudioStreamPlayback, no_init)]
pub struct CustomAudioStreamPlayback {
sequencer: Sequencer,
}
#[godot_api]
impl IAudioStreamPlayback for CustomAudioStreamPlayback {
unsafe fn mix(
&mut self,
buffer: *mut AudioFrame,
_rate_scale: f32,
num_requested_frames: i32,
) -> i32 {
println!(
"CustomAudioStreamPlayback::mix is running on thread {}",
Os::singleton().get_thread_caller_id()
);
self.sequencer.render_audio(num_requested_frames, buffer);
num_requested_frames
}
fn start(&mut self, _from_pos: f64) {}
fn stop(&mut self) {}
fn is_playing(&self) -> bool {
true
}
}
impl CustomAudioStreamPlayback {
fn new(sequencer: Sequencer) -> Self {
println!(
"CustomAudioStreamPlayback::new is running on thread {}",
Os::singleton().get_thread_caller_id()
);
Self { sequencer }
}
}
// Sequencer
pub struct Sequencer {
sample_rate: f32,
sample_index: usize,
}
impl Sequencer {
fn render_audio(&mut self, num_requested_frames: i32, buffer: *mut AudioFrame) {
const FREQUENCY: f32 = 440.0;
for i in 0..num_requested_frames {
let phase = 2.0 * std::f32::consts::PI * FREQUENCY * (self.sample_index as f32)
/ self.sample_rate;
let sample = 0.5 * phase.sin();
unsafe {
*buffer.offset(i as isize) = AudioFrame {
left: sample,
right: sample,
};
}
self.sample_index += 1;
}
}
}
I.e., it is only that This raises the question where the check is actually happening? The traceback doesn't make that very clear -- or it least it is not pointing to anything on user side. So is it just happening on the wrapper side of invoking |
The It can make sense to have methods that are called from other threads The checks happen here: gdext/godot-ffi/src/binding/single_threaded.rs Lines 159 to 163 in 19147e8
|
AudioStreamCallback::_mix()
runs on separate thread
Any ideas on this? I cannot extract anything actionable from this issue, other than "Certain virtual functions in Godot run on different threads, which isn't compatible with We'll likely need a proper threading model #18, and with it a way to explicitly relax such checks (e.g. via |
Some random ideas -- not sure if any of these make sense / is feasible:
Note that the renaming of the issue title no longer captures the actual issue from the user perspective: The fact that
I.e., I was aware that I'm on a different thread and I'm working in an |
Thanks a lot for the input! As mentioned, the As such, I'm not sure if your conclusions hold:
This is a very helpful summary, thank you. I'll try to reflect this in the issue name. ... All in all, I wonder how far we can get with isolated patches to the expectation/thread model. It's likely inevitable to address threads in a more "foundational" way and get them out of the experimental stage. We won't come up with a waterproof model, but we can definitely do better than today. Some parts of the puzzle could be:
|
AudioStreamCallback::_mix()
runs on separate threadAudioStreamCallback::_mix()
runs on separate thread + causes inevitable panic
The following code used to run fine in older versions of
gdext
, but when switching to a more recent commit it starts panicking with:The example is basically the "hello world" of a custom audio stream:
The full traceback is as follows, but not very insightful:
Full output
It looks like it only panics in debug builds, and everything seems to work fine in release builds.
What surprises me: In older versions of gdext this was working without having to enable the
experimental-threads
feature. In general, I wanted to avoid the full overhead of activating this feature, and it seemed to be possible previously. From the traceback it is actually not quite obvious where the access to the binding from a different thread is happening. Is there a way to use this pattern without having to enable the feature?Isn't this pattern actually valid, considering that the pattern used to work and seems to work fine in release builds?
As far as I can see
instantiate_playback
does run on the main thread, and it is only the unsafemix
function that gets called from the audio thread. However, that function is "pure Rust", and doesn't do anything in terms of calling any binding, no?The text was updated successfully, but these errors were encountered: