Closed Bug 1285290 Opened 4 years ago Closed 3 months ago

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

Categories

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

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: padenot, Assigned: karlt)

References

Details

Attachments

(1 file, 10 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
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`.
Blocks: 1285060
Rank: 15
Priority: -- → P1
Assignee: nobody → padenot
Blocks: 1283056
Blocks: 1332033
Blocks: 1305136
Duplicate of this bug: 1375494
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
See Also: → 1375562

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

Assignee: padenot → karlt
Status: NEW → ASSIGNED
Keywords: stale-bug
See Also: → 1269741

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.

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

This will permit passing an AudioContextOperationControlMessage* to other
methods.

Depends on D76798

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

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

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

Depends on D76804

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

Depends on: 1641161

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.

Attachment #9151663 - Attachment is obsolete: true

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.

Attachment #9151664 - Attachment is obsolete: true

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.

Attachment #9151665 - Attachment is obsolete: true

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.

Attachment #9151666 - Attachment is obsolete: true

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.

Attachment #9151669 - Attachment is obsolete: true

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.

Attachment #9151670 - Attachment is obsolete: true

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.

Attachment #9151668 - Attachment is obsolete: true
Attachment #9151669 - Attachment is obsolete: false
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1823eb201ef6
move code to choose the next GraphDriver to CheckDriver() r=padenot
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Blocks: 1644647

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.

No longer blocks: 1283056, 1285060, 1305136, 1332033, 1644647, 1371177
See Also: 1375562, 1269741
Summary: Linearize AudioContextOperation resolution → move code to choose the next GraphDriver to CheckDriver() for Linearize AudioContextOperation resolution
Blocks: 1644647

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.

Attachment #9151667 - Attachment is obsolete: true

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

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

Attachment #9151671 - Attachment is obsolete: true

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.

Attachment #9151672 - Attachment is obsolete: true

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.

Attachment #9151674 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.