AGC2 dummy module: fixed gain param, APM integration, audioproc_f adaptation

webrtc/modules/audio_processing/agc2/gain_controller2.cc

 /*
  *  Copyright (c) 2017 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 "webrtc/modules/audio_processing/agc2/gain_controller2.h"
 
+#include <cmath>
+
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h"
 #include "webrtc/rtc_base/atomicops.h"
 #include "webrtc/rtc_base/checks.h"
+#include "webrtc/rtc_base/safe_minmax.h"
 
 namespace webrtc {
 
-namespace {
-
-constexpr float kGain = 0.5f;
-
-}  // namespace
-
 int GainController2::instance_count_ = 0;
 
-GainController2::GainController2(int sample_rate_hz)
-    : sample_rate_hz_(sample_rate_hz),
-      data_dumper_(new ApmDataDumper(
-        rtc::AtomicOps::Increment(&instance_count_))),
-      digital_gain_applier_(),
-      gain_(kGain) {
-  RTC_DCHECK(sample_rate_hz_ == AudioProcessing::kSampleRate8kHz ||
-             sample_rate_hz_ == AudioProcessing::kSampleRate16kHz ||
-             sample_rate_hz_ == AudioProcessing::kSampleRate32kHz ||
-             sample_rate_hz_ == AudioProcessing::kSampleRate48kHz);
-  data_dumper_->InitiateNewSetOfRecordings();
-  data_dumper_->DumpRaw("gain_", 1, &gain_);
+GainController2::GainController2()
+    : data_dumper_(new ApmDataDumper(instance_count_)),

[peah-webrtc, 2017/09/15 07:44:25]

You need to increase instance_count_ for each new instance, otherwise the
datafiles won't be unique for each instance.

Note that that increment needs to be done in an atomic manner to avoid race
conditions:
new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))), 

[AleBzk, 2017/09/29 09:39:06]

Done, but note that I took LC as reference, in which no atomic increment is used
(see
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process...).
I think it's the same case for AGC2.

+      sample_rate_hz_(0),

[peah-webrtc, 2017/09/15 07:44:25]

Please initialize the sample rate to a valid rate.

[AleBzk, 2017/09/29 09:39:06]

Done.

+      fixed_gain_(1.f) {
+  ++instance_count_;

[peah-webrtc, 2017/09/15 07:44:25]

Please remove this increase and instead do it as above.

[AleBzk, 2017/09/29 09:39:06]

Done.

 }
 
 GainController2::~GainController2() = default;
 
+void GainController2::Initialize(int sample_rate_hz) {
+  RTC_DCHECK(sample_rate_hz == AudioProcessing::kSampleRate8kHz ||
+             sample_rate_hz == AudioProcessing::kSampleRate16kHz ||
+             sample_rate_hz == AudioProcessing::kSampleRate32kHz ||
+             sample_rate_hz == AudioProcessing::kSampleRate48kHz);
+  data_dumper_->InitiateNewSetOfRecordings();
+  data_dumper_->DumpRaw("fixed gain (linear)", fixed_gain_);

[peah-webrtc, 2017/09/15 07:44:25]

Does filenames with spaces and parenthesis always work?
My suggestion would be to anyway avoid that, as you'll need to escape those in
terminals to get them to work. 

[AleBzk, 2017/09/29 09:39:06]

Done.

+  sample_rate_hz_ = sample_rate_hz;
+}
+
 void GainController2::Process(AudioBuffer* audio) {
+  if (fixed_gain_ == 1.f)
+    return;
+
+  bool saturated_frame = false;
   for (size_t k = 0; k < audio->num_channels(); ++k) {
-    auto channel_view = rtc::ArrayView<float>(
-        audio->channels_f()[k], audio->num_frames());
-    digital_gain_applier_.Process(gain_, channel_view);
+    for (size_t j = 0; j < audio->num_frames(); ++j) {
+      audio->channels_f()[k][j] = rtc::SafeClamp(
+          fixed_gain_ * audio->channels_f()[k][j], -32768.f, 32767.f);
+      if (audio->channels_f()[k][j] == -32768.f ||

[peah-webrtc, 2017/09/15 07:44:25]

This is only used for the purpose of the DumpRaw command below, right?

I don't think it makes sense to do this just for the purpose of debugging. This
will incur a nonnegligible cost even during runtime, which I don't think makes
sense for this.

The alternative would be to put that code under build flags, but if possible
that should be avoided I think as it clutters the code.

[AleBzk, 2017/09/29 09:39:06]

Done.

+          audio->channels_f()[k][j] == 32767.f) {
+        saturated_frame = true;
+      }
+    }
   }
+
+  if (saturated_frame) {
+    data_dumper_->DumpRaw("saturated frame detected", true);
+  }
+}
+
+void GainController2::ApplyConfig(
+    const AudioProcessing::Config::GainController2& config) {
+  RTC_DCHECK(Validate(config));
+  fixed_gain_ = (config.fixed_gain_db == 0.f)

[peah-webrtc, 2017/09/15 07:44:25]

The parentheses are not needed here.

[AleBzk, 2017/09/29 09:39:06]

Done.

+                    ? 1.f
+                    : std::pow(10.0, config.fixed_gain_db / 20.0);
 }
 
 bool GainController2::Validate(
     const AudioProcessing::Config::GainController2& config) {
-  return true;
+  return config.fixed_gain_db >= 0.f;
 }
 
 std::string GainController2::ToString(
     const AudioProcessing::Config::GainController2& config) {
   std::stringstream ss;
   ss << "{"
-     << "enabled: " << (config.enabled ? "true" : "false") << "}";
+     << "enabled: " << (config.enabled ? "true" : "false") << ", "
+     << "fixed_gain_dB: " << config.fixed_gain_db << "}";
   return ss.str();
 }
 
 }  // namespace webrtc