audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/fake_recording_device_unittest.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 <sstream>
+#include <string>
+#include <vector>
+
+#include "webrtc/base/array_view.h"
+#include "webrtc/modules/audio_processing/test/fake_recording_device.h"
+#include "webrtc/test/gtest.h"
+
+namespace webrtc {
+
+namespace {
+
+using MappingKind = FakeRecordingDevice::LevelToScalingMappingKind;
+
+const std::vector<MappingKind> kMappingKindsToTest =

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

I suggest adding

TODO(...): add new mapping kinds here as they are added to
LevelToScalingMappingKind.

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

Done.

+    {MappingKind::kIdentity, MappingKind::kLinear};
+
+const std::vector<std::vector<float>> kTestMultiChannelSamples{
+    std::vector<float>{-10.0, -1.0, -0.1, 0.0, 0.1, 1.0, 10.0}};
+
+// Creates a vector of ArrayView instances pointing to distinct array of floats.

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

Comment is wrong. Should be 'vector of *vectors*'.

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

Done.

+std::vector<std::vector<float>> CreateOutputMultiChannelBuffer(
+    const size_t num_channels, const size_t num_samples) {
+  return std::vector<std::vector<float>>(
+      num_channels, std::vector<float>(num_samples, 0.0f));
+}

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

I think we improve readability by removing this function. Compare

auto data = CreateOutputMultiChannelBuffer(channels, samples); 

auto data = std::vector<std::vector<float>(channels, std::vector<float>(samples,
0));

In the first case, it's not immediately clear what type data is and what kind of
buffer the function returns. The reader has to check its implementation.

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

Thanks for this comment.

There's no need anymore for this function since, as you suggest further down, I
can just make a deep copy of kTestMultiChannelSamples.

+
+// Returns a vector of ArrayView items of const floats.
+std::vector<rtc::ArrayView<const float>> VecVecToVecArrayViewOfConst(
+    const std::vector<std::vector<float>>& vectorOfVectors) {
+  std::vector<rtc::ArrayView<const float>> vectorOfArrayViews;
+  for (const auto& v : vectorOfVectors) {
+    vectorOfArrayViews.emplace_back(rtc::MakeArrayView(v.data(), v.size()));

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

Does .emplace_back(v) work? I think ArrayView has a std::vector c-tor.

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

Done.

+  }
+  return vectorOfArrayViews;
+}
+
+// Returns a vector of ArrayView items of non-const floats.
+std::vector<rtc::ArrayView<float>> VecVecToVecArrayView(
+    std::vector<std::vector<float>>* vectorOfVectors) {
+  std::vector<rtc::ArrayView<float>> vectorOfArrayViews;
+  for (auto& v : *vectorOfVectors) {
+    vectorOfArrayViews.emplace_back(rtc::MakeArrayView(v.data(), v.size()));

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

Change to emplace_back(v) if it works.

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

Done.

+  }
+  return vectorOfArrayViews;
+}
+
+// Checks that the samples modified using monotonic level values are also
+// monotonic.
+void CheckIfMonotoneSamplesModules(
+    const std::vector<rtc::ArrayView<float>>& prev,
+    const std::vector<rtc::ArrayView<float>>& curr) {
+  bool valid = true;
+  for (size_t i = 0; i < prev.size(); ++i) {
+    for (size_t j = 0; j < prev[i].size(); ++j) {
+      valid = std::fabs(prev[i][j]) <= std::fabs(curr[i][j]);
+      if (!valid) { break; }
+    }
+    if (!valid) { break; }
+  }
+  EXPECT_TRUE(valid);
+}
+
+// Checks that the samples in each pair have the same sign unless the sample in
+// |dst| is zero (because of zero gain).
+void CheckSameSign(
+    const std::vector<rtc::ArrayView<const float>>& src,
+    const std::vector<rtc::ArrayView<float>>& dst) {
+  auto fsgn = [](float x) { return static_cast<int>

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

const auto

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

Done.

+      ((x < 0) ? -1 : ((x > 0) ? 1 : 0)); };

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

I prefer if / else if / else to two nested ternary operators. Also, why
'static_cast<int>'?

if x < 0: return -1;
else if x == 0: return 0
else: return 1

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

I thought that static_cast <int> was safer since the lambda function is defined
with auto.
But it works without it as well, so let's get rid of it.

Due to this change, everything fits in 1 line.
I'd keep the nested way, which is also used elsewhere (see
https://cs.chromium.org/search/?q=%5C?.%2B%5C:.%2B%5C?+package:%5Echromium$+f...).

+  bool valid = true;
+  for (size_t i = 0; i < src.size(); ++i) {
+    for (size_t j = 0; j < src[i].size(); ++j) {
+      valid = dst[i][j] == 0.0f || fsgn(src[i][j]) == fsgn(dst[i][j]);
+      if (!valid) { break; }
+    }
+    if (!valid) { break; }
+  }
+  EXPECT_TRUE(valid);
+}
+
+std::string MappingKindToString(MappingKind kind) {
+  std::ostringstream ss;
+  ss << "simulated microphone kind code: " << static_cast<size_t>(kind);
+  return ss.str();
+}
+
+std::string AnalogLevelToString(int level) {
+  std::ostringstream ss;
+  ss << "analog level: " << level;
+  return ss.str();
+}
+
+}  // namespace
+
+TEST(AnalogVolumeMapper, CheckAnalogLevelSetGet) {
+  webrtc::FakeRecordingDevice level_mapper(MappingKind::kIdentity);
+  level_mapper.set_analog_level(0);
+  EXPECT_EQ(0, level_mapper.analog_level());
+  level_mapper.set_analog_level(255);
+  EXPECT_EQ(255, level_mapper.analog_level());
+}
+
+TEST(AnalogVolumeMapper, CheckHelperFunctions) {
+  // Check read.
+  auto src_multichannel_samples_view = VecVecToVecArrayViewOfConst(

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

Should be 'const auto' because we don't write to it, I think.

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

Done.

+      kTestMultiChannelSamples);
+  EXPECT_EQ(kTestMultiChannelSamples[0][0],
+            src_multichannel_samples_view[0][0]);
+
+  // Check sizes.
+  auto dst_multichannel_samples = CreateOutputMultiChannelBuffer(
+      src_multichannel_samples_view.size(),
+      src_multichannel_samples_view[0].size());
+  auto dst_multichannel_samples_view = VecVecToVecArrayView(
+      &dst_multichannel_samples);
+  EXPECT_EQ(kTestMultiChannelSamples.size(),  // Same number of channels.
+            dst_multichannel_samples_view.size());
+  EXPECT_EQ(kTestMultiChannelSamples[0].size(),  // Same number of samples.
+            dst_multichannel_samples_view[0].size());
+
+  // Check write.
+  dst_multichannel_samples[0][1] = -5.0f;
+  EXPECT_EQ(dst_multichannel_samples[0][0],
+            dst_multichannel_samples_view[0][0]);
+}
+
+TEST(AnalogVolumeMapper, GainCurveShouldBeMonotone) {
+  // Create test samples.
+  auto src_multichannel_samples_view = VecVecToVecArrayViewOfConst(
+      kTestMultiChannelSamples);
+
+  // Create output buffers.
+  auto dst_multichannel_samples_prev = CreateOutputMultiChannelBuffer(

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

Suggest instead copy 'dst = kTestMultiChannelSamples'.

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

Great point!
Much simpler now and no need for CreateOutputMultiChannelBuffer() anymore.

+      src_multichannel_samples_view.size(),
+      src_multichannel_samples_view[0].size());
+  auto dst_multichannel_samples_prev_view = VecVecToVecArrayView(
+      &dst_multichannel_samples_prev);
+  auto dst_multichannel_samples = CreateOutputMultiChannelBuffer(

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

And also copy here.

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

Done.

+      src_multichannel_samples_view.size(),
+      src_multichannel_samples_view[0].size());
+  auto dst_multichannel_samples_view = VecVecToVecArrayView(
+      &dst_multichannel_samples);
+
+  // Test different mappings.
+  for (auto kind : kMappingKindsToTest) {

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

Nit: prefer 'const auto &' to plain 'auto' if possible. It doesn't make a
difference here, but a reader may think 'why does it have to be plain auto? is
something copied? what is kMappingKindsToTest' etc.

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

Done.

+    SCOPED_TRACE(MappingKindToString(kind));
+    webrtc::FakeRecordingDevice level_mapper(kind);
+
+    // Apply lowest analog level.
+    level_mapper.set_analog_level(0);
+    level_mapper.ProcessStream(
+        src_multichannel_samples_view, dst_multichannel_samples_prev_view);
+
+    // Increment analog level to check monotonicity.
+    for (int i = 1; i <= 255; ++i) {
+      SCOPED_TRACE(AnalogLevelToString(i));
+      level_mapper.set_analog_level(i);
+      level_mapper.ProcessStream(
+          src_multichannel_samples_view, dst_multichannel_samples_view);
+      // TODO(alessiob): The test below is designed for state-less recording
+      // devices. If, for instance, soft-clipping is used, the test might need
+      // to be redesigned.
+      CheckIfMonotoneSamplesModules(
+          dst_multichannel_samples_prev_view, dst_multichannel_samples_view);
+      dst_multichannel_samples_prev_view.swap(dst_multichannel_samples_view);

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

I think the naming doesn't represent how the vectors are used. Every second
iteration, 'prev_view' points to '_samples' and 'samples_view' points to
'_prev'. The vectors '_prev/_samples' are rather working buffers. WDYT re this
instead:

// Update prev and views:
dst_multichannel_samples_prev = dst_multichannel_samples;
dst_multichannel_samples_prev_view = ..ToArrayView(_prev);

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

Done.

+    }
+  }
+}
+
+TEST(AnalogVolumeMapper, GainCurveShouldNotChangeSign) {
+  // Create test samples.
+  auto src_multichannel_samples_view = VecVecToVecArrayViewOfConst(

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

const auto

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

Done.

+      kTestMultiChannelSamples);
+
+  // Create output buffer.
+  auto dst_multichannel_samples = CreateOutputMultiChannelBuffer(
+      src_multichannel_samples_view.size(),
+      src_multichannel_samples_view[0].size());
+  auto dst_multichannel_samples_view = VecVecToVecArrayView(
+      &dst_multichannel_samples);
+
+  // Test different mappings.
+  for (auto kind : kMappingKindsToTest) {
+    SCOPED_TRACE(MappingKindToString(kind));
+    webrtc::FakeRecordingDevice level_mapper(kind);
+
+    for (int i = 0; i <= 255; ++i) {
+      SCOPED_TRACE(AnalogLevelToString(i));
+      level_mapper.set_analog_level(i);
+      level_mapper.ProcessStream(
+          src_multichannel_samples_view, dst_multichannel_samples_view);
+      // TODO(alessiob): The test below is designed for state-less recording
+      // devices. If, for instance, soft-clipping is used, the test might need
+      // to be redesigned.
+      CheckSameSign(
+          src_multichannel_samples_view, dst_multichannel_samples_view);
+    }
+  }
+}
+
+}  // namespace webrtc