Skip to content

Commit

Permalink
Handle pathological case where an AudioParam outlives an AudioNode
Browse files Browse the repository at this point in the history
This could crash the render thread because previously it would drop the
AudioParamRenderer alongside the AudioNode
  • Loading branch information
orottier committed Oct 27, 2023
1 parent 404c48b commit eb7473d
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 5 deletions.
16 changes: 11 additions & 5 deletions src/render/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,23 +451,27 @@ impl Graph {
// Nodes are only dropped when they do not have incoming connections.
// But they may have AudioParams feeding into them, these can de dropped too.
nodes.retain(|id, node| {
let node = node.get_mut(); // unwrap the RefCell

// Check if this node was connected to the dropped node. In that case, it is
// either an AudioParam (which can be dropped), or the AudioListener that feeds
// into a PannerNode (which can be disconnected).
let was_connected = {
let outgoing_edges = &mut node.borrow_mut().outgoing_edges;
let outgoing_edges = &mut node.outgoing_edges;
let prev_len = outgoing_edges.len();
outgoing_edges.retain(|e| e.other_id != *index);
outgoing_edges.len() != prev_len
};

// retain when special or not connected to this dropped node
let special = id < 2; // never drop Listener and Destination node
let retain = special || !was_connected;
// Retain when
// - special node (destination = id 0, listener = id 1), or
// - not connected to this dropped node, or
// - if the control thread still has a handle to it.
let retain = id < 2 || !was_connected || !node.free_when_finished;

if !retain {
self.reclaim_id_channel
.push(node.get_mut().reclaim_id.take().unwrap());
.push(node.reclaim_id.take().unwrap());
}
retain
})
Expand Down Expand Up @@ -727,6 +731,8 @@ mod tests {
// Add an AudioParam at id 4, it should be dropped alongside the regular node
let param = Box::new(TestNode { tail_time: true }); // audio params have tail time true
add_node(&mut graph, 3, param);
// Mark the node as 'detached from the control thread', so it is allowed to drop
graph.nodes[3].get_mut().free_when_finished = true;

// Connect the audioparam to the regular node
add_audioparam(&mut graph, 3, 2);
Expand Down
27 changes: 27 additions & 0 deletions tests/online.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,34 @@ fn test_panner_node_drop_panic() {

// A crashed thread will not fail the test (only if the main thread panics).
// Instead inspect if there is progression of time in the audio context.
let time = context.current_time();
std::thread::sleep(std::time::Duration::from_millis(200));
assert!(context.current_time() >= time + 0.15);
}

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

// Create a node with an audioparam, drop to node but keep the audioparam
let gain = context.create_gain();
let gain_param = gain.gain().clone();
drop(gain);

// Start the audio graph, and give some time to drop the gain node (it has no inputs connected
// so dynamic lifetime will drop the node);
std::thread::sleep(std::time::Duration::from_millis(200));

// We still have a handle to the param, so that should not be removed from the audio graph.
// So by updating the value, the render thread should not crash.
gain_param.set_value(1.);

// A crashed thread will not fail the test (only if the main thread panics).
// Instead inspect if there is progression of time in the audio context.
let time = context.current_time();
std::thread::sleep(std::time::Duration::from_millis(200));
assert!(context.current_time() >= time + 0.15);
Expand Down

0 comments on commit eb7473d

Please sign in to comment.