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

webrtc/modules/audio_processing/agc2/gain_controller2.cc

Index: webrtc/modules/audio_processing/agc2/gain_controller2.cc
diff --git a/webrtc/modules/audio_processing/agc2/gain_controller2.cc b/webrtc/modules/audio_processing/agc2/gain_controller2.cc
index 20680f64a840eee3aa1c63ee8cb23d8dfdf0dae2..58beda301216a9cce45f6a3c07ceb04e4a0cd955 100644
--- a/webrtc/modules/audio_processing/agc2/gain_controller2.cc
+++ b/webrtc/modules/audio_processing/agc2/gain_controller2.cc
@@ -10,6 +10,8 @@
 
 #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"
@@ -17,48 +19,56 @@
 
 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(const float fixed_gain_db)

[aleloi, 2017/08/15 14:34:02]

The list of args will certainly grow. At some point we'll almost surely need a
config struct. We could change the c-tor to take a 'Config' now, or could wait
until we know more about the design of this new module. 

WDYT?

[peah-webrtc, 2017/08/18 04:51:06]

I agree, and I think we instead need to adopt the ApplyConfig scheme for this
instead of setting the parameters in the constructor, as injecting an AGC will
need to be controlled without going through the constructor.

[AleBzk, 2017/09/14 09:21:55]

Thanks. As per our discussion, I switched to the ApplyConfig() scheme.

[AleBzk, 2017/09/14 09:21:55]

Done.

+    : data_dumper_(new ApmDataDumper(instance_count_)),
+      fixed_gain_(std::pow(10.0, fixed_gain_db / 20.0)) {
+  Initialize(AudioProcessing::kSampleRate48kHz);
+  ++instance_count_;
 }
 
 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);

[aleloi, 2017/08/15 14:34:03]

Why restriction on sample rates? A complete gain controller could be designed
rate-independently. The current code works with all sample rates.

Also, why should the rate be set in an Init method? I think it could be simpler
if the 'sample_rate_hz_' member is const and set in the constructor. If the rate
changes, you re-initialize the whole GainController2.

If I was to decide, I'd make the different sample rates to be template
parameters for the whole AudioProcessing module. If a rate changes for some
reason, you re-create the whole APM with a new set of template params (they have
to come from a fixed list known to the compiler). Advantage: when code runs, the
size of everything is fixed, the compiler can align memory as it wants and
hopefully add SSE optimizations by itself. Disadvantage: possibly gigantic
binaries and possibly slow compile times.

[peah-webrtc, 2017/08/18 04:51:06]

Regarding being sample rate agnostic, I agree, but it is not a must if it turns
out not to be possible.

Regarding the Initialize method, I think it is needed as we cannot change this
by recreating the submodule if we use an injected AGC.

I agree,

[aleloi, 2017/08/18 08:28:31]

Acknowledged. Keep Initialize because re-creating may not be an option.

[AleBzk, 2017/09/14 09:21:54]

Acknowledged.

[AleBzk, 2017/09/14 09:21:55]

This dummy AGC2 module does work at any sample rate, true. However, we want that
the "AGC2-to-be" allows a few audio frame size options, including one for which
44100 Hz leads to a fractional frame length.

You may then wonder why I added Initialize(), since the sample rate is not used
by the fixed gain applier with hard-clipping. The whole purpose of this CL is
integrating AGC2 into APM in preparation for the injectable AGC work. In fact,
this CL has triggered the discussion around the ApplyConfig() + Initialize()
scheme, which is exactly what I wanted to decide with you in this CL.

For the aforementioned reasons, I would prefer to keep Initalize() as well as
the sample rate RTC_DCHECKs.

[AleBzk, 2017/09/14 09:21:55]

Acknowledged.

+  data_dumper_->InitiateNewSetOfRecordings();
+  data_dumper_->DumpRaw("fixed gain (linear)", fixed_gain_);
+  sample_rate_hz_ = sample_rate_hz;
+}
+
 void GainController2::Process(AudioBuffer* audio) {

[aleloi, 2017/08/15 14:34:03]

Can you please add a DCHECK for the correct rate here? In combination with a
test, we could use it to know that settings in AudioProcessing were correctly
applied.

[AleBzk, 2017/09/14 09:21:55]

Unfortunately not. AudioBuffer doesn't have a sample rate member.

+  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] = std::min(
+          32767.f, std::max(-32768.f, fixed_gain_ * audio->channels_f()[k][j]));

[aleloi, 2017/08/15 14:34:03]

I think we can use SafeClamp or similar here:
https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/master/r...

[AleBzk, 2017/09/14 09:21:55]

Done.

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

[aleloi, 2017/08/15 14:34:03]

Does it only make sense to use this module for amplification? I know at least
the audio mixer divides its audio samples by 2 (but that's something we'd like
to avoid doing).

[AleBzk, 2017/09/14 09:21:55]

I'd say yes. I don't see any useful application for a fixed attenuation.

 }
 
 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();
 }