On Thu, 2017-09-14 at 12:21 -0700, James Huang wrote:
> ---
> src/modules/echo-cancel/webrtc.cc | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-cancel/webrtc.cc
> index aadb1af2..107d2301 100644
> --- a/src/modules/echo-cancel/webrtc.cc
> +++ b/src/modules/echo-cancel/webrtc.cc
> @@ -55,6 +55,7 @@ PA_C_DECL_END
> #define DEFAULT_AGC_START_VOLUME 85
> #define DEFAULT_BEAMFORMING false
> #define DEFAULT_TRACE false
> +#define DEFAULT_ECHO_SUPPRESSION_LEVEL 0
>
> #define WEBRTC_AGC_MAX_VOLUME 255
>
> @@ -76,6 +77,7 @@ static const char* const valid_modargs[] = {
> "mic_geometry", /* documented in parse_mic_geometry() */
> "target_direction", /* documented in parse_mic_geometry() */
> "trace",
> + "echo_suppression_level",
> NULL
> };
>
> @@ -240,7 +242,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
> webrtc::Config config;
> bool hpf, ns, agc, dgc, mobile, cn, vad, ext_filter, intelligibility, experimental_agc, beamforming;
> int rm = -1, i;
> - uint32_t agc_start_volume;
> + uint32_t agc_start_volume, echo_suppression_level;
> pa_modargs *ma;
> bool trace = false;
>
> @@ -290,6 +292,12 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
> goto fail;
> }
>
> + echo_suppression_level = DEFAULT_ECHO_SUPPRESSION_LEVEL;
> + if (pa_modargs_get_value_u32(ma, "echo_suppression_level", &echo_suppression_level) < 0) {
> + pa_log("Failed to parse echo_suppression_level value");
> + goto fail;
> + }
> +
> if (mobile) {
> if (ec->params.drift_compensation) {
> pa_log("Can't use drift_compensation in mobile mode");
> @@ -439,6 +447,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
> if (hpf)
> apm->high_pass_filter()->Enable(true);
>
> + apm->echo_cancellation()->set_suppression_level((webrtc::EchoCancellation::SuppressionLevel) echo_suppression_level);
> if (!mobile) {
> apm->echo_cancellation()->enable_drift_compensation(ec->params.drift_compensation);
> apm->echo_cancellation()->Enable(true);
The code looks fine, but the commit message should explain the
following things:
What problem does the patch solve? What does the "echo suppression
level" mean? What's the range of valid values for the level? (Oh, now I
realized that the code needs some changes too: it should check that the
passed value is in the valid range, unless the range covers all
possible u32 values.) If the set_suppression_level() function is not
called, is that equivalent to setting the level to 0?
--
Tanu
https://www.patreon.com/tanuk
