1. MSG thread: a source node S starts playing. 2. MSG thread: a downstream, possibly indirectly connected node D sees sound start and sends a PlayingRefChanged(ADDREF) message to the main thread to take a tail time reference on node D. 3. main thread: one of the links between the source node and the downstream node is disconnected. 4. main thread: cycle collection happens or unref triggers destruction of nodes downstream from the disconnect, including node D. 5. main thread: PlayingRefChanged(ADDREF) message is processed but node D is no longer.

I think the best solution here is that explicit disconnections will only remove references from output nodes after the graph is notified and the main thread receives a reply. Similarly, nodes with playing or tail-time references release these references only after receiving notification from their engine on the graph thread that playing has stopped. Engines notifying the main thread that they have finished do so strictly *after* producing and returning their last block. In this way, an engine that receives non-null input knows that the input comes from nodes that are still alive and will keep their output nodes alive for at least as long as it takes to process messages from the graph thread. i.e. the engine receiving non-null input knows that its node is still alive, and will still be alive when it receives a message from the engine. This produces better behavior with cycles than the ideas in bug 898291 comment 7. It is the Engines that know whether there is sound in the cycle. Also, engines need only send one tail-time RELEASE message from to their node because tail-time ADDREF messages are also from engine to node. The "disconnect" refs are handled separately, and so there are no ordering difficulties as when sharing a self-reference (bug 890072 comment 3). Another alternative I considered was to call downstream during explicit disconnect to find nodes that want to know. Only those node types would need to temporarily keep a self reference. Calling all downstream nodes on a single disconnect is expensive, leading to possible O(n^2) scenarios. Multiple downstream nodes could need to handle a single upstream disconnect, resulting in more messages in some situations. (In reply to Karl Tomlinson (:karlt) from bug 898291 comment #7) > We can take the self reference from the Node when an input is added. ADDREF > should not be happening from an event. The Engine would not send RELEASE > until the last input has been removed and buffered samples have been output. This would be difficult to make work in a way that silent cycles could be collected. Some other system would need to detect such cycles and remove the self references. Bug 881959 comment 19 means that cycles that will never produce sound are possible. > A variation we discussed was taking the self reference when the last input is > removed. This would not keep cycles alive. This is too late to add the reference. Consider the case where node D in comment 0 is replaced with a cycle. The last input is never removed from nodes in the cycle. Generally, adding references when upstream nodes are implicitly disconnected during GC/CC is not feasible because the downstream nodes may be part of a javascript (not graph) cycle that is being collected.

This undoes changes from bug 872635, which are no longer necessary now that it is fixed properly in bug 910171. The risk is that cycle collection or unref-triggered destruction happens between the RELEASE from the upstream node and ADDREF from the downstream node. I haven't managed to trigger this from a test because of lack of resolution in graph state notification events. ScriptProcessorNode has a minimum resolution of 256 but we need notification when one 128 sample block is processed. "ended" events have the necessary precision, but they are out of order because they are implemented with an additional dispatch to main thread. (Perhaps DispatchFromMainThreadAfterNextStreamStateUpdate should be implemented, or perhaps onended should be implemented using DispatchToMainThreadAfterStreamStateUpdate and AddListener instead of AddMainThreadListener. The latter option could help testing this if reference messages also went through DispatchToMainThreadAfterStreamStateUpdate. That would actually be more efficient than separate main thread events. I assume DispatchToMainThreadAfterStreamStateUpdate could be made to work from ProcessedMediaStream, if it doesn't already.) The playedBackAllLeftOvers is now not true as early as it could be, but destroying and re-initializing the DelayProcessor on every iteration is not saving anything anyway. I'll fix this in a subsequent patch.

