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

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
Web Audio
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(11 attachments, 3 obsolete attachments)

1.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.99 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.58 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.60 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 813363 [details] [diff] [review]
keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Attachment #813363 - Flags: review?(roc)
(Assignee)

Comment 3

4 years ago
Created attachment 813388 [details] [diff] [review]
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.
Attachment #813388 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 813393 [details] [diff] [review]
part 4: remove now-unnecessary AcceptPlayingRefRelease
Attachment #813393 - Flags: review?(roc)
(Assignee)

Comment 5

4 years ago
Created attachment 813394 [details] [diff] [review]
part 6: remove now-unnecessary node-type templating of PlayingRefChangeHandler
Attachment #813394 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
Created attachment 813396 [details] [diff] [review]
send only one release message when delay buffer is drained
Attachment #813396 - Flags: review?(roc)
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
Created attachment 813398 [details] [diff] [review]
send only one release message when convolver tail time expires
Attachment #813398 - Flags: review?(roc)
(Assignee)

Comment 9

4 years ago
Created attachment 813399 [details] [diff] [review]
skip delay processing when nothing is buffered and input is null
Attachment #813399 - Flags: review?(roc)
(Assignee)

Comment 10

4 years ago
Created attachment 813400 [details] [diff] [review]
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.
Attachment #813400 - Flags: review?(roc)

Comment 11

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 923319
(Assignee)

Updated

4 years ago
Blocks: 924288
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.
Attachment #813363 - Flags: review?(roc) → review-
(Assignee)

Updated

4 years ago
Depends on: 914016
(Assignee)

Comment 13

4 years ago
Created attachment 820829 [details] [diff] [review]
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.
Attachment #813363 - Attachment is obsolete: true
Attachment #820829 - Flags: review?(roc)
(Assignee)

Comment 14

4 years ago
Created attachment 820830 [details] [diff] [review]
part 2: keep disconnected destination nodes alive long enough for their engines to add tail-time references if necessary v2
Attachment #820830 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #813388 - Attachment description: keep a tail-time reference on DelayNode until *after* the last non-silent block has been produced → part 3: keep a tail-time reference on DelayNode until *after* the last non-silent block has been produced
(Assignee)

Updated

4 years ago
Attachment #813393 - Attachment description: remove now-unnecessary AcceptPlayingRefRelease → part 4: remove now-unnecessary AcceptPlayingRefRelease
(Assignee)

Comment 15

4 years ago
Created attachment 820833 [details] [diff] [review]
part 5: remove now-unused GetCurrentGraphUpdateIndex() and rename mGraphUpdatesSent to mNextGraphUpdateIndex
Attachment #820833 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #813394 - Attachment description: remove now-unnecessary node-type templating of PlayingRefChangeHandler → part 6: remove now-unnecessary node-type templating of PlayingRefChangeHandler
(Assignee)

Updated

4 years ago
Attachment #813400 - Attachment description: skip reverb processing when input has been null long enough for output to be null → part 10: skip reverb processing when input has been null long enough for output to be null
(Assignee)

Comment 16

4 years ago
Created attachment 820836 [details] [diff] [review]
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.
Attachment #820836 - Flags: review?(roc)
(Assignee)

Comment 17

4 years ago
Created attachment 820841 [details] [diff] [review]
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.
Attachment #820830 - Attachment is obsolete: true
Attachment #820830 - Flags: review?(roc)
Attachment #820841 - Flags: review?(roc)
(Assignee)

Comment 18

4 years ago
Created attachment 820852 [details] [diff] [review]
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.
Attachment #820841 - Attachment is obsolete: true
Attachment #820841 - Flags: review?(roc)
Attachment #820852 - Flags: review?(roc)
(Assignee)

Comment 21

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

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

Comment 23

4 years ago
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.
Attachment #820836 - Flags: approval-mozilla-beta?
Attachment #820836 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Whiteboard: [qa-]

Updated

4 years ago
Depends on: 939491
(Assignee)

Updated

4 years ago
No longer depends on: 939491
(Assignee)

Updated

3 years ago
Depends on: 1144189
You need to log in before you can comment on or make changes to this bug.