Fix dynamic lifetime issue for nodes connected to params

Introduce AudioProcess::has_side_effects

A processor with no side effect will be dropped when the control handle
is gone and there are not output ports connected.

This makes the cleanup of AudioParams easier and more robust.

Relatest to #397 and #468

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 outpus 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
     }
 
@@ -503,31 +510,14 @@ 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.
-                self.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);
+                });
             }
         });
 
@@ -819,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);
@@ -870,6 +863,11 @@ mod tests {
         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 {
@@ -879,7 +877,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);
@@ -889,7 +890,8 @@ mod tests {
         assert!(node_id_consumer.pop().is_none());
 
         // Render again
-        graph.render(&scope);
+        graph.render(&scope); // param signal source is dropped
+        assert_eq!(node_id_consumer.pop().unwrap().0, 4);
     }
 
     #[test]

src/render/node_collection.rs

@@ -67,20 +67,6 @@ impl NodeCollection {
         self.nodes[index.0 as usize].as_mut()
     }
 
-    #[inline(always)]
-    pub fn retain<F>(&mut self, mut f: F)
-    where
-        F: FnMut(AudioNodeId, &mut RefCell<Node>) -> bool,
-    {
-        self.nodes.iter_mut().enumerate().for_each(|(i, opt)| {
-            if let Some(v) = opt.as_mut() {
-                if !f(AudioNodeId(i as u64), v) {
-                    *opt = None;
-                }
-            }
-        })
-    }
-
     #[track_caller]
     #[inline(always)]
     pub fn get_unchecked(&self, index: AudioNodeId) -> &RefCell<Node> {

src/render/processor.rs

@@ -141,10 +141,21 @@ pub trait AudioProcessor: Send {
     }
 
     /// Return the name of the actual AudioProcessor type
-    #[doc(hidden)] // not meant to be user facing
     fn name(&self) -> &'static str {
         std::any::type_name::<Self>()
     }
+
+    /// Indicates if this processor has 'side effects' other than producing output
+    ///
+    /// Processors without side effects can not be dropped when there are no outputs connected, and
+    /// when the control side handle no longer exists
+    ///
+    /// Side effects could include
+    /// - IO (e.g. speaker output of the destination node)
+    /// - Message passing (e.g. worklet nodes)
+    fn has_side_effects(&self) -> bool {
+        false
+    }
 }
 
 impl std::fmt::Debug for dyn AudioProcessor {

src/worklet.rs

@@ -431,6 +431,10 @@ impl<P: AudioWorkletProcessor> AudioProcessor for AudioWorkletRenderer<P> {
     fn onmessage(&mut self, msg: &mut dyn Any) {
         self.processor.load().onmessage(msg)
     }
+
+    fn has_side_effects(&self) -> bool {
+        true // could be IO, message passing, ..
+    }
 }
 
 #[cfg(test)]