Closed
Bug 1456115
Opened 6 years ago
Closed 6 years ago
Make some code called on the MSG thread a bit more real-time safe
Categories
(Core :: WebRTC: Audio/Video, enhancement, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: padenot, Assigned: padenot)
Details
Attachments
(9 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
175.41 KB,
image/png
|
Details | |
242.51 KB,
image/png
|
Details | |
216.37 KB,
image/png
|
Details | |
288.77 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
This bug is going to have a few patches to aim at making the overall audio quality when doing calls better.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → padenot
Assignee | ||
Updated•6 years ago
|
Rank: 10
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
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 10•6 years ago
|
||
mozreview-review |
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 11•6 years ago
|
||
mozreview-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 12•6 years ago
|
||
mozreview-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 13•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8970212 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
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
Assignee | ||
Comment 22•6 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 24•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c2d3c16a653313bcb10ee088a4d506a669e9bf
Comment 25•6 years ago
|
||
mozreview-review |
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 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8970854 [details] Bug 1456115 - Increase stack sizes when using audioipc. https://reviewboard.mozilla.org/r/239634/#review245350
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8970854 -
Flags: review?(kinetik)
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8970854 [details] Bug 1456115 - Increase stack sizes when using audioipc. https://reviewboard.mozilla.org/r/239634/#review245352
Attachment #8970854 -
Flags: review+
Comment 29•6 years ago
|
||
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
Assignee | ||
Comment 30•6 years ago
|
||
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)
Comment 31•6 years ago
|
||
That's fine, thanks for the NI to keep me updated. 256kB per thread seems fine.
Flags: needinfo?(kinetik)
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91e41f6d1baf https://hg.mozilla.org/mozilla-central/rev/290879198eb4 https://hg.mozilla.org/mozilla-central/rev/4a1df66695dd https://hg.mozilla.org/mozilla-central/rev/956b62a9c5a4 https://hg.mozilla.org/mozilla-central/rev/c4b87fb85648
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•