Comment on attachment 813396 [details] [diff] [review] send only one release message when delay buffer is drained >+ } else { >+ if (mLeftOverData != INT32_MIN) { I haven't merged this into and else if because a subsequent patch will use the else.

This doesn't get the null output channel count right, but the channel count of null output is not often observable and is a general issue for bug 916392.

Comment on attachment 813363 [details] [diff] [review] keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary Review of attachment 813363 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioNode.cpp @@ +7,5 @@ > #include "AudioNode.h" > #include "mozilla/ErrorResult.h" > #include "AudioNodeStream.h" > #include "AudioNodeEngine.h" > +#include "MediaStreamGraphImpl.h" Not sure if roc will like this. ;-) @@ +303,5 @@ > dest->mInputNodes.RemoveElementAt(j); > // Remove one instance of 'dest' from mOutputNodes. There could be > // others, and it's not correct to remove them all since some of them > // could be for different output ports. > + auto message = new RoundTripReleaseMessage(mOutputNodes[i].forget()); Please use an nsRefPtr here. Otherwise, this object will leak if AppendMessage() never AddRef/Release's it.

Comment on attachment 813363 [details] [diff] [review] keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary Review of attachment 813363 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioNode.cpp @@ +275,5 @@ > + // engine for a downstream node may be sending a PlayingRefChangeHandler > + // ADDREF message to this (main) thread. Wait for a round trip before > + // releasing nodes, to give engines receiving sound now time to keep their > + // nodes alive. > + class RoundTripReleaseMessage : public ControlMessage { Yeah, let's not extend ControlMessage here. Instead, add an API to MediaStreamGraph that lets you run an arbitrary runnable on the graph thread after all state updates have been applied.

I don't want to add a new kind of runnable to the graph thread that runs out of order with the existing ControlMessage runnables, and piggybacking one kind of runnable on another that looks the same is not sounding ideal. The only improvement that I could make to the ControlMessage class would be to force some kind of move semantics to make it clear that the class is not ref-counted, and perhaps remove the MediaStream parameter. I could go through AudioNodeStream to use ControlMessage like Send*Parameter*ToStream(). However, I also need this event to run and release the reference even when the MSGI has not started. My old patch got that wrong. ControlMessage wants to store a MediaStream and so is easier to use with a MediaStream, and the client has a MediaStream rather than a Graph, so I've put a method on MediaStream, even though it really operates on the graph.

This goes with part 2 really, but then I'd need to rewrite many of the subsequent patches.

This version makes it clearer that RunAfterPendingUpdates() takes ownership.

And the runnable is not a message now.

https://tbpl.mozilla.org/?tree=Try&rev=e723e9dff389 https://tbpl.mozilla.org/?tree=Try&rev=246ec6c74586

https://hg.mozilla.org/integration/mozilla-inbound/rev/aee15d62fc00 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe16e582db9f https://hg.mozilla.org/integration/mozilla-inbound/rev/9ced7065dec4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1a12845a97ce https://hg.mozilla.org/integration/mozilla-inbound/rev/42bb49c92705 https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0a112e86d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/5be14790a6ff https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1ebb917518 https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf3f03f869c https://hg.mozilla.org/integration/mozilla-inbound/rev/4690758bd1e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b415b13afe3a

Missed some PlayingRefChangeHandler dispatches in part 11, so moved them to also use DispatchToMainThreadAfterStreamStateUpdate: https://hg.mozilla.org/integration/mozilla-inbound/rev/72efd8463aa0

https://hg.mozilla.org/mozilla-central/rev/aee15d62fc00 https://hg.mozilla.org/mozilla-central/rev/fe16e582db9f https://hg.mozilla.org/mozilla-central/rev/9ced7065dec4 https://hg.mozilla.org/mozilla-central/rev/1a12845a97ce https://hg.mozilla.org/mozilla-central/rev/42bb49c92705 https://hg.mozilla.org/mozilla-central/rev/0b0a112e86d9 https://hg.mozilla.org/mozilla-central/rev/5be14790a6ff https://hg.mozilla.org/mozilla-central/rev/5c1ebb917518 https://hg.mozilla.org/mozilla-central/rev/ecf3f03f869c https://hg.mozilla.org/mozilla-central/rev/4690758bd1e7 https://hg.mozilla.org/mozilla-central/rev/b415b13afe3a https://hg.mozilla.org/mozilla-central/rev/72efd8463aa0

Comment on attachment 820836 [details] [diff] [review] part 11: use MediaStreamGraph to dispatch PlayingRefChangeHandlers Please consider this a request for all changesets in comment 20 and 21, but the test in b415b13afe3a cannot be enabled on Beta as is because it uses AudioBuffer.copyFromChannel which is only available in 27. content/media/webaudio/test/mochitest.ini doesn't exist on Beta, so the changeset won't enable the test when applied. [Approval Request Comment] Bug caused by (feature/regressing bug #): Web Audio User impact if declined: A significant perform issue has been identified in bug 923319 and 929595 that can degrade some Web Audio demos to the point of being a crunchy mess. The demo in bug 929595 has been modified to mostly workaround the issue, but there likely will be other situations affected. The patches here and in bug 898291 allow unnecessary computation to be skipped, which can make the difference between success and complete failure in some demos on many systems. Testing completed (on m-c, etc.): on m-c Aurora, in testsuite. Risk to taking this patch (and alternatives if risky): There is a moderate amount of code changed here and so moderate risk but limited to Web Audio. We believe that the significant performance improvement is worth making these changes. String or IDL/UUID changes made by this patch: none.

https://hg.mozilla.org/releases/mozilla-beta/rev/7e263b400d76 https://hg.mozilla.org/releases/mozilla-beta/rev/3f314c71835c https://hg.mozilla.org/releases/mozilla-beta/rev/6b200cbb2eba https://hg.mozilla.org/releases/mozilla-beta/rev/28a150c03e63 https://hg.mozilla.org/releases/mozilla-beta/rev/c351f02b3b3e https://hg.mozilla.org/releases/mozilla-beta/rev/3955e034eda3 https://hg.mozilla.org/releases/mozilla-beta/rev/21afec9163f2 https://hg.mozilla.org/releases/mozilla-beta/rev/0210c2621025 https://hg.mozilla.org/releases/mozilla-beta/rev/715efe54a860 https://hg.mozilla.org/releases/mozilla-beta/rev/ea75550e7149 https://hg.mozilla.org/releases/mozilla-beta/rev/27dc756f1d92 https://hg.mozilla.org/releases/mozilla-beta/rev/87184cd10353

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7e263b400d76 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3f314c71835c https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6b200cbb2eba https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/28a150c03e63 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c351f02b3b3e https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3955e034eda3 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/21afec9163f2 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0210c2621025 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/715efe54a860 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ea75550e7149 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/27dc756f1d92 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/87184cd10353