923301 - keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

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.

Comment 1

[Karl Tomlinson (:karlt)]

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.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: part 3: keep a tail-time reference on DelayNode until *after* the last non-silent block has been produced

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 4

[Karl Tomlinson (:karlt)]

Attachment: part 4: remove now-unnecessary AcceptPlayingRefRelease

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: part 6: remove now-unnecessary node-type templating of PlayingRefChangeHandler

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: send only one release message when delay buffer is drained

Comment 7

[Karl Tomlinson (:karlt)]

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.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: send only one release message when convolver tail time expires

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: skip delay processing when nothing is buffered and input is null

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 10: skip reverb processing when input has been null long enough for output to be null

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 11

[(no longer active)]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: part 1: add MediaStreamGraph::RunAfterPendingUpdates()

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.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: part 2: keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary v2

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: part 5: remove now-unused GetCurrentGraphUpdateIndex() and rename mGraphUpdatesSent to mNextGraphUpdateIndex

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: part 11: use MediaStreamGraph to dispatch PlayingRefChangeHandlers

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

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: part 2: keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary v2.1

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

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: part 2: keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary v2.2

And the runnable is not a message now.

Comment 19

[Karl Tomlinson (:karlt)]

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

Comment 20

[Karl Tomlinson (:karlt)]

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

Comment 21

[Karl Tomlinson (:karlt)]

Missed some PlayingRefChangeHandler dispatches in part 11, so moved them to also use DispatchToMainThreadAfterStreamStateUpdate:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72efd8463aa0

Comment 22

[Carsten Book [:Tomcat]]

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 23

[Karl Tomlinson (:karlt)]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0929b541062