1829054 - Resume decoded audio when device is replugged after setSinkId()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P1)

Description

[Karl Tomlinson (:karlt)]

Attachment: testcase

+++ This bug was initially created as a clone of Bug #1801190 +++

https://w3c.github.io/mediacapture-output/#algorithms-new-sink-available

New audio devices may become available to the user agent, or an audio device (identified by a media element's sinkId attribute) that had previously become unavailable may become available again, for example, if it is unplugged and later plugged back in.

In this scenario, the user agent must run the following steps:

1. Let sinkId be the identifier for the newly available device.
2. For each media element whose sinkId attribute is equal to sinkId:
3. If the media element's paused attribute is false, start rendering this object's audio out of the device represented by the sinkId attribute.

STR:

1. Set pref media.setsinkid.enabled true (unnecessary on Nightly, as this is the default).
2. Load testcase.
3. Play video.
4. Click "Default system output..."
5. Select a removable speaker device.
6. Click "Allow".
7. Remove and re-plug removable speaker device.

Expect: audio in speaker selected in 5.
Actual: no audio.

Comment 1

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/jobs?repo=try&revision=2b2b1bad099fc408b525c5a184575190e43d8d95

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 remove declaration of non-existent AudioStream::Reset r?kinetik

Gone since https://hg.mozilla.org/mozilla-central/rev/b12b9840d67b#l1.669

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 use std::function to reduce templating and indirection of AudioSink creation r?padenot

Depends on D180650

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 store new AudioSink in a UniquePtr immediately r?padenot

to remove risk of leak.

Depends on D180651

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 clarify the Start() to Stop() lifetime of AudioSinkWrapper::mEndedPromise r?padenot

Move Ensure() to Start() to clarify that muting and SetPlaying() should not
create a different promise for clients to watch.

Move ResolveIfExists() from Shutdown() to Stop() to make the behavior when
muted consistent with behavior when unmuted and with DecodedStream and
VideoSink.

Shutdown() asserts that Stop() has been called.

Depends on D180652

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 document and rename some AudioSinkWrapper state variables r?padenot

This naming uses "position" instead of "duration" to clarify that this does
not include remaining playback duration.

mPlayStartTime was reset on playback rate changes and muting changes, so was
not necessarily the start of playback.

Depends on D180653

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 reset mClockStartTime and mPositionAtClockStart on Stop() r?padenot

for consistency with state before start and to simplify IsPlaying().

Depends on D180654

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 decide whether an AudioSink should be used in a single method r?padenot

SetPlaying() and asynchronous sink creation no longer starts an AudioSink if
audio has ended.

The ended test with OnMuted() is now effective even when audio ended while
using system time for the clock, when mAudioEnded does not get set.

Depends on D180655

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 use a shared helper method to start and listen to AudioSink r?padenot

Tracking of the AudioSink's promise is now only performed if the AudioSink
successfully starts.

The existing mEndedPromiseHolder.RejectIfExists() does not behave as expected
because the AudioStream::Start() steals the promise even if it fails. This
will be addressed in the next patch to use separate ended promises for
AudioSink and AudioSinkWrapper.

Depends on D180656

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 use a separate promise to track AudioSink and AudioSinkWrapper ended r?padenot

In a future patch this will allow the AudioSinkWrapper to remain unended when
the AudioSink ends due to the device becoming unavailable, and to reopen
another AudioSink when the device becomes available again.

Re-enqueuing unplayed audio even when not required is favored over
the complexity of an otherwise unnecessary parameter.

Depends on D180657

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 rename mAudioSinkEndedPromise to mAudioSinkEndedRequest r?padenot

This helps clarify the difference between mEndedPromiseHolder managed by
AudioSinkWrapper and mAudioSinkEndedRequest watching a promise managed by the
AudioSink/AudioStream.

Depends on D180658

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 split StartAudioSink into sync and async methods r?padenot

This clarifies that the aStartTime parameter is not used in the async method.

Depends on D180659

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 share a helper method to shut down the AudioSink r?padenot

This will simplify keeping track of a previous AudioSink's end time when
switching between clocks. OnAudioEnded() will also use this helper in a
future patch.

Depends on D180660

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 use last queued packet end time when no AudioSink is in use r?padenot

This last packet time is similar to the upper limit used with the return value
from AudioSink::GetEndTime(). It also now correctly returns zero, even when
muted (tested in test_background_video_resume_after_end_show_last_frame.html),
if there is no audio track, as documented in MediaSink.h.

Returning an advancing time was leading to incorrect durations when muted, as
now tested in test_seek_duration.html. MediaSink::GetEndTime() also needs to
cap the system clock so MDSM will not advance playback position beyond the end
of media. https://bugzilla.mozilla.org/show_bug.cgi?id=1386478#c6

Zero is no longer returned when the AudioSink callbacks have not yet started,
which would have the effect of briefly not advancing the current playback
position of a playing audio element when unmuting.

Depends on D180661

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Bug 1829054 re-try explicit audio device periodically after stream failure r?padenot

to keep current playback position progressing and to resume audio output on
replugging a removed device.

Depends on D180662

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1638fd63ec28
remove declaration of non-existent AudioStream::Reset r=kinetik
https://hg.mozilla.org/integration/autoland/rev/1cce87bd67c5
use std::function to reduce templating and indirection of AudioSink creation r=padenot
https://hg.mozilla.org/integration/autoland/rev/b7125a21f04f
store new AudioSink in a UniquePtr immediately r=padenot
https://hg.mozilla.org/integration/autoland/rev/2fbfefb8c6cf
clarify the Start() to Stop() lifetime of AudioSinkWrapper::mEndedPromise r=padenot

Comment 17

[Sandor Molnar[:smolnar]]

Backed out for causing mda failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/2bdaa9d6ac390362734ba199115ca10186243e87

Push with failures

Failure log -> PROCESS-CRASH | MOZ_ASSERT(mRawPtr != nullptr) (You can't dereference a NULL RefPtr with operator->().) [@ libxul.so + 0x00000000007a93bf] | dom/media/mediasource/test/mochitest

Comment 18

[Karl Tomlinson (:karlt)]

Thank you for flagging me.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd4296891041
remove declaration of non-existent AudioStream::Reset r=kinetik
https://hg.mozilla.org/integration/autoland/rev/2cee8a5d0209
use std::function to reduce templating and indirection of AudioSink creation r=padenot
https://hg.mozilla.org/integration/autoland/rev/3661d47fd7f7
store new AudioSink in a UniquePtr immediately r=padenot

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fd95d83faf1
document and rename some AudioSinkWrapper state variables r=padenot
https://hg.mozilla.org/integration/autoland/rev/69915e513481
reset mClockStartTime and mPositionAtClockStart on Stop() r=padenot

Comment 21

[Karl Tomlinson (:karlt)]

https://phabricator.services.mozilla.com/D180653 needs to land with https://phabricator.services.mozilla.com/D180656 to avoid failures in dom/media/mediasource/test/test_BufferingWait.html, dom/media/test/test_background_video_ended_event.html, and dom/media/test/test_reset_src.html as reported in comment 17 and now documented in the commit message.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ef69e37cbf5
clarify the Start() to Stop() lifetime of AudioSinkWrapper::mEndedPromise r=padenot
https://hg.mozilla.org/integration/autoland/rev/d27814dbcf7f
decide whether an AudioSink should be used in a single method r=padenot

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81468f8b39fd
use a shared helper method to start and listen to AudioSink r=padenot
https://hg.mozilla.org/integration/autoland/rev/203bf275e99e
use a separate promise to track AudioSink and AudioSinkWrapper ended r=padenot

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/039e59ad276e
rename mAudioSinkEndedPromise to mAudioSinkEndedRequest r=padenot
https://hg.mozilla.org/integration/autoland/rev/449ecd31e049
split StartAudioSink into sync and async methods r=padenot

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bd4296891041
https://hg.mozilla.org/mozilla-central/rev/2cee8a5d0209
https://hg.mozilla.org/mozilla-central/rev/3661d47fd7f7
https://hg.mozilla.org/mozilla-central/rev/9fd95d83faf1
https://hg.mozilla.org/mozilla-central/rev/69915e513481

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b9574ef0cc1
share a helper method to shut down the AudioSink r=padenot

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecdb4d9d26dc
use last queued packet end time when no AudioSink is in use r=padenot

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5ef69e37cbf5
https://hg.mozilla.org/mozilla-central/rev/d27814dbcf7f
https://hg.mozilla.org/mozilla-central/rev/81468f8b39fd
https://hg.mozilla.org/mozilla-central/rev/203bf275e99e
https://hg.mozilla.org/mozilla-central/rev/039e59ad276e
https://hg.mozilla.org/mozilla-central/rev/449ecd31e049
https://hg.mozilla.org/mozilla-central/rev/2b9574ef0cc1
https://hg.mozilla.org/mozilla-central/rev/ecdb4d9d26dc

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ca8d0459e42
re-try explicit audio device periodically after stream failure r=padenot

Comment 30

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4ca8d0459e42

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 32

[Dianna Smith [:diannaS]]

:Karlt is this something you would like to nominate for 116 release notes?

Comment 33

[Karl Tomlinson (:karlt)]

The key change here will come into effect on release only when bug 1498512 is fixed, so that's the one to watch for release notes, thank you.