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/checks.h"
+#include "webrtc/base/optional.h"
+#include "webrtc/modules/include/module_common_types.h"
+
+namespace webrtc {
+
+// Class for simulating an analog gain controller controlled by

[peah-webrtc, 2017/05/05 06:28:41]

I don't think this comment is correct. The class is not simulating the analog
gain controller. It is simulating a recording device. Furthermore, I think it
could do more than abstracting non-linearities in the gain curve so I don't
think that is needed to be specified in the comment.

[AleBzk, 2017/05/05 12:20:18]

Done.

+// webrtc::GainControl. The class wraps a mapping from the current
+// level to a floating point scaling factor with the goal of
+// abstracting non-linearities in the gain curve of real analog
+// microphones. The intended mode of operation is
+//
+// set_analog_level(|level suggested by GainControl|);

[peah-webrtc, 2017/05/05 06:28:41]

This does not really reflect the way it is currently used in the code, right?

[AleBzk, 2017/05/05 12:20:18]

Right. I'll make it more generic.

+// NotifyAudioDeviceLevel(|recorded level of real microphone|); // Optional!
+// ProcessStream(src, dest)
+//
+// In these three calls, the fake device optionally undoes the gain
+// applied by the real microphone. Then in applies a new gain
+// suggested by the AGC.
+class FakeRecordingDevice {
+ public:
+  enum class LevelToScalingMappingKind {
+    kIdentity,  // Always producing 1.0f.
+    kLinear  // A level within [0, 255] is linearly scaled. 0 produces
+             // 0.f, and 255 is 1.0f.

[aleloi, 2017/05/04 12:47:13]

Nit: please indent comments

[AleBzk, 2017/05/05 12:20:18]

Done.

+  };
+
+  explicit FakeRecordingDevice(LevelToScalingMappingKind mapping_kind);
+
+  ~FakeRecordingDevice();
+
+  // Setters/getters for |level_|. The parameter |level| must be
+  // within [0, 255].
+  void set_analog_level(int level);
+  int analog_level() const;
+
+  LevelToScalingMappingKind mapping_kind() const;

[aleloi, 2017/05/04 12:47:13]

Is mapping_kind() used anywhere?

[AleBzk, 2017/05/05 12:20:18]

Nope. Removed.
I had added it when I made the kind non-const.

+
+  void ProcessStream(std::vector<rtc::ArrayView<const float>> src,

[aleloi, 2017/05/04 12:47:14]

Perhaps add a comment along the lines of 'undoes the gain reported by
NotifyAudioDeviceLevel and applies the gain reported by set_analog_level().

[peah-webrtc, 2017/05/05 06:28:41]

Would it be possible to change ProcessStream to something more descriptive? In
APM, ProcessStream is vague by purpose, and the submodule apis have adapted
that, probably both because their operations are not easy to state in one word
but also in order to conform to the pattern.

In this case, we can be more clear on what we do so I'd prefer something more
descriptive. In particular as this is used in AudioProcessingSimulator which is
using are 2 other methods named ProcessStream.

[AleBzk, 2017/05/05 12:20:18]

Right. I went for SimulateAnalogGain()

+                     std::vector<rtc::ArrayView<float>> dest);
+
+  void ProcessStream(const AudioFrame* src, AudioFrame* dest);

[peah-webrtc, 2017/05/05 06:28:41]

I think the usage of this class is more complicated than it needs to.

I'd prefer one single call to simulate the microphone gain application.
Furthermore, I see no need or usage for separated input and output variables.
Instead, that separation causes a lot of boilerplate code to be present in
AudioProcessingSimulator.

My proposal would be to specify the reported gain, the new gain, and the
input/output stream in one single call. They belong together so I don't think
there is any need to separate them:
void Transform(optional<int> mic_level_for_input, AudioFrame* samples).


I think that would significantly simplify the code using this class.

WDYT?

[AleBzk, 2017/05/05 12:20:18]

Thanks for this comment. I think the change you propose fits well with having a
private/protected member in AudioProcessingSimulator to store the suggested mic
level and, optionally, the undo level.

+
+  // Meant to be called before ProcessStream if the source signal
+  // passed to ProcessStream has been recorded from a microphone with
+  // known gain.
+  void NotifyAudioDeviceLevel(int level);
+
+ private:
+  // Computes scaling factor based on both the recorded real device
+  // level, gain curve (which depends on |mapping_kind_) and the

[aleloi, 2017/05/04 12:47:13]

Nit: formatting, |mapping_kind_|)

[AleBzk, 2017/05/05 12:20:18]

Done.

+  // current level.
+  float ComputeCompoundScalingFactor();
+
+  // Compute scaling factor based on current gain curve, which depends
+  // on |mapping_kind_|.
+  float GetScalingFactor(int level) const;
+
+  int level_ = 0;
+  rtc::Optional<int> real_device_level_;
+  const LevelToScalingMappingKind mapping_kind_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_TEST_FAKE_RECORDING_DEVICE_H_