1897328 - smooth buffering level input to DriftController

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

Variations in the timing and size of audio packets can cause fluctuations in the AudioDriftCorrection buffering level, to which DriftController responds with unnecessary changes in the resampling rate. Such changes can be large enough to degrade echo canceller performance.

The variations can be due to

• Differences between input and output callback frequency or timing.
• Rounding to 128-frame chunks for the MediaTrackGraph and Web Audio.
• Inconsistent packet sizes from the system audio callback. This seems particularly common with a pulseaudio server. (Pipewire was much more consistent.) It was observed with an Intel 100 Series/C230 Series Chipset Family HD Audio Controller, but not with a bluetooth speaker.
  Doubling minreq to tlength / 4, as suggested in the commit message of https://github.com/mozilla/cubeb-pulse-rs/commit/ee66661422726f6db781c9043de90ccec4e3520a, changed the pattern, but packets were still irregularly size. Perhaps larger packets could be written than requested, but this would have an effect on latency.
  Evidence of chunk or packet size differences can be observed to some extent in the "In latency" and "Out latency" plots linked from bug 1890467 comment 1, which variables are 5-point equal-weighted FIR moving averages of frame counts between each output request.

The variations are short term and often oscillatory, and so low-pass filter smoothing would reduce their effect.

A naive smoothing of the effects of the PID controller would make the controller unstable due to the delay in feedback.

Comment 1

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/jobs?repo=try&revision=132bda07d3a7b2a3b355fd19d12541dbcf2b0de1

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Pulseaudio 48->44.1 before changes

The attached plot is from a system with pulseaudio. The DriftController sat between a graph running at 48kHz and a secondary audio output (from setSinkId()) running at 44.1kHz. The "media.cubeb.force_sample_rate" pref can be useful to configure the graph for a rate different to the preferred rate of an output device.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Pulseaudio 48->44.1 with smoothed buffering level

With smoothing, the variability in the configured in sample rate is reduced by enough that the responses to changes in desired buffering dominate.
The graphs may be zoomed to see the variations in configured in sample rate.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: TestAudioDriftCorrection.DynamicInputBufferSizeChanges before changes

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: TestAudioDriftCorrection.DynamicInputBufferSizeChanges with smoothed buffering level

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: TestDriftController.LargeStepResponse with integral term

The configured resampling rate and actual buffering in TestDriftController.LargeStepResponse (a new test to be added here) are similar with and without smoothing of the buffer level because the buffer level is smooth. This plot is for the controller with smoothing.

The caps on the changes in the configured resampling rate delay the response of the PID controller, causing large oscillations.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: TestDriftController.LargeStepResponse for PD controller with clock drift estimate

After replacing the integral term of the PID controller with a clock drift estimate, the oscillations attenuate much faster.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: TestAudioDriftCorrection.DynamicInputBufferSizeChanges with smoothed buffering level

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Pulseaudio 48->44.1 PD controller with clock drift estimate

Replacing the integral term with the drift estimate does not have a significant effect on the variability in configured in sample rate.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1897328 Remove unused MediaTrackGraphImpl::mIterationEndTime r?padenot

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1897328 Add a profiling marker to trace the period rendered by the MediaTrackGraph r?padenot

Depends on D215483

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1897328 Provide input durations to DriftController tests that are consistent with the buffering levels r?pehrsons

The DriftController will be modified in a subsequent patch to use these values.

Depends on D215484

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1897328 Smooth buffering level input to DriftController r?pehrsons

An average buffering level estimate is used as input to the PID controller.
This estimate is updated for known changes in resampling (control signal
feedback) and for an estimated drift rate.

The hysteresis band is reduced because the average buffer level is less
variable. The hysteresis band must be smaller for the instantaneous error to
remain within its (wider) band for assessing whether to reduce the desired
buffer level.

The number of iterations is increased with a smaller output packet size in
several TestDriftController tests so that buffer level changes are reflected
in its moving average faster. The controller intentionally does not respond
as much to variations in a small number of buffering level data points.

An extra second is added to TestDriftController.VerySmallBufferedFrames
because, as the corrected rate gets very small, the input packet durations are
sometimes zero and the corrected rate does not get updated when the input
packet is zero.

TestAudioDriftCorrection.DriftStepResponseUnderrunHighLatencyInput has higher
actual buffering before the missing input segment, and so takes longer to
underrun.

TestAudioDriftCorrection.DynamicInputBufferSizeChanges was still adjusting the
desired buffering level at the end of the test, both before and after this
change, but the narrower hysteresis is less tolerant of desired buffer level
changes.

Depends on D215485

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1897328 Replace integral controller term with clock drift estimate r?pehrsons

The drift estimate is not affected by feedback from the PD controller and so
does not have the same stability risks as the integral term, such as integral
wind up while the change in response is being capped. The response while not
capped and outside hysteresis is now purely an exponential decay, no longer
with an oscillatory component. When the capping is applied, the lag in
adjustment still causes some oscillations from overshoot, but these now
attenuate much faster.

If the drift estimate applied to the corrected rate is accurate enough, then
the desired steady state contribution from the remaining terms approaches zero
as the error approaches zero, so the integral term is no longer required.

The rate at which the drift estimate adjusts to changes in input depends on
packet rate. For typical low latency packet sizes, the drift estimate should
adjust faster than the integral term. This makes the controller more
resistant to underrun in TestAudioDriftCorrection.DriftStepResponseUnderrun,
which is adjusted according.

The clock drift estimate does not anticipate ongoing desired buffer level
changes like the intergral term did, and so
TestAudioDriftCorrection.DynamicInputBufferSizeChanges does not reach the
hysteresis band by the end of the test.

TestDriftController.VerySmallBufferedFrames now runs slightly longer because
the rate of change in correction is not quite as steep as the capped rate.

Depends on D215486

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28378fd26a21
Remove unused MediaTrackGraphImpl::mIterationEndTime r=padenot
https://hg.mozilla.org/integration/autoland/rev/6c5306138d62
Add a profiling marker to trace the period rendered by the MediaTrackGraph r=padenot

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63258b634468
Provide input durations to DriftController tests that are consistent with the buffering levels r=pehrsons

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/432ef28ef357
Smooth buffering level input to DriftController r=pehrsons

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/545e985e2428
Replace integral controller term with clock drift estimate r=pehrsons

Comment 19

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/28378fd26a21
https://hg.mozilla.org/mozilla-central/rev/6c5306138d62

Comment 20

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/63258b634468
https://hg.mozilla.org/mozilla-central/rev/432ef28ef357
https://hg.mozilla.org/mozilla-central/rev/545e985e2428