Reland of added functionality for specifying the initial signal level to use for the gain estimation

webrtc/modules/audio_processing/audio_processing_unittest.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include <math.h>
 #include <stdio.h>
 
 #include <algorithm>
 #include <limits>
 #include <memory>
 #include <queue>
 
+#include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/base/arraysize.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/common_audio/include/audio_util.h"
 #include "webrtc/common_audio/resampler/include/push_resampler.h"
 #include "webrtc/common_audio/resampler/push_sinc_resampler.h"
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
+#include "webrtc/modules/audio_processing/audio_processing_impl.h"
 #include "webrtc/modules/audio_processing/beamformer/mock_nonlinear_beamformer.h"
 #include "webrtc/modules/audio_processing/common.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
+#include "webrtc/modules/audio_processing/level_controller/lc_constants.h"

[the sun, 2016/09/30 08:51:16]

please don't abbreviate file names

[peah-webrtc, 2016/10/03 09:52:13]

Done.

 #include "webrtc/modules/audio_processing/test/protobuf_utils.h"
 #include "webrtc/modules/audio_processing/test/test_utils.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/system_wrappers/include/event_wrapper.h"
 #include "webrtc/system_wrappers/include/trace.h"
 #include "webrtc/test/testsupport/fileutils.h"
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
-#include "gtest/gtest.h"
 #include "external/webrtc/webrtc/modules/audio_processing/test/unittest.pb.h"
 #else
-#include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/modules/audio_processing/unittest.pb.h"
 #endif
 
 namespace webrtc {
 namespace {
 
 // TODO(ekmeyerson): Switch to using StreamConfig and ProcessingConfig where
 // applicable.
 
 // TODO(bjornv): This is not feasible until the functionality has been
@@ skipping 2718 matching lines @@
                     std::tr1::make_tuple(16000, 44100, 16000, 44100, 15, 15),
                     std::tr1::make_tuple(16000, 32000, 48000, 32000, 25, 35),
                     std::tr1::make_tuple(16000, 32000, 32000, 32000, 25, 0),
                     std::tr1::make_tuple(16000, 32000, 16000, 32000, 25, 20),
                     std::tr1::make_tuple(16000, 16000, 48000, 16000, 35, 20),
                     std::tr1::make_tuple(16000, 16000, 32000, 16000, 35, 20),
                     std::tr1::make_tuple(16000, 16000, 16000, 16000, 0, 0)));
 #endif
 
 }  // namespace
