audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/fake_recording_device.h

+/*
+ *  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.
+ */
+
+#ifndef WEBRTC_MODULES_AUDIO_PROCESSING_TEST_FAKE_RECORDING_DEVICE_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_TEST_FAKE_RECORDING_DEVICE_H_
+
+#include <algorithm>
+#include <vector>
+
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/callback.h"
+#include "webrtc/base/checks.h"
+#include "webrtc/common_audio/channel_buffer.h"
+#include "webrtc/modules/include/module_common_types.h"
+
+namespace webrtc {
+

[peah-webrtc, 2017/05/16 12:19:36]

I think it would make sense to use the test namespace, right?

[AleBzk, 2017/05/17 11:52:23]

Done.

+typedef rtc::Callback3<int16_t, int16_t, int, rtc::Optional<int>>
+    SimulatorCallbackInt16;
+typedef rtc::Callback3<float, float, int, rtc::Optional<int>>
+    SimulatorCallbackFloat;
+
+const int16_t kSampleMinInt16 = 255;

[peah-webrtc, 2017/05/16 12:19:36]

These constants are not used in the header file. Please put them into the
anonymous namespace in the source file.

[AleBzk, 2017/05/17 11:52:24]

Done.

+const int16_t kSampleMaxInt16 = -255;
+const float kSampleMinFloat = -1.0f;
+const float kSampleMaxFloat = 1.0f;
+
+// Class for simulating a microphone with analog gain.
+//
+// The intended mode of operation is the following:
+//
+// auto fake_mic = FakeRecordingDevice(
+//     FakeRecordingDevice::MicrophoneKind::kLinear);
+// fake_mic.SimulateAnalogGain(target_level, real_device_level, buffer);
+//
+// The last call optionally "undoes" the gain applied by the real microphone.
+// For instance, when an AEC dump includes the applied mic gain, this value can
+// be used to virtually restore the microphone signal with unmodified level.
+class FakeRecordingDevice final {
+ public:
+  enum class MicrophoneKind {
+    kIdentity,
+    kLinear,
+  };
+
+  explicit FakeRecordingDevice(MicrophoneKind mapping_kind);
+
+  ~FakeRecordingDevice();
+
+  // Simulates the analog gain on an std::vector<rtc::ArrayView<float>> buffer.

[peah-webrtc, 2017/05/16 12:19:36]

The buffer type is stated in the method header: 

Simulates the analog gain on an std::vector<rtc::ArrayView<float>> buffer. ->
Simulates the analog gain on the samples in buffer.

[AleBzk, 2017/05/17 11:52:23]

I would leave it as it is to distinguish the 3 methods by comment.
WDYT?

[peah-webrtc, 2017/05/17 14:52:12]

Lets discuss the three methods first. In general, I think it is of great value
to keep the comments as short as possible. Since this type is quite verbose, and
furthermore also declared 5 lines below the comment, I'd prefer not to include
it in the comment as it makes the comment longer and is redundant.

+  // If |real_device_level| is a valid level, the unmodified mic signal is
+  // virtually restored. To skip the latter step set |real_device_level| to
+  // nullptr.

[peah-webrtc, 2017/05/16 12:19:36]

real_device_level is not a pointer but an optional so you cannot set it to
nullptr.

[AleBzk, 2017/05/17 11:52:23]

Comment changed.

+  void SimulateAnalogGain(int level, rtc::Optional<int> real_device_level,
+                          std::vector<rtc::ArrayView<float>> buffer);

[peah-webrtc, 2017/05/16 12:19:36]

Why do you need 3 methods for SimulateAnalogGain? I can only see 2 being used in
the code.

[AleBzk, 2017/05/17 11:52:24]

I left SimulateAnalogGain(int, rtc::Optional<int>,
std::vector<rtc::ArrayView<float>>) because it's used in the unit tests and from
SimulateAnalogGain(int, rtc::Optional<int>, ChannelBuffer<float>* buffer), which
is just an adapter in order to call the first method.

Also, I thought it was good to leave an interface supporting ArrayView objects
for future use.

WDYT?

[peah-webrtc, 2017/05/17 14:52:12]

I think it always makes sense to keep the API surface as small as possible, and
it is better to let the unittest test the methods that are used rather than this
one which is not. 

I don't think it is good to add api calls for potential future use before they
are implemented. From my experience in most scenarios you end up in either
a) The API is never used, which means that this just becomes code bloat which
makes the code harder to read.
b) The API turned out not to be exactly what you wanted, which means that you
anyway need to rewrite the code.

Do you have any specific use of that API interface? Otherwise I think you should
wait with adding it until you see the need.

+
+  // Simulates the analog gain on a ChannelBuffer<float> buffer.
+  // For further details, see the comment above.
+  void SimulateAnalogGain(int level, rtc::Optional<int> real_device_level,
+                          ChannelBuffer<float>* buffer);
+
+  // Simulates the analog gain on an AudioFrame buffer (fixed point).
+  // For further details, see the comment above.
+  void SimulateAnalogGain(int level, rtc::Optional<int> real_device_level,
+                          AudioFrame* buffer);
+
+ private:
+  // Identity simulator: the samples are not changed.
+  SimulatorCallbackInt16 AnalogGainSimulatorIdentityInt16();
+  SimulatorCallbackFloat AnalogGainSimulatorIdentityFloat();
+
+  // Linear simulator with hard-clipping.
+  SimulatorCallbackInt16 AnalogGainSimulatorLinearInt16();
+  SimulatorCallbackFloat AnalogGainSimulatorLinearFloat();
+
+  int16_t ClipSample(int16_t sample);
+  float ClipSample(float sample);
+
+  const MicrophoneKind mapping_kind_;
+  SimulatorCallbackInt16 simulator_callback_int16_;
+  SimulatorCallbackFloat simulator_callback_float_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_TEST_FAKE_RECORDING_DEVICE_H_