Merge pull request #469 from orottier/bugfix/panics-in-nodecollection

Dynamic lifetime issues and the node collection

Author: orottier

src/node/delay.rs

@@ -374,6 +374,10 @@ impl AudioProcessor for DelayWriter {
         // let the node be decommisioned if it has no input left
         false
     }
+
+    fn has_side_effects(&self) -> bool {
+        true // message passing
+    }
 }
 
 impl DelayWriter {

src/node/destination.rs

@@ -141,4 +141,8 @@ impl AudioProcessor for DestinationRenderer {
 
         true
     }
+
+    fn has_side_effects(&self) -> bool {
+        true // speaker output
+    }
 }

src/render/graph.rs

@@ -89,19 +89,26 @@ impl Node {
             return false;
         }
 
-        // Drop when the node does not have any inputs and outputs
-        if !self.has_inputs_connected && self.outgoing_edges.is_empty() {
-            return true;
+        // When the nodes has no incoming connections:
+        if !self.has_inputs_connected {
+            // Drop when the processor reports it won't yield output.
+            if !tail_time {
+                return true;
+            }
+
+            // Drop when the node does not have any inputs and outputs
+            if self.outgoing_edges.is_empty() {
+                return true;
+            }
         }
 
-        // Drop when the node does not have any inputs connected,
-        // and if the processor reports it won't yield output.
-        if !self.has_inputs_connected && !tail_time {
+        // Node has no control handle and does have inputs connected.
+        // Drop when the processor when it has no outputs connected and does not have side effects
+        if !self.processor.has_side_effects() && self.outgoing_edges.is_empty() {
             return true;
         }
 
         // Otherwise, do not drop the node.
-        // (Even if it has no outputs connected, it may have side effects)
         false
     }
 
@@ -193,8 +200,8 @@ impl Graph {
     }
 
     pub fn add_edge(&mut self, source: (AudioNodeId, usize), dest: (AudioNodeId, usize)) {
-        self.nodes[source.0]
-            .get_mut()
+        self.nodes
+            .get_unchecked_mut(source.0)
             .outgoing_edges
             .push(OutgoingEdge {
                 self_index: source.1,
@@ -206,8 +213,8 @@ impl Graph {
     }
 
     pub fn remove_edge(&mut self, source: AudioNodeId, dest: AudioNodeId) {
-        self.nodes[source]
-            .get_mut()
+        self.nodes
+            .get_unchecked_mut(source)
             .outgoing_edges
             .retain(|edge| edge.other_id != dest);
 
@@ -216,7 +223,7 @@ impl Graph {
 
     pub fn remove_edges_from(&mut self, source: AudioNodeId) {
         // Remove outgoing edges
-        self.nodes[source].get_mut().outgoing_edges.clear();
+        self.nodes.get_unchecked_mut(source).outgoing_edges.clear();
 
         // Remove incoming edges - we need to traverse all nodes
         self.nodes.values_mut().for_each(|node| {
@@ -241,21 +248,27 @@ impl Graph {
     }
 
     pub fn mark_cycle_breaker(&mut self, index: AudioNodeId) {
-        self.nodes[index].get_mut().cycle_breaker = true;
+        self.nodes.get_unchecked_mut(index).cycle_breaker = true;
     }
 
     pub fn set_channel_count(&mut self, index: AudioNodeId, v: usize) {
-        self.nodes[index].get_mut().channel_config.count = v;
+        self.nodes.get_unchecked_mut(index).channel_config.count = v;
     }
     pub fn set_channel_count_mode(&mut self, index: AudioNodeId, v: ChannelCountMode) {
-        self.nodes[index].get_mut().channel_config.count_mode = v;
+        self.nodes
+            .get_unchecked_mut(index)
+            .channel_config
+            .count_mode = v;
     }
     pub fn set_channel_interpretation(&mut self, index: AudioNodeId, v: ChannelInterpretation) {
-        self.nodes[index].get_mut().channel_config.interpretation = v;
+        self.nodes
+            .get_unchecked_mut(index)
+            .channel_config
+            .interpretation = v;
     }
 
     pub fn route_message(&mut self, index: AudioNodeId, msg: &mut dyn Any) {
-        self.nodes[index].get_mut().processor.onmessage(msg);
+        self.nodes.get_unchecked_mut(index).processor.onmessage(msg);
     }
 
     /// Helper function for `order_nodes` - traverse node and outgoing edges
@@ -278,7 +291,7 @@ impl Graph {
             let cycle_breaker_node = marked_temp
                 .iter()
                 .skip(pos)
-                .find(|&&node_id| self.nodes[node_id].borrow().cycle_breaker);
+                .find(|&&node_id| self.nodes.get_unchecked(node_id).borrow().cycle_breaker);
 
             match cycle_breaker_node {
                 Some(&node_id) => {
@@ -307,7 +320,13 @@ impl Graph {
         marked_temp.push(node_id);
 
         // Visit outgoing nodes, and call `visit` on them recursively
-        for edge in self.nodes[node_id].borrow().outgoing_edges.iter() {
+        for edge in self
+            .nodes
+            .get_unchecked(node_id)
+            .borrow()
+            .outgoing_edges
+            .iter()
+        {
             let cycle_breaker_applied = self.visit(
                 edge.other_id,
                 marked,
@@ -386,9 +405,11 @@ impl Graph {
 
             if cycle_breaker_applied {
                 // clear the outgoing edges of the nodes that have been recognized as cycle breaker
-                let nodes = &mut self.nodes;
                 cycle_breakers.iter().for_each(|node_id| {
-                    nodes[*node_id].get_mut().outgoing_edges.clear();
+                    self.nodes
+                        .get_unchecked_mut(*node_id)
+                        .outgoing_edges
+                        .clear();
                 });
 
                 continue;
@@ -423,16 +444,13 @@ impl Graph {
         // keep track of end-of-lifecyle nodes
         let mut nodes_dropped = false;
 
-        // for borrow-checker reasons, move mutable borrow of nodes out of self
-        let nodes = &mut self.nodes;
-
         // process every node, in topological sorted order
         self.ordered.iter().for_each(|index| {
             // acquire a mutable borrow of the current processing node
-            let mut node = nodes[*index].borrow_mut();
+            let mut node = self.nodes.get_unchecked(*index).borrow_mut();
 
             // let the current node process (catch any panics that may occur)
-            let params = AudioParamValues::from(nodes);
+            let params = AudioParamValues::from(&self.nodes);
             scope.node_id.set(*index);
             let (success, tail_time) = {
                 // We are abusing AssertUnwindSafe here, we cannot guarantee it upholds.
@@ -456,7 +474,7 @@ impl Graph {
                 // audio params are connected to the 'hidden' usize::MAX output, ignore them here
                 .filter(|edge| edge.other_index != usize::MAX)
                 .for_each(|edge| {
-                    let mut output_node = nodes[edge.other_id].borrow_mut();
+                    let mut output_node = self.nodes.get_unchecked(edge.other_id).borrow_mut();
                     output_node.has_inputs_connected = true;
                     let signal = &node.outputs[edge.self_index];
                     let channel_config = &output_node.channel_config.clone();
@@ -482,7 +500,7 @@ impl Graph {
             // Check if we can decommission this node (end of life)
             if can_free {
                 // Node is dropped, remove it from the node list
-                let mut node = nodes.remove(*index).into_inner();
+                let mut node = self.nodes.remove(*index).into_inner();
                 self.reclaim_id_channel
                     .push(node.reclaim_id.take().unwrap());
                 drop(node);
@@ -492,39 +510,22 @@ 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
-
+                self.nodes.values_mut().for_each(|node| {
                     // 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.outgoing_edges;
-                        let prev_len = outgoing_edges.len();
-                        outgoing_edges.retain(|e| e.other_id != *index);
-                        outgoing_edges.len() != prev_len
-                    };
-
-                    // 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.0 < 2 || !was_connected || !node.control_handle_dropped;
-
-                    if !retain {
-                        self.reclaim_id_channel
-                            .push(node.reclaim_id.take().unwrap());
-                    }
-                    retain
-                })
+                    // either an AudioParam or the AudioListener that feeds into a PannerNode.
+                    // These should be disconnected
+                    node.get_mut()
+                        .outgoing_edges
+                        .retain(|e| e.other_id != *index);
+                });
             }
         });
 
         // If there were any nodes decommissioned, remove from graph order
         if nodes_dropped {
             let mut i = 0;
             while i < self.ordered.len() {
-                if nodes.get(self.ordered[i]).is_none() {
+                if !self.nodes.contains(self.ordered[i]) {
                     self.ordered.remove(i);
                 } else {
                     i += 1;
@@ -533,7 +534,7 @@ impl Graph {
         }
 
         // Return the output buffer of destination node
-        &self.nodes[AudioNodeId(0)].get_mut().outputs[0]
+        &self.nodes.get_unchecked_mut(AudioNodeId(0)).outputs[0]
     }
 }
 
@@ -735,7 +736,10 @@ mod tests {
         // dropped and the AudioNodeId(3) should be reclaimed
         add_node(&mut graph, 2, node.clone());
         // Mark the node as 'detached from the control thread', so it is allowed to drop
-        graph.nodes[AudioNodeId(2)].get_mut().control_handle_dropped = true;
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(2))
+            .control_handle_dropped = true;
 
         // Connect the regular node to the AudioDestinationNode
         add_edge(&mut graph, 2, 0);
@@ -777,7 +781,10 @@ mod tests {
         // dropped and the AudioNodeId(3) should be reclaimed
         add_node(&mut graph, 2, node.clone());
         // Mark the node as 'detached from the control thread', so it is allowed to drop
-        graph.nodes[AudioNodeId(2)].get_mut().control_handle_dropped = true;
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(2))
+            .control_handle_dropped = true;
 
         // Connect the regular node to the AudioDestinationNode
         add_edge(&mut graph, 2, 0);
@@ -786,7 +793,10 @@ mod tests {
         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[AudioNodeId(3)].get_mut().control_handle_dropped = true;
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(3))
+            .control_handle_dropped = true;
 
         // Connect the audioparam to the regular node
         add_audioparam(&mut graph, 3, 2);
@@ -799,7 +809,10 @@ mod tests {
             node_id: std::cell::Cell::new(AudioNodeId(0)),
             event_sender: None,
         };
-        graph.render(&scope);
+
+        // render twice
+        graph.render(&scope); // node is dropped
+        graph.render(&scope); // param is dropped
 
         // First the regular node should be dropped, then the audioparam
         assert_eq!(node_id_consumer.pop().unwrap().0, 2);
@@ -809,6 +822,78 @@ mod tests {
         assert!(node_id_consumer.pop().is_none());
     }
 
+    #[test]
+    fn test_audio_param_with_signal_lifecycle() {
+        let (node_id_producer, mut node_id_consumer) = llq::Queue::new().split();
+        let mut graph = Graph::new(node_id_producer);
+
+        let node = Box::new(TestNode { tail_time: false });
+
+        // Destination Node is always node id 0, and should never drop
+        add_node(&mut graph, 0, node.clone());
+
+        // AudioListener Node is always node id 1, and should never drop
+        add_node(&mut graph, 1, node.clone());
+
+        // Add a regular node at id 3, it has tail time false so after rendering it should be
+        // dropped and the AudioNodeId(3) should be reclaimed
+        add_node(&mut graph, 2, node.clone());
+        // Mark the node as 'detached from the control thread', so it is allowed to drop
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(2))
+            .control_handle_dropped = true;
+
+        // Connect the regular node to the AudioDestinationNode
+        add_edge(&mut graph, 2, 0);
+
+        // 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
+            .get_unchecked_mut(AudioNodeId(3))
+            .control_handle_dropped = true;
+
+        // Connect the audioparam to the regular node
+        add_audioparam(&mut graph, 3, 2);
+
+        // Add a source node to feed into the AudioParam
+        let signal = Box::new(TestNode { tail_time: true });
+        add_node(&mut graph, 4, signal);
+        add_edge(&mut graph, 4, 3);
+        // Mark the node as 'detached from the control thread', so it is allowed to drop
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(4))
+            .control_handle_dropped = true;
+
+        // Render a single quantum
+        let scope = AudioWorkletGlobalScope {
+            current_frame: 0,
+            current_time: 0.,
+            sample_rate: 48000.,
+            node_id: std::cell::Cell::new(AudioNodeId(0)),
+            event_sender: None,
+        };
+
+        // render twice
+        graph.render(&scope); // node is dropped
+        graph.render(&scope); // param is dropped
+
+        // First the regular node should be dropped, then the audioparam
+        assert_eq!(node_id_consumer.pop().unwrap().0, 2);
+        assert_eq!(node_id_consumer.pop().unwrap().0, 3);
+
+        // No other dropped nodes
+        assert!(node_id_consumer.pop().is_none());
+
+        // Render again
+        graph.render(&scope); // param signal source is dropped
+        assert_eq!(node_id_consumer.pop().unwrap().0, 4);
+    }
+
     #[test]
     fn test_release_orphaned_source_nodes() {
         let (node_id_producer, mut node_id_consumer) = llq::Queue::new().split();
@@ -828,7 +913,10 @@ mod tests {
         add_node(&mut graph, 2, node);
 
         // Mark the node as 'detached from the control thread', so it is allowed to drop
-        graph.nodes[AudioNodeId(2)].get_mut().control_handle_dropped = true;
+        graph
+            .nodes
+            .get_unchecked_mut(AudioNodeId(2))
+            .control_handle_dropped = true;
 
         // Render a single quantum
         let scope = AudioWorkletGlobalScope {