Firefox not releasing unreachable nodes

[tambien]

This might not be possible to fix with this library, but i'm facing this issue with Firefox where source nodes that are not connected to the output will still be processed and degrade performance over time (it doesn't seem to be an issue in Chrome). You can test it out yourself by doing something like:

setInterval(() => {
  source = context.createConstantSource()
  source.start(0)
}, 10)

Even though the source is not connected to the larger graph, it will still be processed and eventually degrade the performance. If you watch your OS activity monitor, you can see firefox uses more and more performance over time until it eventually crackles up.

Since i see you've got some graph analysis and connection memory already in the library, i'm wondering if it's possible to stop ConstantSourceNodes when they are no longer reachable in the audio graph? and restart them when they become connected. You'd have to replace the node and connections when you restart it since ConstantSourceNodes are single use, but it would fix this nasty performance bug. I don't think this type of thing would be possible with OscillatorNodes or BufferSourceNodes since offset and phase are big issues there.

The solution that i was thinking of would be to maintain a list of active (started) ConstantSourceNodes, and occasionally check if it's still reachable to the destination node by iterating over the tree of connected nodes using get-audio-node-connections and get-audio-param-connections to see if it reaches the destination. if it does not, it can be stopped and removed from the active source list which will also free it up for GC if there are no other references to that node. If there is still a reference to that node then restart it when there is a new connection attached and it becomes reachable again.

I realize that this is quite complicated for an optimization, but i've found this memory leak to cause quite a few performance issues. Also i totally understand if this is not something that you think this library should tackle, feel free to close. i might be able to implement something like this on my side of things using the APIs that you expose.

[tambien]

Was thinking about something like this for checking reachability:

function reachesNode(current: IAudioNode<any>, goal: IAudioNode<any>): boolean {
	if (current === goal) {
		return true;
	} else {
		const connections = getAudioNodeConnections(current);
		return Array.from(connections.outputs).some(connection => {
			if (isAnyAudioParam(connection[0])) {
				// get node belonging to param
				const node = AUDIO_PARAM_AUDIO_NODE_STORE.get(connection[0]);
				if (node) {
					return reachesNode(node, goal);
				}
			} else {
				return reachesNode(connection[0], goal);
			}
		});
	}
}

export function reachesDestination(node: IAudioNode<any>): boolean {
	return reachesNode(node, node.context.destination);
}

[chrisguttandin]

Hi Yotam, I think this is an excellent idea.

Chrome and Firefox have a different approach when it comes to rendering the graph. Chrome's implementation also has some edge cases. When an AudioWorklet (or a ScriptProcessor) has no output it never gets called.

https://github.com/chrisguttandin/standardized-audio-context/blob/master/test/expectation/chrome/any/audio-worklet-node.js#L51

https://github.com/chrisguttandin/standardized-audio-context/blob/master/test/expectation/chrome/any/offline-audio-context-constructor.js#L49

But as you already mentioned a fix would only be possible for the ConstantSourceNode and I think this would be even more confusing if it behaves differently compared to the other nodes that produce sound.

What would you think about creating a dedicated dev mode. Which either throws errors or logs warnings when someone uses suspicious patterns. That would make it all less magical and it would also avoid to add runtime checks which run in production code.

I had creating something like this on my todo list for a long time. It could for example also warn when someone calls start() with a value below currentTime because this is most likely unintended ... And there are probably many other things that could be covered as well.