From 8ee06a7b0cc22a486ad924e00034b95dbecd70ce Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Tue, 23 Oct 2018 12:32:42 +0200 Subject: [PATCH] Increase coverage of AEC3 JSON config unit tests, fix bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new test checks that json strings are unchanged when parsing to a config and back to a string. This ensures that everything in the json representations is parsed when created a config from the json. This CL also adds the render_levels config substruct to the JSON parser. Some issues were surfaced by the new test: - Config validation clamping silently passed NaNs - Config validation only fixed the first out-of-bounds parameter, and not any subsequent ones - Config validation did not check all values in the config Bug: webrtc:9535 Change-Id: Ie7b588731dc1fe26ba71d1eb2f177f3b3b8139e3 Reviewed-on: https://webrtc-review.googlesource.com/c/107120 Commit-Queue: Sam Zackrisson Reviewed-by: Per Åhgren Cr-Commit-Position: refs/heads/master@{#25310} --- api/audio/echo_canceller3_config.cc | 227 +++++++++--------- api/audio/echo_canceller3_config.h | 2 +- api/audio/echo_canceller3_config_json.cc | 3 +- .../echo_canceller3_config_json_unittest.cc | 70 ++++-- 4 files changed, 165 insertions(+), 137 deletions(-) diff --git a/api/audio/echo_canceller3_config.cc b/api/audio/echo_canceller3_config.cc index 46c1eda326..f040790682 100644 --- a/api/audio/echo_canceller3_config.cc +++ b/api/audio/echo_canceller3_config.cc @@ -10,6 +10,7 @@ #include "api/audio/echo_canceller3_config.h" #include +#include #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_minmax.h" @@ -18,6 +19,7 @@ namespace webrtc { namespace { bool Limit(float* value, float min, float max) { float clamped = rtc::SafeClamp(*value, min, max); + clamped = std::isfinite(clamped) ? clamped : min; bool res = *value == clamped; *value = clamped; return res; @@ -85,164 +87,161 @@ bool EchoCanceller3Config::Validate(EchoCanceller3Config* config) { c->delay.down_sampling_factor = 4; res = false; } - - if (c->delay.num_filters == 0) { - c->delay.num_filters = 1; - res = false; - } - if (c->delay.api_call_jitter_blocks == 0) { - c->delay.api_call_jitter_blocks = 1; - res = false; - } - - if (c->delay.api_call_jitter_blocks == 0) { - c->delay.api_call_jitter_blocks = 1; - res = false; - } if (c->delay.delay_headroom_blocks <= 1 && c->delay.hysteresis_limit_1_blocks == 1) { c->delay.hysteresis_limit_1_blocks = 0; res = false; } - res = res && Limit(&c->delay.delay_estimate_smoothing, 0.f, 1.f); - res = res && Limit(&c->delay.delay_candidate_detection_threshold, 0.f, 1.f); - res = res && Limit(&c->delay.delay_selection_thresholds.initial, 1, 250); - res = res && Limit(&c->delay.delay_selection_thresholds.converged, 1, 250); + res = res & Limit(&c->delay.default_delay, 0, 5000); + res = res & Limit(&c->delay.num_filters, 0, 5000); + res = res & Limit(&c->delay.api_call_jitter_blocks, 1, 5000); + res = res & Limit(&c->delay.min_echo_path_delay_blocks, 0, 5000); + res = res & Limit(&c->delay.delay_headroom_blocks, 0, 5000); + res = res & Limit(&c->delay.hysteresis_limit_1_blocks, 0, 5000); + res = res & Limit(&c->delay.hysteresis_limit_2_blocks, 0, 5000); + res = res & Limit(&c->delay.skew_hysteresis_blocks, 0, 5000); + res = res & Limit(&c->delay.fixed_capture_delay_samples, 0, 5000); + res = res & Limit(&c->delay.delay_estimate_smoothing, 0.f, 1.f); + res = res & Limit(&c->delay.delay_candidate_detection_threshold, 0.f, 1.f); + res = res & Limit(&c->delay.delay_selection_thresholds.initial, 1, 250); + res = res & Limit(&c->delay.delay_selection_thresholds.converged, 1, 250); - res = res && Limit(&c->filter.main.length_blocks, 1, 50); - res = res && Limit(&c->filter.main.leakage_converged, 0.f, 1000.f); - res = res && Limit(&c->filter.main.leakage_diverged, 0.f, 1000.f); - res = res && Limit(&c->filter.main.error_floor, 0.f, 1000.f); - res = res && Limit(&c->filter.main.noise_gate, 0.f, 100000000.f); + res = res & Limit(&c->filter.main.length_blocks, 1, 50); + res = res & Limit(&c->filter.main.leakage_converged, 0.f, 1000.f); + res = res & Limit(&c->filter.main.leakage_diverged, 0.f, 1000.f); + res = res & Limit(&c->filter.main.error_floor, 0.f, 1000.f); + res = res & Limit(&c->filter.main.error_ceil, 0.f, 100000000.f); + res = res & Limit(&c->filter.main.noise_gate, 0.f, 100000000.f); - res = res && Limit(&c->filter.main_initial.length_blocks, 1, 50); - res = res && Limit(&c->filter.main_initial.leakage_converged, 0.f, 1000.f); - res = res && Limit(&c->filter.main_initial.leakage_diverged, 0.f, 1000.f); - res = res && Limit(&c->filter.main_initial.error_floor, 0.f, 1000.f); - res = res && Limit(&c->filter.main_initial.noise_gate, 0.f, 100000000.f); + res = res & Limit(&c->filter.main_initial.length_blocks, 1, 50); + res = res & Limit(&c->filter.main_initial.leakage_converged, 0.f, 1000.f); + res = res & Limit(&c->filter.main_initial.leakage_diverged, 0.f, 1000.f); + res = res & Limit(&c->filter.main_initial.error_floor, 0.f, 1000.f); + res = res & Limit(&c->filter.main_initial.error_ceil, 0.f, 100000000.f); + res = res & Limit(&c->filter.main_initial.noise_gate, 0.f, 100000000.f); if (c->filter.main.length_blocks < c->filter.main_initial.length_blocks) { c->filter.main_initial.length_blocks = c->filter.main.length_blocks; res = false; } - res = res && Limit(&c->filter.shadow.length_blocks, 1, 50); - res = res && Limit(&c->filter.shadow.rate, 0.f, 1.f); - res = res && Limit(&c->filter.shadow.noise_gate, 0.f, 100000000.f); + res = res & Limit(&c->filter.shadow.length_blocks, 1, 50); + res = res & Limit(&c->filter.shadow.rate, 0.f, 1.f); + res = res & Limit(&c->filter.shadow.noise_gate, 0.f, 100000000.f); - res = res && Limit(&c->filter.shadow_initial.length_blocks, 1, 50); - res = res && Limit(&c->filter.shadow_initial.rate, 0.f, 1.f); - res = res && Limit(&c->filter.shadow_initial.noise_gate, 0.f, 100000000.f); + res = res & Limit(&c->filter.shadow_initial.length_blocks, 1, 50); + res = res & Limit(&c->filter.shadow_initial.rate, 0.f, 1.f); + res = res & Limit(&c->filter.shadow_initial.noise_gate, 0.f, 100000000.f); if (c->filter.shadow.length_blocks < c->filter.shadow_initial.length_blocks) { c->filter.shadow_initial.length_blocks = c->filter.shadow.length_blocks; res = false; } - res = res && Limit(&c->filter.config_change_duration_blocks, 0, 100000); - res = res && Limit(&c->filter.initial_state_seconds, 0.f, 100.f); + res = res & Limit(&c->filter.config_change_duration_blocks, 0, 100000); + res = res & Limit(&c->filter.initial_state_seconds, 0.f, 100.f); - res = res && Limit(&c->erle.min, 1.f, 100000.f); - res = res && Limit(&c->erle.max_l, 1.f, 100000.f); - res = res && Limit(&c->erle.max_h, 1.f, 100000.f); + res = res & Limit(&c->erle.min, 1.f, 100000.f); + res = res & Limit(&c->erle.max_l, 1.f, 100000.f); + res = res & Limit(&c->erle.max_h, 1.f, 100000.f); if (c->erle.min > c->erle.max_l || c->erle.min > c->erle.max_h) { c->erle.min = std::min(c->erle.max_l, c->erle.max_h); res = false; } - res = res && Limit(&c->ep_strength.lf, 0.f, 1000000.f); - res = res && Limit(&c->ep_strength.mf, 0.f, 1000000.f); - res = res && Limit(&c->ep_strength.hf, 0.f, 1000000.f); - res = res && Limit(&c->ep_strength.default_len, 0.f, 1.f); - - res = res && - Limit(&c->echo_audibility.low_render_limit, 0.f, 32768.f * 32768.f); - res = res && - Limit(&c->echo_audibility.normal_render_limit, 0.f, 32768.f * 32768.f); - res = res && Limit(&c->echo_audibility.floor_power, 0.f, 32768.f * 32768.f); - res = res && Limit(&c->echo_audibility.audibility_threshold_lf, 0.f, - 32768.f * 32768.f); - res = res && Limit(&c->echo_audibility.audibility_threshold_mf, 0.f, - 32768.f * 32768.f); - res = res && Limit(&c->echo_audibility.audibility_threshold_hf, 0.f, - 32768.f * 32768.f); - - res = res && - Limit(&c->render_levels.active_render_limit, 0.f, 32768.f * 32768.f); - res = res && Limit(&c->render_levels.poor_excitation_render_limit, 0.f, - 32768.f * 32768.f); - res = res && Limit(&c->render_levels.poor_excitation_render_limit_ds8, 0.f, - 32768.f * 32768.f); + res = res & Limit(&c->ep_strength.lf, 0.f, 1000000.f); + res = res & Limit(&c->ep_strength.mf, 0.f, 1000000.f); + res = res & Limit(&c->ep_strength.hf, 0.f, 1000000.f); + res = res & Limit(&c->ep_strength.default_len, 0.f, 1.f); res = - res && Limit(&c->echo_removal_control.gain_rampup.initial_gain, 0.f, 1.f); - res = res && Limit(&c->echo_removal_control.gain_rampup.first_non_zero_gain, - 0.f, 1.f); - res = res && Limit(&c->echo_removal_control.gain_rampup.non_zero_gain_blocks, - 0, 100000); - res = res && + res & Limit(&c->echo_audibility.low_render_limit, 0.f, 32768.f * 32768.f); + res = res & + Limit(&c->echo_audibility.normal_render_limit, 0.f, 32768.f * 32768.f); + res = res & Limit(&c->echo_audibility.floor_power, 0.f, 32768.f * 32768.f); + res = res & Limit(&c->echo_audibility.audibility_threshold_lf, 0.f, + 32768.f * 32768.f); + res = res & Limit(&c->echo_audibility.audibility_threshold_mf, 0.f, + 32768.f * 32768.f); + res = res & Limit(&c->echo_audibility.audibility_threshold_hf, 0.f, + 32768.f * 32768.f); + + res = res & + Limit(&c->render_levels.active_render_limit, 0.f, 32768.f * 32768.f); + res = res & Limit(&c->render_levels.poor_excitation_render_limit, 0.f, + 32768.f * 32768.f); + res = res & Limit(&c->render_levels.poor_excitation_render_limit_ds8, 0.f, + 32768.f * 32768.f); + + res = + res & Limit(&c->echo_removal_control.gain_rampup.initial_gain, 0.f, 1.f); + res = res & Limit(&c->echo_removal_control.gain_rampup.first_non_zero_gain, + 0.f, 1.f); + res = res & Limit(&c->echo_removal_control.gain_rampup.non_zero_gain_blocks, + 0, 100000); + res = res & Limit(&c->echo_removal_control.gain_rampup.full_gain_blocks, 0, 100000); - res = res && Limit(&c->echo_model.noise_floor_hold, 0, 1000); - res = res && Limit(&c->echo_model.min_noise_floor_power, 0, 2000000.f); - res = res && Limit(&c->echo_model.stationary_gate_slope, 0, 1000000.f); - res = res && Limit(&c->echo_model.noise_gate_power, 0, 1000000.f); - res = res && Limit(&c->echo_model.noise_gate_slope, 0, 1000000.f); - res = res && Limit(&c->echo_model.render_pre_window_size, 0, 100); - res = res && Limit(&c->echo_model.render_post_window_size, 0, 100); - res = res && Limit(&c->echo_model.render_pre_window_size_init, 0, 100); - res = res && Limit(&c->echo_model.render_post_window_size_init, 0, 100); - res = res && Limit(&c->echo_model.nonlinear_hold, 0, 100); - res = res && Limit(&c->echo_model.nonlinear_release, 0, 1.f); + res = res & Limit(&c->echo_model.noise_floor_hold, 0, 1000); + res = res & Limit(&c->echo_model.min_noise_floor_power, 0, 2000000.f); + res = res & Limit(&c->echo_model.stationary_gate_slope, 0, 1000000.f); + res = res & Limit(&c->echo_model.noise_gate_power, 0, 1000000.f); + res = res & Limit(&c->echo_model.noise_gate_slope, 0, 1000000.f); + res = res & Limit(&c->echo_model.render_pre_window_size, 0, 100); + res = res & Limit(&c->echo_model.render_post_window_size, 0, 100); + res = res & Limit(&c->echo_model.render_pre_window_size_init, 0, 100); + res = res & Limit(&c->echo_model.render_post_window_size_init, 0, 100); + res = res & Limit(&c->echo_model.nonlinear_hold, 0, 100); + res = res & Limit(&c->echo_model.nonlinear_release, 0, 1.f); - res = res && + res = res & Limit(&c->suppressor.nearend_average_blocks, 1, 5000); + + res = res & Limit(&c->suppressor.normal_tuning.mask_lf.enr_transparent, 0.f, 100.f); - res = res && + res = res & Limit(&c->suppressor.normal_tuning.mask_lf.enr_suppress, 0.f, 100.f); - res = res && + res = res & Limit(&c->suppressor.normal_tuning.mask_lf.emr_transparent, 0.f, 100.f); - res = res && + res = res & Limit(&c->suppressor.normal_tuning.mask_hf.enr_transparent, 0.f, 100.f); - res = res && + res = res & Limit(&c->suppressor.normal_tuning.mask_hf.enr_suppress, 0.f, 100.f); - res = res && + res = res & Limit(&c->suppressor.normal_tuning.mask_hf.emr_transparent, 0.f, 100.f); - res = res && Limit(&c->suppressor.normal_tuning.max_inc_factor, 0.f, 100.f); - res = - res && Limit(&c->suppressor.normal_tuning.max_dec_factor_lf, 0.f, 100.f); + res = res & Limit(&c->suppressor.normal_tuning.max_inc_factor, 0.f, 100.f); + res = res & Limit(&c->suppressor.normal_tuning.max_dec_factor_lf, 0.f, 100.f); - res = res && Limit(&c->suppressor.nearend_tuning.mask_lf.enr_transparent, 0.f, - 100.f); - res = res && + res = res & Limit(&c->suppressor.nearend_tuning.mask_lf.enr_transparent, 0.f, + 100.f); + res = res & Limit(&c->suppressor.nearend_tuning.mask_lf.enr_suppress, 0.f, 100.f); - res = res && Limit(&c->suppressor.nearend_tuning.mask_lf.emr_transparent, 0.f, - 100.f); - res = res && Limit(&c->suppressor.nearend_tuning.mask_hf.enr_transparent, 0.f, - 100.f); - res = res && + res = res & Limit(&c->suppressor.nearend_tuning.mask_lf.emr_transparent, 0.f, + 100.f); + res = res & Limit(&c->suppressor.nearend_tuning.mask_hf.enr_transparent, 0.f, + 100.f); + res = res & Limit(&c->suppressor.nearend_tuning.mask_hf.enr_suppress, 0.f, 100.f); - res = res && Limit(&c->suppressor.nearend_tuning.mask_hf.emr_transparent, 0.f, - 100.f); - res = res && Limit(&c->suppressor.nearend_tuning.max_inc_factor, 0.f, 100.f); + res = res & Limit(&c->suppressor.nearend_tuning.mask_hf.emr_transparent, 0.f, + 100.f); + res = res & Limit(&c->suppressor.nearend_tuning.max_inc_factor, 0.f, 100.f); res = - res && Limit(&c->suppressor.nearend_tuning.max_dec_factor_lf, 0.f, 100.f); + res & Limit(&c->suppressor.nearend_tuning.max_dec_factor_lf, 0.f, 100.f); - res = res && Limit(&c->suppressor.dominant_nearend_detection.enr_threshold, - 0.f, 1000000.f); - res = res && Limit(&c->suppressor.dominant_nearend_detection.snr_threshold, - 0.f, 1000000.f); - res = res && Limit(&c->suppressor.dominant_nearend_detection.hold_duration, 0, - 10000); - res = - res && Limit(&c->suppressor.dominant_nearend_detection.trigger_threshold, - 0, 10000); + res = res & Limit(&c->suppressor.dominant_nearend_detection.enr_threshold, + 0.f, 1000000.f); + res = res & Limit(&c->suppressor.dominant_nearend_detection.snr_threshold, + 0.f, 1000000.f); + res = res & Limit(&c->suppressor.dominant_nearend_detection.hold_duration, 0, + 10000); + res = res & Limit(&c->suppressor.dominant_nearend_detection.trigger_threshold, + 0, 10000); - res = res && Limit(&c->suppressor.high_bands_suppression.enr_threshold, 0.f, - 1000000.f); - res = res && Limit(&c->suppressor.high_bands_suppression.max_gain_during_echo, - 0.f, 1.f); + res = res & Limit(&c->suppressor.high_bands_suppression.enr_threshold, 0.f, + 1000000.f); + res = res & Limit(&c->suppressor.high_bands_suppression.max_gain_during_echo, + 0.f, 1.f); - res = res && Limit(&c->suppressor.floor_first_increase, 0.f, 1000000.f); + res = res & Limit(&c->suppressor.floor_first_increase, 0.f, 1000000.f); return res; } diff --git a/api/audio/echo_canceller3_config.h b/api/audio/echo_canceller3_config.h index 5bb5ece15f..5eb2fd2ed9 100644 --- a/api/audio/echo_canceller3_config.h +++ b/api/audio/echo_canceller3_config.h @@ -19,7 +19,7 @@ namespace webrtc { // Configuration struct for EchoCanceller3 struct RTC_EXPORT EchoCanceller3Config { - // Checks and updates the parameters in a config to lie within reasonable + // Checks and updates the config parameters to lie within (mostly) reasonable // ranges. Returns true if and only of the config did not need to be changed. static bool Validate(EchoCanceller3Config* config); diff --git a/api/audio/echo_canceller3_config_json.cc b/api/audio/echo_canceller3_config_json.cc index 2b6a329b29..bb44162730 100644 --- a/api/audio/echo_canceller3_config_json.cc +++ b/api/audio/echo_canceller3_config_json.cc @@ -30,6 +30,7 @@ void ReadParam(const Json::Value& root, std::string param_name, size_t* param) { RTC_DCHECK(param); int v; if (rtc::GetIntFromJsonObject(root, param_name, &v)) { + RTC_DCHECK_GE(v, 0); *param = v; } } @@ -216,7 +217,7 @@ EchoCanceller3Config Aec3ConfigFromJsonString(absl::string_view json_string) { &cfg.echo_audibility.audibility_threshold_hf); ReadParam(section, "use_stationary_properties", &cfg.echo_audibility.use_stationary_properties); - ReadParam(section, "use_stationary_properties_at_init", + ReadParam(section, "use_stationarity_properties_at_init", &cfg.echo_audibility.use_stationarity_properties_at_init); } diff --git a/api/audio/test/echo_canceller3_config_json_unittest.cc b/api/audio/test/echo_canceller3_config_json_unittest.cc index 7c3a445c60..16967fcecf 100644 --- a/api/audio/test/echo_canceller3_config_json_unittest.cc +++ b/api/audio/test/echo_canceller3_config_json_unittest.cc @@ -8,34 +8,62 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "api/audio/echo_canceller3_config_json.h" +#include +#include + #include "api/audio/echo_canceller3_config.h" +#include "api/audio/echo_canceller3_config_json.h" +#include "rtc_base/random.h" #include "test/gtest.h" namespace webrtc { +namespace { +EchoCanceller3Config GenerateRandomConfig(Random* prng) { + std::array random_bytes; + for (uint8_t& byte : random_bytes) { + byte = prng->Rand(); + } + auto* config = reinterpret_cast(random_bytes.data()); + EchoCanceller3Config::Validate(config); + return *config; +} +} // namespace TEST(EchoCanceller3JsonHelpers, ToStringAndParseJson) { - EchoCanceller3Config cfg; - cfg.delay.down_sampling_factor = 1u; - cfg.filter.shadow_initial.length_blocks = 7u; - cfg.suppressor.normal_tuning.mask_hf.enr_suppress = .5f; - std::string json_string = Aec3ConfigToJsonString(cfg); - EchoCanceller3Config cfg_transformed = Aec3ConfigFromJsonString(json_string); + Random prng(7297352569823ull); + for (int i = 0; i < 10; ++i) { + EchoCanceller3Config cfg = GenerateRandomConfig(&prng); + std::string json_string = Aec3ConfigToJsonString(cfg); + EchoCanceller3Config cfg_transformed = + Aec3ConfigFromJsonString(json_string); - // Expect unchanged values to remain default. - EXPECT_EQ(cfg.filter.main.error_floor, - cfg_transformed.filter.main.error_floor); - EXPECT_EQ(cfg.ep_strength.default_len, - cfg_transformed.ep_strength.default_len); - EXPECT_EQ(cfg.suppressor.normal_tuning.mask_lf.enr_suppress, - cfg_transformed.suppressor.normal_tuning.mask_lf.enr_suppress); + // Expect an arbitrary subset of values to carry through the transformation. + constexpr float kEpsilon = 1e-4; + EXPECT_NEAR(cfg.filter.main.error_floor, + cfg_transformed.filter.main.error_floor, kEpsilon); + EXPECT_NEAR(cfg.ep_strength.default_len, + cfg_transformed.ep_strength.default_len, kEpsilon); + EXPECT_NEAR(cfg.suppressor.normal_tuning.mask_lf.enr_suppress, + cfg_transformed.suppressor.normal_tuning.mask_lf.enr_suppress, + kEpsilon); + EXPECT_EQ(cfg.delay.down_sampling_factor, + cfg_transformed.delay.down_sampling_factor); + EXPECT_EQ(cfg.filter.shadow_initial.length_blocks, + cfg_transformed.filter.shadow_initial.length_blocks); + EXPECT_NEAR(cfg.suppressor.normal_tuning.mask_hf.enr_suppress, + cfg_transformed.suppressor.normal_tuning.mask_hf.enr_suppress, + kEpsilon); + } +} - // Expect changed values to carry through the transformation. - EXPECT_EQ(cfg.delay.down_sampling_factor, - cfg_transformed.delay.down_sampling_factor); - EXPECT_EQ(cfg.filter.shadow_initial.length_blocks, - cfg_transformed.filter.shadow_initial.length_blocks); - EXPECT_EQ(cfg.suppressor.normal_tuning.mask_hf.enr_suppress, - cfg_transformed.suppressor.normal_tuning.mask_hf.enr_suppress); +TEST(EchoCanceller3JsonHelpers, IteratedToStringGivesIdenticalStrings) { + Random prng(7297352569824ull); + for (int i = 0; i < 10; ++i) { + EchoCanceller3Config config = GenerateRandomConfig(&prng); + std::string json = Aec3ConfigToJsonString(config); + std::string iterated_json = + Aec3ConfigToJsonString(Aec3ConfigFromJsonString(json)); + EXPECT_EQ(json, iterated_json); + } } } // namespace webrtc