1754006 - Increase the processed media queue size and threshold in AudioSink

Status: RESOLVED FIXED

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

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1754006 - Increase the processed media queue size and threshold in AudioSink. r?alwu

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1754006 - Add profiler markers in push/pop operations on the AudioSink ring buffer. r?alwu

Depends on D138028

Comment 3

[Alastor Wu [:alwu]]

Is the reason of increasing the size of SPSC queue that we want to make sure the audio stream won't be starved (as much as possible) when MDSM is entering the extreme situation decribed in your document (thread is still waiting for next scheduling)?

However, from my observation, when I play audio (on MacBook M1) under $ stress-ng --cpu 32 --bigheap 32, the SPSC queue is usually used around only 50% usage, it still has plenty of room to store audio, so I'm not sure adding more spaces would be helpful.

I also got an interesting profile in which you can see a gap happening around 65s. That gap is different from Bobby's profile, because it seems caused by NativeAudioCallback. You can see there are two NativeAudioCallback, the first one stops around 65s and another one starts after that, which seems a cause for the gap.

I feel like what happened in Bobby's profile is that, propbably our audio clock wasn't updated in time (I guess? because I'm not familiar with how we get the clock from the audio IPC) First, the thread scheduling was already slow under that extreme situation, so the checking happens in MDSM won't be performed frequently. If the clock time are updated too slow, because it requires getting time from remote audio process. The slow clock time would further affect the both rendering video and requesting audio decode tasks. For video, we calculate the delta based on the audio clock to deteminine the time of rendering next video. For audio, we calculate decoded audio duration based on the audio clock to see if we still need to dispatch an audio decoding task.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/078521a5c82b
Increase the processed media queue size and threshold in AudioSink. r=alwu
https://hg.mozilla.org/integration/autoland/rev/f22af4b15704
Add profiler markers in push/pop operations on the AudioSink ring buffer. r=alwu

Comment 5

[Narcis Beleuzu [:NarcisB]]

Backed out for mda failures on test_bug1113600.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/66d3c5ef6d4b596269e76d97078b78883c1ad6c7
Log link: https://treeherder.mozilla.org/logviewer?job_id=367089707&repo=autoland&lineNumber=2320

Comment 6

[Narcis Beleuzu [:NarcisB]]

Please also check: https://treeherder.mozilla.org/logviewer?job_id=367094282&repo=autoland&lineNumber=36213

Comment 7

[Paul Adenot (:padenot)]

(In reply to Alastor Wu [:alwu] from comment #3)

Is the reason of increasing the size of SPSC queue that we want to make sure the audio stream won't be starved (as much as possible) when MDSM is entering the extreme situation decribed in your document (thread is still waiting for next scheduling)?

However, from my observation, when I play audio (on MacBook M1) under $ stress-ng --cpu 32 --bigheap 32, the SPSC queue is usually used around only 50% usage, it still has plenty of room to store audio, so I'm not sure adding more spaces would be helpful.

Yes, this is to handle extreme cases. Most of the time, it's fine indeed. It's clear that we're in the presence of an spsc under-run because there are markers when that happens, and they are present in the profile.

I also got an interesting profile in which you can see a gap happening around 65s. That gap is different from Bobby's profile, because it seems caused by NativeAudioCallback. You can see there are two NativeAudioCallback, the first one stops around 65s and another one starts after that, which seems a cause for the gap.

I'm not sure what's up with that. It seems caused by having logging enabled in the AudioSink. Regular MOZ_LOG is clearly not real-time safe (quite the opposite in fact).

I feel like what happened in Bobby's profile is that, propbably our audio clock wasn't updated in time (I guess? because I'm not familiar with how we get the clock from the audio IPC).

But we see a request for decoding happening, and it's taking 600+ms instead of a few microseconds, right? Since task queue runnables need to be resolved in order (which makes sense), it's essentially like head-of-line blocking: something takes too long to finish, and everything is delayed.

First, the thread scheduling was already slow under that extreme situation, so the checking happens in MDSM won't be performed frequently. If the clock time are updated too slow, because it requires getting time from remote audio process. The slow clock time would further affect the both rendering video and requesting audio decode tasks.

We might want to fix this indeed: https://searchfox.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#2907-2911. It's unnecessary to have it based on the clock. We can just sum the various queues (audio queue, spsc queue).

For video, we calculate the delta based on the audio clock to deteminine the time of rendering next video. For audio, we calculate decoded audio duration based on the audio clock to see if we still need to dispatch an audio decoding task.

Again, we see a task dispatched, but it's taking too long. Immediately after, lots of tasks are dispatched and resolved quickly.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Paul Adenot (:padenot) from comment #7)

(In reply to Alastor Wu [:alwu] from comment #3)

I feel like what happened in Bobby's profile is that, propbably our audio clock wasn't updated in time (I guess? because I'm not familiar with how we get the clock from the audio IPC).

Also this profile was with the default state of audioipc disabled. It's just a read from a triple-buffer data structure (which is non-blocking), and a mach_absolute_time(): https://searchfox.org/mozilla-central/source/third_party/rust/cubeb-coreaudio/src/backend/mod.rs#3484-3520

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af366e7b6233
Increase the processed media queue size and threshold in AudioSink. r=alwu
https://hg.mozilla.org/integration/autoland/rev/f6e4e5fac9b4
Add profiler markers in push/pop operations on the AudioSink ring buffer. r=alwu

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/af366e7b6233
https://hg.mozilla.org/mozilla-central/rev/f6e4e5fac9b4

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 9262673 [details]
Bug 1754006 - Increase the processed media queue size and threshold in AudioSink. r?alwu

Beta/Release Uplift Approval Request

• User impact if declined: Glitches under load on M1-based macOS
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is simply tweaking the size of a buffer, and making it configurable via a pref -- we can trivially revert to the previous behavior by changing this pref, but it does make things better, when testing.
• String changes made/needed:

Comment 12

[Paul Adenot (:padenot)]

[Tracking Requested - why for this release]:

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9262673 [details]
Bug 1754006 - Increase the processed media queue size and threshold in AudioSink. r?alwu

Approved for 98 beta 7, thanks.

Comment 14

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f86a3207fc2
https://hg.mozilla.org/releases/mozilla-beta/rev/3fb60c8e50b6