audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/fake_recording_device.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/test/fake_recording_device.h"
+
+#include <algorithm>
+
+#include "webrtc/base/logging.h"
+#include "webrtc/base/ptr_util.h"
+
+namespace webrtc {
+namespace test {
+
+// Abstract class for the different fake recording devices.

[peah-webrtc, 2017/06/29 05:45:27]

The scheme for the clipping and analog gain is quite intricate, but regardless
of that I don't think it is general enough to scale. Therefore, I'd propose (as
I previously suggested) to have a much simpler implementation now until the real
simulation work of the recording device is started, as the implementation herein
is currently only a placeholder functionality for this.

My main concerns are the clipping. As it is now, that is hardcoded in the parent
class but we have discussed clipping solutions which don't comply to the scheme
that is hardcoded. How do you envision that to fit into the framework below? 

[AleBzk, 2017/06/29 11:43:35]

Thanks for these concerns.

First, to improve the clarity, I moved constants and the hard clipping functions
in the anon namespace.
It's good to have them somewhere to avoid code duplication.

Then note that each implementation of FakeRecordingDeviceWorker can implement
its clipping strategy - not obliged to use ClipSampleInt16/ClipSampleFloat.
Hence, I find the current design extremely flexible and also compact. In fact,
FakeRecordingDeviceLinear::ModifySampleFloat() is just 5 lines of code excl.
comments.

Also note that we need ModifySampleInt16/ModifySampleFloat mainly because of the
client code.
Another way is to only implement the float interface and scale in [-2^15, 2^15 -
1] afterwards for int16.

With that being said, if you find that having moved code to the anon namespace
is still insufficient, could you then be more specific when you say that we
could have a much simpler implementation?

[peah-webrtc, 2017/06/29 22:03:59]

With simpler implementation, I rather mean less advanced, since it is yet to be
used and since the work on actually simulating this has not started, which means
it will anyway likely change.

For instance the following would be sufficient for the floating point, 
for (auto & x : float_sample_vector) {
  x = std::min(1.f, std::max(-1.f, mic_level/undo_mic_level));
}

[AleBzk, 2017/07/26 13:42:30]

I removed the hard clipping functions from the anon namespace to avoid that the
reader thinks that hard-clipping is the only possible choice.
In the future, we might add them back if needed.

Regarding the sample code you added in your last comment. Note that we don't
have a simple vector because the samples are encoded either in a ChannelBuffer
or an AudioFrame instance. As I pointed out in a previous comment, in order to
work with vectors we could for instance use ArrayView (or similar). This would
however end up to adding boilerplate and duplicated code, because we need to
convert from ChannelBuffer/AudioFrame to ArrayView while supporting multiple
channels.
This is why, for the time being, I find the current code a good trade-off.

+class FakeRecordingDeviceWorker {
+ private:
+  const int16_t kInt16SampleMin = -32768;
+  const int16_t kInt16SampleMax = 32767;
+  const float kFloatSampleMin = -1.0f;
+  const float kFloatSampleMax = 1.0f;
+ public:
+  FakeRecordingDeviceWorker(
+      const int& mic_level, const rtc::Optional<int>& undo_mic_level)
+          : mic_level_(mic_level), undo_mic_level_(undo_mic_level) {}
+  virtual ~FakeRecordingDeviceWorker() = default;
+  virtual void ModifySampleInt16(int16_t* sample) = 0;
+  virtual void ModifySampleFloat(float* sample) = 0;
+ protected:
+  int16_t ClipSampleInt16(int16_t sample) {
+    return std::max(std::min(sample, kInt16SampleMax), kInt16SampleMin);
+  }
+  float ClipSampleFloat(float sample) {
+    return std::max(std::min(sample, kFloatSampleMax), kFloatSampleMin);
+  }
+  const int& mic_level_;
+  const rtc::Optional<int>& undo_mic_level_;
+};

[AleBzk, 2017/06/22 10:16:01]

This is the class to be implemented for each simulated recording device.
The implementations are below in the anon ns.

Note that FakeRecordingDeviceWorker cannot be defined in the anon ns, because
it's needed in fake_recording_device.h.

+
+namespace {
+
+// Identity fake recording device. The samples are not modified, which is
+// equivalent to a constant gain curve at 1.0 - only used for testing.
+class FakeRecordingDeviceIdentity final : public FakeRecordingDeviceWorker {
+ public:
+  FakeRecordingDeviceIdentity(
+      const int& mic_level, const rtc::Optional<int>& undo_mic_level)
+          : FakeRecordingDeviceWorker(mic_level, undo_mic_level) {}
+  ~FakeRecordingDeviceIdentity() override = default;
+  void ModifySampleInt16(int16_t* sample) override {}
+  void ModifySampleFloat(float* sample) override {}

[AleBzk, 2017/06/22 10:16:01]

This is a recording device that does nothing.

+};
+
+// Linear fake recording device. The gain curve is a linear function mapping the
+// mic levels range [0, 255] to [0.0, 1.0].
+class FakeRecordingDeviceLinear final : public FakeRecordingDeviceWorker {
+ public:
+  FakeRecordingDeviceLinear(
+      const int& mic_level, const rtc::Optional<int>& undo_mic_level)
+          : FakeRecordingDeviceWorker(mic_level, undo_mic_level) {}
+  ~FakeRecordingDeviceLinear() override = default;
+  void ModifySampleInt16(int16_t* sample) override {
+    float sample_f = static_cast<float>(*sample);

[peah-webrtc, 2017/06/29 05:45:27]

You don't need to do the cast here.

[AleBzk, 2017/06/29 11:43:35]

Done.

+
+    if (undo_mic_level_ && *undo_mic_level_ > 0) {
+      // Virtually restore the unmodified microphone level.
+      *sample = ClipSampleInt16(
+          sample_f * static_cast<float>(mic_level_) / static_cast<float>(

[peah-webrtc, 2017/06/29 05:45:27]

There is a bug here I think. A float is  passed to ClipSampleInt16 but since
that parameter is an int16_t I guess the value could be saturated already when
the parameter is passed. I thought the compiler would warn for that though.

[peah-webrtc, 2017/06/29 05:45:27]

Same thing here, no casts are needed.

[AleBzk, 2017/06/29 11:43:35]

Right, thanks. Using goma I missed the compiler warnings.
I tried the code, clipping already occurs just by casting.
However, to avoid issues, I changed ClipSampleInt16 into ClipSampleFloatToInt16,
with the latter accepting a float as input (instead of int16).

[AleBzk, 2017/06/29 11:43:36]

Done.

+              *undo_mic_level_));
+    } else {
+      // Simulate the mic gain only.
+      *sample = ClipSampleInt16(
+          sample_f * static_cast<float>(mic_level_) / 255.0f);
+    }
+  }
+  void ModifySampleFloat(float* sample) override {
+    if (undo_mic_level_ && *undo_mic_level_ > 0) {
+      // Virtually restore the unmodified microphone level.
+      *sample = ClipSampleFloat(
+          *sample * static_cast<float>(mic_level_) / static_cast<float>(

[peah-webrtc, 2017/06/29 05:45:27]

Are the static casts really needed? Since sample is of float, and the conversion
goes from left to right, that case should be automatic by the rules of C++ I
think.

[AleBzk, 2017/06/29 11:43:35]

Done.

+              *undo_mic_level_));
+    } else {
+      // Simulate the mic gain only.
+      *sample = ClipSampleFloat(
+          *sample * static_cast<float>(mic_level_) / 255.0f);

[peah-webrtc, 2017/06/29 05:45:27]

The cast is not needed.

[AleBzk, 2017/06/29 11:43:36]

Done.

+    }
+  }
+};
+
+}  // namespace
+
+FakeRecordingDevice::FakeRecordingDevice(int initial_mic_level, DeviceKind kind)
+    : mic_level_(initial_mic_level) {
+  switch (kind) {
+    case FakeRecordingDevice::DeviceKind::IDENTITY:
+      worker_ = rtc::MakeUnique<FakeRecordingDeviceIdentity>(
+          mic_level_, undo_mic_level_);
+      break;
+    case FakeRecordingDevice::DeviceKind::LINEAR:
+      worker_ = rtc::MakeUnique<FakeRecordingDeviceLinear>(
+          mic_level_, undo_mic_level_);
+      break;
+    default:
+      RTC_NOTREACHED();
+      break;
+  }
+}
+
+FakeRecordingDevice::~FakeRecordingDevice() = default;
+
+void FakeRecordingDevice::SimulateAnalogGain(ChannelBuffer<float>* buffer) {
+  RTC_DCHECK(worker_);
+  size_t number_of_samples = buffer->num_frames();
+  for (size_t i = 0; i < buffer->num_channels(); ++i) {
+    std::for_each(buffer->channels()[i],
+                  buffer->channels()[i] + number_of_samples,
+                  [this](float& x) { worker_->ModifySampleFloat(&x); });

[peah-webrtc, 2017/06/29 05:45:27]

Why not pass the whole vector into ModifySampleFloat?

[AleBzk, 2017/06/29 11:43:35]

Just wanted to decouple FakeRecordingDeviceWorker implementations from a buffer
type (to avoid code duplication across the implementations).
However, the overhead of a lambda invoked for each sample is not that nice
either.

Thanks to your comments I realized that it's better to fully delegate
FakeRecordingDeviceWorker implementations, since a simulated device may have
memory and hence require a different way to iterate through the samples -
depending on the specific buffer type. For instance, AudioFrame has interleaved
samples and therefore care must be taken if we want to simulate soft-clipping
with memory.

WDYT?

[peah-webrtc, 2017/06/29 22:03:59]

I'd say do this as simple as possible now, as you'll anyway need to rewrite this
once the work on simulating the analog microphone starts.

[AleBzk, 2017/07/26 13:42:30]

Yup. Not sure if you've seen the latest PS before you replied.
I think I addressed your comment - and find the new code the simplest option for
the time being.

+  }
+}
+
+void FakeRecordingDevice::SimulateAnalogGain(AudioFrame* buffer) {
+  RTC_DCHECK(worker_);
+  const size_t number_of_samples =
+      buffer->samples_per_channel_ * buffer->num_channels_;
+  RTC_DCHECK_LE(number_of_samples, AudioFrame::kMaxDataSizeSamples);
+  std::for_each(buffer->mutable_data(),
+                buffer->mutable_data() + number_of_samples,
+                [this](int16_t& x) { worker_->ModifySampleInt16(&x); });

[peah-webrtc, 2017/06/29 05:45:27]

Why not just pass the whole vector into ModifySampleInt16? Even though this is
not realtime code, it still makes sense to write it in a fast way if possible,
and I don't see a big gain in terms of readability for making separate calls on
a sample basis.

[AleBzk, 2017/06/29 11:43:35]

Addressed (see previous comment).
Also good to correctly implement the iteration over channels.

+}
+
+}  // namespace test
+}  // namespace webrtc