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 hanging UI thread for certain AudioContext::suspend/stop actions #499

Merged
merged 4 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/context/concrete_base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,8 @@ impl ConcreteBaseAudioContext {

/// Updates state of current context
pub(super) fn set_state(&self, state: AudioContextState) {
// Only to be used from OfflineAudioContext, the online one should emit the state changes
// from the render thread
debug_assert!(self.offline());

// Only used from OfflineAudioContext or suspended AudioContext, otherwise the state
// changed ar spawned from the render thread
orottier marked this conversation as resolved.
Show resolved Hide resolved
let current_state = self.state();
if current_state != state {
self.inner.state.store(state as u8, Ordering::Release);
Expand Down
143 changes: 119 additions & 24 deletions src/context/online.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,21 +279,26 @@ impl AudioContext {
/// is currently not implemented.
#[allow(clippy::needless_collect, clippy::missing_panics_doc)]
pub fn set_sink_id_sync(&self, sink_id: String) -> Result<(), Box<dyn Error>> {
log::debug!("SinkChange requested");
if self.sink_id() == sink_id {
log::debug!("SinkChange: no-op");
return Ok(()); // sink is already active
}

if !is_valid_sink_id(&sink_id) {
Err(format!("NotFoundError: invalid sinkId {sink_id}"))?;
};

log::debug!("SinkChange: locking backend manager");
let mut backend_manager_guard = self.backend_manager.lock().unwrap();
let original_state = self.state();
if original_state == AudioContextState::Closed {
log::debug!("SinkChange: context is closed");
return Ok(());
}

// Acquire exclusive lock on ctrl msg sender
log::debug!("SinkChange: locking message channel");
let ctrl_msg_send = self.base.lock_control_msg_sender();

// Flush out the ctrl msg receiver, cache
Expand All @@ -303,13 +308,17 @@ impl AudioContext {
let graph = if matches!(pending_msgs.first(), Some(ControlMessage::Startup { .. })) {
// Handle the edge case where the previous backend was suspended for its entire lifetime.
// In this case, the `Startup` control message was never processed.
log::debug!("SinkChange: recover unstarted graph");

let msg = pending_msgs.remove(0);
match msg {
ControlMessage::Startup { graph } => graph,
_ => unreachable!(),
}
} else {
// Acquire the audio graph from the current render thread, shutting it down
log::debug!("SinkChange: recover graph from render thread");

let (graph_send, graph_recv) = crossbeam_channel::bounded(1);
let message = ControlMessage::CloseAndRecycle { sender: graph_send };
ctrl_msg_send.send(message).unwrap();
Expand All @@ -321,6 +330,7 @@ impl AudioContext {
graph_recv.recv().unwrap()
};

log::debug!("SinkChange: closing audio stream");
backend_manager_guard.close();

// hotswap the backend
Expand All @@ -330,10 +340,12 @@ impl AudioContext {
sink_id,
render_size_hint: AudioContextRenderSizeCategory::default(), // todo reuse existing setting
};
log::debug!("SinkChange: starting audio stream");
*backend_manager_guard = io::build_output(options, self.render_thread_init.clone());

// if the previous backend state was suspend, suspend the new one before shipping the graph
if original_state == AudioContextState::Suspended {
log::debug!("SinkChange: suspending audio stream");
backend_manager_guard.suspend();
}

Expand All @@ -352,6 +364,7 @@ impl AudioContext {
// trigger event when all the work is done
let _ = self.base.send_event(EventDispatch::sink_change());

log::debug!("SinkChange: done");
Ok(())
}

Expand Down Expand Up @@ -422,18 +435,30 @@ impl AudioContext {
/// * The audio device is not available
/// * For a `BackendSpecificError`
pub async fn suspend(&self) {
// First, pause rendering via a control message
// Don't lock the backend manager because we can't hold is across the await point
log::debug!("Suspend called");

if self.state() != AudioContextState::Running {
log::debug!("Suspend no-op - context is not running");
return;
}

// Pause rendering via a control message
let (sender, receiver) = oneshot::channel();
let notify = OneshotNotify::Async(sender);
self.base
.send_control_msg(ControlMessage::Suspend { notify });

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
log::debug!("Suspending audio graph, waiting for signal..");
receiver.await.unwrap();

// Then ask the audio host to suspend the stream
log::debug!("Suspended audio graph. Suspending audio stream..");
self.backend_manager.lock().unwrap().suspend();

log::debug!("Suspended audio stream");
}

/// Resumes the progression of time in an audio context that has previously been
Expand All @@ -445,19 +470,34 @@ impl AudioContext {
///
/// * The audio device is not available
/// * For a `BackendSpecificError`
#[allow(clippy::await_holding_lock)] // false positive due to explicit drop
pub async fn resume(&self) {
// First ask the audio host to resume the stream
self.backend_manager.lock().unwrap().resume();
// Lock the backend manager mutex to avoid concurrent calls
log::debug!("Resume called, locking backend manager");
let backend_manager_guard = self.backend_manager.lock().unwrap();

if self.state() != AudioContextState::Suspended {
log::debug!("Resume no-op - context is not suspended");
return;
}

// Ask the audio host to resume the stream
backend_manager_guard.resume();

// Then, ask to resume rendering via a control message
log::debug!("Resumed audio stream, waking audio graph");
let (sender, receiver) = oneshot::channel();
let notify = OneshotNotify::Async(sender);
self.base
.send_control_msg(ControlMessage::Resume { notify });

// Drop the Mutex guard so we won't hold it across an await
drop(backend_manager_guard);

// Wait for the render thread to have processed the resume message
// The AudioContextState will be updated by the render thread.
receiver.await.unwrap();
log::debug!("Resumed audio graph");
}

/// Closes the `AudioContext`, releasing the system resources being used.
Expand All @@ -469,22 +509,37 @@ impl AudioContext {
///
/// Will panic when this function is called multiple times
pub async fn close(&self) {
// First, stop rendering via a control message
let (sender, receiver) = oneshot::channel();
let notify = OneshotNotify::Async(sender);
self.base.send_control_msg(ControlMessage::Close { notify });
// Don't lock the backend manager because we can't hold is across the await point
log::debug!("Close called");

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
receiver.await.unwrap();
if self.state() == AudioContextState::Closed {
log::debug!("Close no-op - context is already closed");
return;
}

if self.state() == AudioContextState::Running {
// First, stop rendering via a control message
let (sender, receiver) = oneshot::channel();
let notify = OneshotNotify::Async(sender);
self.base.send_control_msg(ControlMessage::Close { notify });

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
log::debug!("Suspending audio graph, waiting for signal..");
receiver.await.unwrap();
} else {
// if the context is not running, change the state manually
self.base.set_state(AudioContextState::Closed);
}

// Then ask the audio host to close the stream
log::debug!("Suspended audio graph. Closing audio stream..");
self.backend_manager.lock().unwrap().close();

// Stop the AudioRenderCapacity collection thread
self.render_capacity.stop();

// TODO stop the event loop <https://github.com/orottier/web-audio-api-rs/issues/421>
log::debug!("Closed audio stream");
}

/// Suspends the progression of time in the audio context.
Expand All @@ -502,18 +557,31 @@ impl AudioContext {
/// * The audio device is not available
/// * For a `BackendSpecificError`
pub fn suspend_sync(&self) {
// First, pause rendering via a control message
// Lock the backend manager mutex to avoid concurrent calls
log::debug!("Suspend_sync called, locking backend manager");
let backend_manager_guard = self.backend_manager.lock().unwrap();

if self.state() != AudioContextState::Running {
log::debug!("Suspend_sync no-op - context is not running");
return;
}

// Pause rendering via a control message
let (sender, receiver) = crossbeam_channel::bounded(0);
let notify = OneshotNotify::Sync(sender);
self.base
.send_control_msg(ControlMessage::Suspend { notify });

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
log::debug!("Suspending audio graph, waiting for signal..");
receiver.recv().ok();

// Then ask the audio host to suspend the stream
self.backend_manager.lock().unwrap().suspend();
log::debug!("Suspended audio graph. Suspending audio stream..");
backend_manager_guard.suspend();

log::debug!("Suspended audio stream");
}

/// Resumes the progression of time in an audio context that has previously been
Expand All @@ -529,10 +597,20 @@ impl AudioContext {
/// * The audio device is not available
/// * For a `BackendSpecificError`
pub fn resume_sync(&self) {
// First ask the audio host to resume the stream
self.backend_manager.lock().unwrap().resume();
// Lock the backend manager mutex to avoid concurrent calls
log::debug!("Resume_sync called, locking backend manager");
let backend_manager_guard = self.backend_manager.lock().unwrap();

if self.state() != AudioContextState::Suspended {
log::debug!("Resume no-op - context is not suspended");
return;
}

// Ask the audio host to resume the stream
backend_manager_guard.resume();

// Then, ask to resume rendering via a control message
log::debug!("Resumed audio stream, waking audio graph");
let (sender, receiver) = crossbeam_channel::bounded(0);
let notify = OneshotNotify::Sync(sender);
self.base
Expand All @@ -541,6 +619,7 @@ impl AudioContext {
// Wait for the render thread to have processed the resume message
// The AudioContextState will be updated by the render thread.
receiver.recv().ok();
log::debug!("Resumed audio graph");
}

/// Closes the `AudioContext`, releasing the system resources being used.
Expand All @@ -555,22 +634,38 @@ impl AudioContext {
///
/// Will panic when this function is called multiple times
pub fn close_sync(&self) {
// First, stop rendering via a control message
let (sender, receiver) = crossbeam_channel::bounded(0);
let notify = OneshotNotify::Sync(sender);
self.base.send_control_msg(ControlMessage::Close { notify });
// Lock the backend manager mutex to avoid concurrent calls
log::debug!("Close_sync called, locking backend manager");
let backend_manager_guard = self.backend_manager.lock().unwrap();

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
receiver.recv().ok();
if self.state() == AudioContextState::Closed {
log::debug!("Close no-op - context is already closed");
return;
}

// First, stop rendering via a control message
if self.state() == AudioContextState::Running {
let (sender, receiver) = crossbeam_channel::bounded(0);
let notify = OneshotNotify::Sync(sender);
self.base.send_control_msg(ControlMessage::Close { notify });

// Wait for the render thread to have processed the suspend message.
// The AudioContextState will be updated by the render thread.
log::debug!("Suspending audio graph, waiting for signal..");
receiver.recv().ok();
} else {
// if the context is not running, change the state manually
self.base.set_state(AudioContextState::Closed);
}

// Then ask the audio host to close the stream
self.backend_manager.lock().unwrap().close();
log::debug!("Suspended audio graph. Closing audio stream..");
backend_manager_guard.close();

// Stop the AudioRenderCapacity collection thread
self.render_capacity.stop();

// TODO stop the event loop <https://github.com/orottier/web-audio-api-rs/issues/421>
log::debug!("Closed audio stream");
}

/// Creates a [`MediaStreamAudioSourceNode`](node::MediaStreamAudioSourceNode) from a
Expand Down
69 changes: 68 additions & 1 deletion tests/online.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,79 @@ fn test_closed() {
let context = AudioContext::new(options);
let node = context.create_gain();

// close the context, and drop it as well (otherwise the comms channel is kept alive)
// Close the context
context.close_sync();
assert_eq!(context.state(), AudioContextState::Closed);

// Should not be able to resume
context.resume_sync();
assert_eq!(context.state(), AudioContextState::Closed);

// Drop the context (otherwise the comms channel is kept alive)
drop(context);

// allow some time for the render thread to drop
std::thread::sleep(Duration::from_millis(10));

node.disconnect(); // should not panic
}

#[test]
fn test_double_suspend() {
let options = AudioContextOptions {
sink_id: "none".into(),
..AudioContextOptions::default()
};
let context = AudioContext::new(options);

context.suspend_sync();
assert_eq!(context.state(), AudioContextState::Suspended);
context.suspend_sync();
assert_eq!(context.state(), AudioContextState::Suspended);
context.resume_sync();
assert_eq!(context.state(), AudioContextState::Running);
}

#[test]
fn test_double_resume() {
let options = AudioContextOptions {
sink_id: "none".into(),
..AudioContextOptions::default()
};
let context = AudioContext::new(options);

context.suspend_sync();
assert_eq!(context.state(), AudioContextState::Suspended);
context.resume_sync();
assert_eq!(context.state(), AudioContextState::Running);
context.resume_sync();
assert_eq!(context.state(), AudioContextState::Running);
}

#[test]
fn test_double_close() {
let options = AudioContextOptions {
sink_id: "none".into(),
..AudioContextOptions::default()
};
let context = AudioContext::new(options);

context.close_sync();
assert_eq!(context.state(), AudioContextState::Closed);
context.close_sync();
assert_eq!(context.state(), AudioContextState::Closed);
}

#[test]
fn test_suspend_then_close() {
let options = AudioContextOptions {
sink_id: "none".into(),
..AudioContextOptions::default()
};
let context = AudioContext::new(options);

context.suspend_sync();
assert_eq!(context.state(), AudioContextState::Suspended);
context.close_sync();
assert_eq!(context.state(), AudioContextState::Closed);
}
Loading