+
+TEST(ApmConfiguration, DefaultBehavior) {
+  // Verify that the level controller is default off, it can be activated using
+  // the config, and that the default initial level is maintained after the
+  // config has been applied.
+  const float kTolerance = 0.0001f;
+  std::unique_ptr<AudioProcessingImpl> apm(
+      new AudioProcessingImpl(webrtc::Config()));
+  AudioProcessing::Config config;
+  EXPECT_FALSE(apm->config_.level_controller.enabled);
+  // TODO(peah): Add test for the existence of the level controller object once
+  // that is created only when that is specified in the config.
+  // TODO(peah): Remove the testing for
+  // apm->capture_nonlocked_.level_controller_enabled once the value in config_
+  // is instead used to activate the level controller.
+  EXPECT_FALSE(apm->capture_nonlocked_.level_controller_enabled);
+  EXPECT_NEAR(kTargetLcPeakLeveldBFS,
+              apm->config_.level_controller.initial_peak_level_dbfs,
+              kTolerance);
+  config.level_controller.enabled = true;
+  apm->ApplyConfig(config);
+  EXPECT_TRUE(apm->config_.level_controller.enabled);

[the sun, 2016/09/30 08:51:16]

Aren't you testing this in ValidConfigBehavior? Dedupe?

[peah-webrtc, 2016/10/03 09:52:13]

No, if I understand your question correctly, I don't think do that.

-What DefaultBehavior does is to ensure that the default values are applied when
no other values have been specified. The intention of this test is basically to
doublecheck that the default values are not modified by accident.
-What ValidConfigBehavior does is to ensure that specified values are applied
for a valid config. The intention of this test is to ensure that specified
settings actually take effect.
-What InValidConfigBehavior does is to ensure that the specified fallback
settings are applied when the config is not valid. The intention of this test is
to ensure that the proper fallback behavior is used.

I agree that DefaultBehavior and ValidConfigBehavior are very similar but they
actually do slightly different things. WDYT?

[the sun, 2016/10/06 07:21:13]

Acknowledged.

+  // TODO(peah): Add test for the existence of the level controller object once
+  // that is created only when the that is specified in the config.
+  // TODO(peah): Remove the testing for
+  // apm->capture_nonlocked_.level_controller_enabled once the value in config_
+  // is instead used to activate the level controller.
+  EXPECT_TRUE(apm->capture_nonlocked_.level_controller_enabled);
+  EXPECT_NEAR(kTargetLcPeakLeveldBFS,
+              apm->config_.level_controller.initial_peak_level_dbfs,
+              kTolerance);
+}
+
+TEST(ApmConfiguration, ValidConfigBehavior) {
+  // Verify that the initial level can be specified and is retained after the
+  // config has been applied.
+  const float kTolerance = 0.0001f;
+  std::unique_ptr<AudioProcessingImpl> apm(
+      new AudioProcessingImpl(webrtc::Config()));
+  AudioProcessing::Config config;
+  config = AudioProcessing::Config();
+  config.level_controller.initial_peak_level_dbfs = -50.f;
+  apm->ApplyConfig(config);
+  EXPECT_FALSE(apm->config_.level_controller.enabled);
+  // TODO(peah): Add test for the existence of the level controller object once
+  // that is created only when the that is specified in the config.
+  // TODO(peah): Remove the testing for
+  // apm->capture_nonlocked_.level_controller_enabled once the value in config_
+  // is instead used to activate the level controller.
+  EXPECT_FALSE(apm->capture_nonlocked_.level_controller_enabled);
+  EXPECT_NEAR(-50.f, apm->config_.level_controller.initial_peak_level_dbfs,
+              kTolerance);
+}
+
+TEST(ApmConfiguration, InValidConfigBehavior) {
+  // Verify that the config is properly reset when nonproper values are applied
+  // for the initial level.
+  const float kTolerance = 0.0001f;
+  std::unique_ptr<AudioProcessingImpl> apm(
+      new AudioProcessingImpl(webrtc::Config()));
+  AudioProcessing::Config config;
+  config = AudioProcessing::Config();
+  config.level_controller.enabled = true;
+  config.level_controller.initial_peak_level_dbfs = -101.f;
+  apm->ApplyConfig(config);
+  EXPECT_FALSE(apm->config_.level_controller.enabled);
+  // TODO(peah): Add test for the existence of the level controller object once
+  // that is created only when the that is specified in the config.
+  // TODO(peah): Remove the testing for
+  // apm->capture_nonlocked_.level_controller_enabled once the value in config_
+  // is instead used to activate the level controller.
+  EXPECT_FALSE(apm->capture_nonlocked_.level_controller_enabled);
+  EXPECT_NEAR(kTargetLcPeakLeveldBFS,
+              apm->config_.level_controller.initial_peak_level_dbfs,
+              kTolerance);
+
+  apm.reset(new AudioProcessingImpl(webrtc::Config()));

[the sun, 2016/09/30 08:51:16]

So here's either a separate test case, or a comment is needed to explain why a
new APM is created.

[peah-webrtc, 2016/10/03 09:52:13]

I added comments about what is being done. Ideally I'd like a separate TEST for
that but as that would then as well need to be explicitly added as a friend to
AudioProcessingImpl I think it is better to bundle all the valid, invalid and
default configurations tests into these three tests.

WDYT?

[the sun, 2016/10/06 07:21:13]

Acknowledged.

+  config = AudioProcessing::Config();
+  config.level_controller.enabled = true;
+  config.level_controller.initial_peak_level_dbfs = 1.f;
+  apm->ApplyConfig(config);
+  EXPECT_FALSE(apm->config_.level_controller.enabled);
+  // TODO(peah): Add test for the existence of the level controller object once
+  // that is created only when that is specified in the config.
+  // TODO(peah): Remove the testing for
+  // apm->capture_nonlocked_.level_controller_enabled once the value in config_
+  // is instead used to activate the level controller.
+  EXPECT_FALSE(apm->capture_nonlocked_.level_controller_enabled);
+  EXPECT_NEAR(kTargetLcPeakLeveldBFS,
+              apm->config_.level_controller.initial_peak_level_dbfs,
+              kTolerance);
+}
+
 }  // namespace webrtc