Closed Bug 1456115 Opened 2 years ago Closed 2 years ago

Make some code called on the MSG thread a bit more real-time safe

Categories

(Core :: WebRTC: Audio/Video, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: padenot, Assigned: padenot)

Details

Attachments

(9 files, 1 obsolete file)

This bug is going to have a few patches to aim at making the overall audio quality when doing calls better.
Assignee: nobody → padenot
Rank: 10
Priority: -- → P2
Attached image normal.png
Using dom/media/Tracing.h, this is what we have for a "normal" MSG iteration. This is a six people call (five callers + the callee).

Time goes from left to right, the number of milliseconds is at the top.

The budget we have to render some audio is the blue bar. The time it took this time is the green/gray bar at the top.

There are multiple lanes:
- 0 and 1627xxxx is the audio callback thread, I've separated the TrackUnionStream::ProcessInput calls for unrelated reasons (there are small green events on the right, there are a lot of them)
- 1 is the budget (not a thread per so)
- 13xxxx is the main thread, we trace the stable state runnables
- The long numbers are other threads (it's a hash of the thread id) off which we run NetEQ.

Here, we have a iteration where things go according to plans: we need to pull 10ms of inbound audio for two peers, everything happens on time, we're well in our budget.
Attached image scheduling.png
This exposes a possible source of high load: despite having dispatched the runnables, the OS scheduler does not schedule our NotifyPull until much later, the audio callback waits unnecessarily.

On this very iteration, we're lucky and are still in our rendering budget, though.
Attached image locking.png
This image shows another issue: the NotifyPull call is being scheduled on time, but something takes a very long time inside `NotifyInputImpl`. Usually, on my machine, it takes around 0.1ms. Here, it is 10ms, clearly destroying our budget.
Attached image rtptimestamp.png
This shows an example of what the patches above solve: we need the sample-rate at which the decoder gives us the audio to do some timestamp calculation [0].

Because of that, the code gets the full decoder output format, and only uses the sample-rate, but it still takes a while to do so, because another thread has the lock and probably does not have the CPU.

[0]: https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/voice_engine/channel.cc#953
Comment on attachment 8970208 [details]
Bug 1456115 - Stop locking when getting the sampling rate from the channel in acm_receiver.cc.

https://reviewboard.mozilla.org/r/239004/#review244718

::: media/webrtc/trunk/webrtc/modules/audio_coding/acm2/acm_receiver.h:287
(Diff revision 1)
>    CallStatistics call_stats_ GUARDED_BY(crit_sect_);
>    NetEq* neteq_;
>    Clock* clock_;  // TODO(henrik.lundin) Make const if possible.
>    bool resampled_last_output_frame_ GUARDED_BY(crit_sect_);
>    rtc::Optional<int> last_packet_sample_rate_hz_ GUARDED_BY(crit_sect_);
> +  std::atomic<int> receive_format_frequency_;

I take it we might have to resample the packet data and so we can't assume that last_packet_sample_rate_hz_ will be the same as last_audio_format_->clockrate_hz? If that were not the case, you could just make that variable atomic and avoid adding something new.

Please rename this variable last_audio_format_clockrate_hz for consistency.
Attachment #8970208 - Flags: review?(dminor) → review+
Comment on attachment 8970209 [details]
Bug 1456115 - Remove locking in AcmReceiver::GetAudio.

https://reviewboard.mozilla.org/r/239006/#review244726

::: media/webrtc/trunk/webrtc/common_types.h:413
(Diff revision 1)
>          decoded_plc_cng(0),
>          decoded_muted_output(0) {}
>  
> -  int calls_to_silence_generator;  // Number of calls where silence generated,
> +  AudioDecodingCallStats(const AudioDecodingCallStats& other)
> +  {
> +    calls_to_silence_generator = other.calls_to_silence_generator.load();

nit: Is it necessary to use load() explicitly here?

::: media/webrtc/trunk/webrtc/modules/audio_coding/acm2/acm_receiver.h:281
(Diff revision 1)
>  
>    rtc::CriticalSection crit_sect_;
>    rtc::Optional<CodecInst> last_audio_decoder_ GUARDED_BY(crit_sect_);
>    rtc::Optional<SdpAudioFormat> last_audio_format_ GUARDED_BY(crit_sect_);
>    ACMResampler resampler_ GUARDED_BY(crit_sect_);
> -  std::unique_ptr<int16_t[]> last_audio_buffer_ GUARDED_BY(crit_sect_);
> +  // This is only ever touched on a single thread, no need to lock

Please specify which thread.

::: media/webrtc/trunk/webrtc/modules/audio_coding/acm2/acm_receiver.cc:122
(Diff revision 1)
>                            bool* muted) {
>    RTC_DCHECK(muted);
> -  // Accessing members, take the lock.
> -  rtc::CritScope lock(&crit_sect_);
> -
>    if (neteq_->GetAudio(audio_frame, muted) != NetEq::kOK) {

Please consider adding an assertion to guarantee this is always accessed by the same thread.

I think you could add an instance of the rtc::ThreadChecker in webrtc/base and then add an assertion like:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
to this method.
Attachment #8970209 - Flags: review?(dminor) → review+
Comment on attachment 8970211 [details]
Bug 1456115 - Make last_sample_rate_ in `neteq_impl.cc` atomic.

https://reviewboard.mozilla.org/r/239010/#review244738

lgtm
Attachment #8970211 - Flags: review?(dminor) → review+
Comment on attachment 8970210 [details]
Bug 1456115 - Re-serialize inbound NotifyPull.

https://reviewboard.mozilla.org/r/239008/#review244882
Attachment #8970210 - Flags: review?(jyavenard) → review+
(In reply to Dan Minor [:dminor] from comment #11)
> nit: Is it necessary to use load() explicitly here?

Yes:

> error: object of type 'std::__1::atomic<int>' cannot be assigned because its copy assignment operator is implicitly deleted

> ::: media/webrtc/trunk/webrtc/modules/audio_coding/acm2/acm_receiver.cc:122
> (Diff revision 1)
> >                            bool* muted) {
> >    RTC_DCHECK(muted);
> > -  // Accessing members, take the lock.
> > -  rtc::CritScope lock(&crit_sect_);
> > -
> >    if (neteq_->GetAudio(audio_frame, muted) != NetEq::kOK) {
> 
> Please consider adding an assertion to guarantee this is always accessed by
> the same thread.
> 
> I think you could add an instance of the rtc::ThreadChecker in webrtc/base
> and then add an assertion like:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());
> to this method.

We have the guarantee that this will only ever called on one thread at a time, never concurrently, but the underlying thread can change:
- If we change audio device (sometimes, depending on the OS)
- If we're using audio remoting, because the callbacks audio callbacks are called from a thread pool, in a round robin fashion

It's a bit harder to have the guarantee we need here, since the TID can change.
Attachment #8970212 - Attachment is obsolete: true
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/bffecfd92808
Stop locking when getting the sampling rate from the channel in acm_receiver.cc. r=dminor
https://hg.mozilla.org/integration/autoland/rev/e48268ffa4d8
Remove locking in AcmReceiver::GetAudio. r=dminor
https://hg.mozilla.org/integration/autoland/rev/9c77f5b53b56
Re-serialize inbound NotifyPull. r=jya
https://hg.mozilla.org/integration/autoland/rev/3379d4fc07f6
Make last_sample_rate_ in `neteq_impl.cc` atomic. r=dminor
Backed out for Asan Mochitest filures "AddressSanitizer: stack-overflow /celt/bands.c:1437:4 in quant_all_bands"

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3379d4fc07f6802fb46fa3c0ce639d4afac85adf
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175368487&repo=autoland&lineNumber=3080
Flags: needinfo?(padenot)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1e4fdc34670
Backed out 4 changesets for Asan Mochitest filures "AddressSanitizer: stack-overflow /celt/bands.c:1437:4 in quant_all_bands". CLOSED TREE
Yeah well we now have a stack space of 65536 bytes with audio remoting on, it's not going to cut it.
Flags: needinfo?(padenot)
Comment on attachment 8970854 [details]
Bug 1456115 - Increase stack sizes when using audioipc.

https://reviewboard.mozilla.org/r/239634/#review245348

::: modules/libpref/init/all.js:598
(Diff revision 1)
>  
>  // Cubeb sandbox (remoting) control
>  #ifdef XP_LINUX
>  pref("media.cubeb.sandbox", true);
>  pref("media.audioipc.pool_size", 2);
>  // 64 kB stack per pool thread.

please update the comment
Attachment #8970854 - Flags: review+
Comment on attachment 8970854 [details]
Bug 1456115 - Increase stack sizes when using audioipc.

https://reviewboard.mozilla.org/r/239634/#review245350
Attachment #8970854 - Flags: review?(kinetik)
Comment on attachment 8970854 [details]
Bug 1456115 - Increase stack sizes when using audioipc.

https://reviewboard.mozilla.org/r/239634/#review245352
Attachment #8970854 - Flags: review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/91e41f6d1baf
Stop locking when getting the sampling rate from the channel in acm_receiver.cc. r=dminor
https://hg.mozilla.org/integration/autoland/rev/290879198eb4
Remove locking in AcmReceiver::GetAudio. r=dminor
https://hg.mozilla.org/integration/autoland/rev/4a1df66695dd
Re-serialize inbound NotifyPull. r=jya
https://hg.mozilla.org/integration/autoland/rev/956b62a9c5a4
Make last_sample_rate_ in `neteq_impl.cc` atomic. r=dminor
https://hg.mozilla.org/integration/autoland/rev/c4b87fb85648
Increase stack sizes when using audioipc. r=jya
Matthew, I had you on the review here, but I wanted to land before the soft freeze so jya r+ed, I hope it's not a problem.

65536 is a bit too small for us now, 4a1df66695dd in this bug makes the MSG call down into the opus decoder again, in addition to webrtc.org code tendency to use stack buffers (after a hiatus of about 4 months, during which we ran then off-audio-thread), and the audioipc stack size changes have landed in between.
Flags: needinfo?(kinetik)
That's fine, thanks for the NI to keep me updated.  256kB per thread seems fine.
Flags: needinfo?(kinetik)
You need to log in before you can comment on or make changes to this bug.