1887774 - Set EchoCanceller3Config::echo_removal_control.has_clock_drift when clock drift is expected

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

Andreas pointed out the has_clock_drift configuration for AudioProcessing, which may help the echo canceller when it struggles due to clock drift with a secondary audio output or input.

This parameter was initially introduced because setups with clock-drift are generally difficult to handle for echo cancellers, but it had no effect at that time. It was later used, when echo suppression was reduced for dominant near end, so as not to reduce echo suppression when clock drift is expected. Calls with clock drift tend to have less accurate linear filters. The parameter forces limiting of the high frequency gains to avoid echo leakage due to an imperfect filter.

Even without this parameter set, aec3 attempts to detect clock drift and apply the same limiting if detected. The ClockdriftDetector, however, did not detect drift, even when the AudioProcessing successfully cancels echo at 1.002 drift.

The only API to set has_clock_drift for an AudioProcessing is an EchoCanceller3Factory, which must be set when constructing the AudioProcessing. The echo controller is always enabled regardless of Config::echo_canceller::enabled if echo_control_factory_ is set.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 add prefs to simulate drift in audio streams r?padenot

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 apply a saved "media.cubeb.force_sample_rate" pref in content processes r?padenot

Without PREF_CUBEB_FORCE_SAMPLE_RATE in gInitCallbackPrefs, content processes
did not see the pref value unless the pref was changed during the lifetime of
the content process.

Depends on D206863

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 skip sleeps to run AudioProcessingTrackDisabling GTest 6 seconds faster r?padenot

Depends on D206864

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 convert from MediaEnginePrefs to AudioProcessing config in AudioInputProcessing r?pehrsons

This clarifies that config, pass-through, and input channel count are all set
in the same way.

It will also facilitate delayed AudioProcessing construction in a subsequent
patch.

Depends on D206865

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 delay mPacketizerInput initialization until required r?chunmin

mPacketizerInput won't need to be re-created for every channel count change if
no processing is done for some channel counts.

More signficantly, this will facilitate delaying, in a subsequent patch,
AudioProcessing construction while its parameters may still be changing.

Depends on D206866

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 delay AudioProcessing initialization until required r?chunmin

The benefit is that whether the DeviceInputTrack or reverse stream could be
affected by clock drift is more likely to be known, and so recreation of
mAudioProcessing will be less likely when, in a subsequent patch, the
AudioProcessing needs to be replaced when the possibility of drift changes.

mSkipProcessing and mRequestedInputChannelCount are now replaced by their
MediaEnginePrefs equivalents. The AudioInputProcessing now starts in
pass-through mode, consistent with its initial AEC, AGC, and noise suppression
settings.

Depends on D206867

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 remove unnecessary aPort parameter from GetInputSourceData() r?chunmin

Depends on D206868

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 rename ConnectToNativeDevice introspection function to ConnectedToNativeDevice r?chunmin

Depends on D206869

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 pass a track to EnsureAudioProcessing() r?pehrsons

from a secondary input or output device.

Depends on D206870

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 GTest EchoCanceller3Config::echo_removal_control.has_clock_drift config r?pehrsons

Depends on D206871

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 add a preference to override EchoCanceller3Config::echo_removal_control.has_clock_drift r?pehrsons

The pref name is chosen for consistency with
https://phabricator.services.mozilla.com/D205940?id=842393

Depends on D206872

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1887774 Set EchoCanceller3Config::echo_removal_control.has_clock_drift when clock drift is expected r?pehrsons

from a secondary input or output device.

Depends on D206871

Comment 13

[Karl Tomlinson (:karlt)]

This change helps echo cancellation but drift still makes cancellation less effective than when there is no drift.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0902541309d
add prefs to simulate drift in audio streams r=padenot

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/679472e2bd34
apply a saved "media.cubeb.force_sample_rate" pref in content processes r=padenot

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/399bfc5051b2
skip sleeps to run AudioProcessingTrackDisabling GTest 6 seconds faster r=padenot

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d2d7dc377c8
convert from MediaEnginePrefs to AudioProcessing config in AudioInputProcessing r=pehrsons

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deb37d76e032
remove unnecessary aPort parameter from GetInputSourceData() r=pehrsons

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9eb44aa2ffa
rename ConnectToNativeDevice introspection function to ConnectedToNativeDevice r=pehrsons

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd2cdabcc883
delay mPacketizerInput initialization until required r=pehrsons

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/210dbccad6f2
delay AudioProcessing initialization until required r=pehrsons

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29c59d5d9bee
pass a track to EnsureAudioProcessing() r=pehrsons

Comment 23

[Cosmin Sabou [:CosminS]]

Backed out for causing gtest failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=210dbccad6f29779466e74fda0844312546b23ee&selectedTaskRun=Z2P0gGrOSeuM1ZevY5URmg.0

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=454251246&repo=autoland
• https://treeherder.mozilla.org/logviewer?job_id=454249539&repo=autoland&lineNumber=7087
• https://treeherder.mozilla.org/logviewer?job_id=454249304&repo=autoland&lineNumber=36777

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

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd8757fdd446
Backed out 2 changesets for causing gtest failures. CLOSED TREE

Comment 25

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a0902541309d
https://hg.mozilla.org/mozilla-central/rev/679472e2bd34
https://hg.mozilla.org/mozilla-central/rev/399bfc5051b2
https://hg.mozilla.org/mozilla-central/rev/5d2d7dc377c8
https://hg.mozilla.org/mozilla-central/rev/deb37d76e032
https://hg.mozilla.org/mozilla-central/rev/e9eb44aa2ffa
https://hg.mozilla.org/mozilla-central/rev/bd2cdabcc883

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to Cosmin Sabou [:CosminS] from comment #23)

Backed out for causing gtest failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=210dbccad6f29779466e74fda0844312546b23ee&selectedTaskRun=Z2P0gGrOSeuM1ZevY5URmg.0

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=454251246&repo=autoland

Thanks.
https://phabricator.services.mozilla.com/D206868?vs=847872&id=848372#toc addresses this.

• https://treeherder.mozilla.org/logviewer?job_id=454249539&repo=autoland&lineNumber=7087

Bug 1794126.

• https://treeherder.mozilla.org/logviewer?job_id=454249304&repo=autoland&lineNumber=36777

This looks like bug 1867766.

https://phabricator.services.mozilla.com/D206865 might plausibly make this more or less frequent.
Please feel free to back out 399bfc5051b2 if that seems to be making those failures more frequent.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c06840f719aa
delay AudioProcessing initialization until required r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/6c8ba616b1c4
pass a track to EnsureAudioProcessing() r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/df631cf3e2aa
Set EchoCanceller3Config::echo_removal_control.has_clock_drift when clock drift is expected r=pehrsons

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebe948d4a99a
GTest EchoCanceller3Config::echo_removal_control.has_clock_drift config r=pehrsons

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2da6f77d3ad
add a preference to override EchoCanceller3Config::echo_removal_control.has_clock_drift r=pehrsons

Comment 30

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c06840f719aa
https://hg.mozilla.org/mozilla-central/rev/6c8ba616b1c4
https://hg.mozilla.org/mozilla-central/rev/df631cf3e2aa
https://hg.mozilla.org/mozilla-central/rev/ebe948d4a99a
https://hg.mozilla.org/mozilla-central/rev/b2da6f77d3ad