1285290 - move code to choose the next GraphDriver to CheckDriver() for Linearize AudioContextOperation resolution

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P3)

Description

[Paul Adenot (:padenot)]

We still have a fast-path when something is still using the AudioCallbackDriver, where we tell the content to resolve the promise immediately without sending it to the GraphDriver.

This causes the events to not stay in order, and blow up the state change assert back in `AudioContext::OnStateChanged`.

Comment 1

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 3

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 5

[Karl Tomlinson (:karlt)]

I'll have a look at this as part of bug 1638243.

Comment 6

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=096f332daa2c77188b9390482817412d9b8f9488

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 set prefs on two crashtests to allow autoplay r?padenot

Apparently this pref is already set when the tests are run on CI,
but it is not being set when running a single test or subdir.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 don't add destroyed tracks to graph from Increment/DecrementSuspendCount r?padenot

Tracks remove themselves from the graph after DestroyImpl() and so should not
be added again after this.

These methods are now invoked only on the MediaTrack removing the possibility
of calling the wrong method.

Depends on D76797

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 move AudioContextOperationControlMessage out of function scope r?padenot

This will permit passing an AudioContextOperationControlMessage* to other
methods.

Depends on D76798

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 alias move() to std::move() in MediaTrackGraph.cpp and AudioContext.cpp r?padenot

Depends on D76799

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 use strong pointers for tracks in ApplyAudioContextOperation() r?padenot

This will be helpful when delaying resumption of tracks until there is an AudioCallbackDriver.

The memory for the array is also transferred to the callee to save an allocation.

Depends on D76800

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 remove some graph-thread LIFECYCLE_RUNNING conditions r?padenot

On the graph thread, mLifecycleState is known to be LIFECYCLE_RUNNING.
OpenAudioInputImpl(), CloseAudioInputImpl(), AddAudioOutputImpl() are called
only from ControlMessage::Run(), not RunDuringShutdown().

These conditions were first added in
https://hg.mozilla.org/mozilla-central/rev/dcd375e2750d425af210fee36962219a97a334fd#l3.12
but I don't know why.

Depends on D76801

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 move code to choose the next GraphDriver to CheckDriver() r?padenot

There is a small logic change here when
!audioTrackPresent && audioCallbackDriver && !IsStarted() &&
graphOutputChannelCount != audioCallbackDriver->OutputChannelCount()
Now, no driver change is scheduled in that situation.

Depends on D76802

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 Document suspend/resume accounting wrt ApplyAudioContextOperation() r?padenot

Depends on D76803

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 introduce PendingResumeOperation r?padenot

Depends on D76804

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 rewrite AudioContextOperation handling using PendingResumeOperation r?padenot

The key change here is that AudioContextOperation promises for the same
AudioContext are resolved in order as long as the graph is running.
There is one remaining case where AudioContextOperation promises can be
resolved out of order, which is when
AudioContextOperationControlMessage::RunDuringShutdown() resolves a Close
operation shortly after Run() has queued its update via mUpdateRunnables.
mUpdateRunnables are run after controlMessagesToRunDuringShutdown.
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/dom/media/MediaTrackGraph.cpp#1793,1803

Pending resume operations are stored on the graph instead of on mNextDriver.
This means there is no need to move operations between drivers when switching
from one mNextDriver to another, the code for which was previously missing.

Suspend operations are performed immediately, because even a callback driver
can render nothing. Tracks are not resumed until either there is an
AudioCallbackDriver to deliver the rendered audio or the Resume operation is
canceled by a subsequent Suspend.

While the graph is running, DispatchToMainThreadStableState() is used
consistently, whereas previously this was mixed with direct Dispatch() to the
main thread, which could have the runnable Run() before main thread state had
been updated.

Depends on D76805

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: Bug 1285290 remove waits for suspend and resume completion added to workaround bug 1198386 r?padenot

Depends on D76807

Comment 18

[Phabricator Automation]

Comment on attachment 9151663 [details]
Bug 1285290 set prefs on two crashtests to allow autoplay r?padenot

Revision D76797 was moved to bug 1641161. Setting attachment 9151663 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9151664 [details]
Bug 1285290 don't add destroyed tracks to graph from Increment/DecrementSuspendCount r?padenot

Revision D76798 was moved to bug 1641161. Setting attachment 9151664 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9151665 [details]
Bug 1285290 move AudioContextOperationControlMessage out of function scope r?padenot

Revision D76799 was moved to bug 1641161. Setting attachment 9151665 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9151666 [details]
Bug 1285290 alias move() to std::move() in MediaTrackGraph.cpp and AudioContext.cpp r?padenot

Revision D76800 was moved to bug 1641161. Setting attachment 9151666 [details] to obsolete.

Comment 22

[Phabricator Automation]

Comment on attachment 9151669 [details]
Bug 1285290 move code to choose the next GraphDriver to CheckDriver() r?padenot

Revision D76803 was moved to bug 1641161. Setting attachment 9151669 [details] to obsolete.

Comment 23

[Phabricator Automation]

Comment on attachment 9151670 [details]
Bug 1285290 Document suspend/resume accounting wrt ApplyAudioContextOperation() r?padenot

Revision D76804 was moved to bug 1641161. Setting attachment 9151670 [details] to obsolete.

Comment 24

[Phabricator Automation]

Comment on attachment 9151668 [details]
Bug 1285290 remove some graph-thread LIFECYCLE_RUNNING conditions r?padenot

Revision D76802 was moved to bug 1641161. Setting attachment 9151668 [details] to obsolete.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1823eb201ef6
move code to choose the next GraphDriver to CheckDriver() r=padenot

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1823eb201ef6

Comment 27

[Karl Tomlinson (:karlt)]

Bug 1644647 tracks remaining work. https://phabricator.services.mozilla.com/D76803 accidentally landed with the wrong bug number, so I'll use this bug to track that change.

Comment 28

[Phabricator Automation]

Comment on attachment 9151667 [details]
Bug 1285290 use strong pointers for tracks in ApplyAudioContextOperation() r?padenot

Revision D76801 was moved to bug 1644647. Setting attachment 9151667 [details] to obsolete.

Comment 29

[Phabricator Automation]

Comment on attachment 9151671 [details]
Bug 1285290 introduce PendingResumeOperation r?padenot

Revision D76805 was moved to bug 1644647. Setting attachment 9151671 [details] to obsolete.

Comment 30

[Phabricator Automation]

Comment on attachment 9151672 [details]
Bug 1285290 rewrite AudioContextOperation handling using PendingResumeOperation r?padenot

Revision D76806 was moved to bug 1644647. Setting attachment 9151672 [details] to obsolete.

Comment 31

[Phabricator Automation]

Comment on attachment 9151674 [details]
Bug 1285290 remove waits for suspend and resume completion added to workaround bug 1198386 r?padenot

Revision D76808 was moved to bug 1644647. Setting attachment 9151674 [details] to obsolete.