Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest

webrtc/modules/audio_device/audio_device_unittest.cc

Index: webrtc/modules/audio_device/audio_device_unittest.cc
diff --git a/webrtc/modules/audio_device/audio_device_unittest.cc b/webrtc/modules/audio_device/audio_device_unittest.cc
index de9c197945d87ba3b2e681ed204c1a7e8844604d..71ff29b7ada310e73927283d201790bb62e26f0f 100644
--- a/webrtc/modules/audio_device/audio_device_unittest.cc
+++ b/webrtc/modules/audio_device/audio_device_unittest.cc
@@ -10,9 +10,13 @@
 
 #include <cstring>
 
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/buffer.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_device/audio_device_impl.h"
 #include "webrtc/modules/audio_device/include/audio_device.h"
 #include "webrtc/modules/audio_device/include/mock_audio_transport.h"
@@ -48,9 +52,13 @@ namespace {
 
 // Number of callbacks (input or output) the tests waits for before we set
 // an event indicating that the test was OK.
-static const size_t kNumCallbacks = 10;
+static constexpr size_t kNumCallbacks = 10;
 // Max amount of time we wait for an event to be set while counting callbacks.
-static const int kTestTimeOutInMilliseconds = 10 * 1000;
+static constexpr int kTestTimeOutInMilliseconds = 10 * 1000;
+// Average number of audio callbacks per second assuming 10ms packet size.
+static constexpr size_t kNumCallbacksPerSecond = 100;
+// Run the full-duplex test during this time (unit is in seconds).
+static constexpr int kFullDuplexTimeInSec = 5;
 
 enum class TransportType {
   kInvalid,
@@ -58,8 +66,82 @@ enum class TransportType {
   kRecord,
   kPlayAndRecord,
 };
+
+// Interface for processing the audio stream. Real implementations can e.g.
+// run audio in loopback, read audio from a file or perform latency
+// measurements.
+class AudioStream {
+ public:
+  virtual void Write(rtc::ArrayView<const int16_t> source, size_t channels) = 0;
+  virtual void Read(rtc::ArrayView<int16_t> destination, size_t channels) = 0;
+
+  virtual ~AudioStream() = default;
+};
+
 }  // namespace
 
+// Simple first in first out (FIFO) class that wraps a list of 16-bit audio
+// buffers of fixed size and allows Write and Read operations. The idea is to
+// store recorded audio buffers (using Write) and then read (using Read) these
+// stored buffers with as short delay as possible when the audio layer needs
+// data to play out. The number of buffers in the FIFO will stabilize under
+// normal conditions since there will be a balance between Write and Read calls.
+// The container is a std::list container and access is protected with a lock
+// since both sides (playout and recording) are driven by its own thread.
+// Note that, we know by design that the size of the audio buffer will not
+// change over time and that both sides will use the same size.
+class FifoAudioStream : public AudioStream {
+ public:
+  void Write(rtc::ArrayView<const int16_t> source, size_t channels) override {
+    EXPECT_EQ(channels, 1u);
+    Buffer16* buffer = new Buffer16(source.data(), source.size());

[kwiberg-webrtc, 2017/04/05 09:29:12]

Here's a raw pointer that owns stuff. I would advise you to use unique_ptr
instead, but I'm pretty sure that you should just put buffer directly on the
stack. See the comment below on the fifo_ member variable.

[henrika_webrtc, 2017/04/05 14:20:59]

Done.

+    const size_t size = [&] {
+      rtc::CritScope lock(&lock_);
+      fifo_.push_back(buffer);
+      return fifo_.size();
+    }();
+    if (size > max_size_) {
+      max_size_ = size;
+    }
+    write_count_++;
+    written_elements_ += size;
+  }
+
+  void Read(rtc::ArrayView<int16_t> destination, size_t channels) override {
+    EXPECT_EQ(channels, 1u);
+    const size_t bytes_per_buffer = sizeof(int16_t) * destination.size();
+    rtc::CritScope lock(&lock_);
+    if (fifo_.empty()) {
+      memset(destination.data(), 0, bytes_per_buffer);

[kwiberg-webrtc, 2017/04/05 09:29:12]

std::fill(destination.begin(), destination.end(), 0);

That way, you can't mis-calculate the size of the region to fill.

[henrika_webrtc, 2017/04/05 14:21:00]

Done.

+    } else {
+      Buffer16* buffer = fifo_.front();
+      memcpy(destination.data(), buffer->data(), bytes_per_buffer);

[kwiberg-webrtc, 2017/04/05 09:29:12]

RTC_CHECK_EQ(buffer.size(), destination.size());
std::copy(buffer.begin(), buffer.end(), destination.begin());

Again, this makes it easier to not mis-calculate the size of the region.

[henrika_webrtc, 2017/04/05 14:21:00]

Done.

+      fifo_.pop_front();

[kwiberg-webrtc, 2017/04/05 09:29:12]

Here you leak |buffer|. See the comment below on the fifo_ member variable.

[henrika_webrtc, 2017/04/05 14:20:59]

Done.

+    }
+  }
+
+  size_t size() const {
+    rtc::CritScope lock(&lock_);
+    return fifo_.size();
+  }
+
+  size_t max_size() const { return max_size_; }
+
+  size_t average_size() const {
+    return 0.5 + static_cast<float>(written_elements_ / write_count_);
+  }
+
+  using Buffer16 = rtc::BufferT<int16_t>;

[kwiberg-webrtc, 2017/04/05 09:29:12]

You reduce the name from 21 to 8 characters, but readers will have to look up
what "Buffer16" means. I'm guessing this is a bad trade-off, but I'll let you
decide.

[henrika_webrtc, 2017/04/05 14:20:59]

Keeping Buffer16 ;-)

+  rtc::CriticalSection lock_;
+  std::list<Buffer16*> fifo_ GUARDED_BY(lock_);

[kwiberg-webrtc, 2017/04/05 09:29:12]

Who owns these buffers? (It would make more sense for the std::list to own them
directly, in which case this type should be std::list<Buffer16>.)

[henrika_webrtc, 2017/04/05 14:20:59]

Acknowledged.

+  // These members are only modified on the playout thread (in
+  // FifoAudioStream::Write()), and then read by the test method after media
+  // has been stopped. Hence, no lock is needed.

[kwiberg-webrtc, 2017/04/05 09:29:12]

OK. Using a race checker to actually verify this claim would have been nice, but
you don't have to.

[henrika_webrtc, 2017/04/05 14:20:59]

Done.

+  size_t write_count_ = 0;
+  size_t max_size_ = 0;
+  size_t written_elements_ = 0;
+};
+
 // Mocks the AudioTransport object and proxies actions for the two callbacks
 // (RecordedDataIsAvailable and NeedMorePlayData) to different implementations
 // of AudioStreamInterface.
@@ -72,8 +154,11 @@ class MockAudioTransport : public test::MockAudioTransport {
   // implementation where the number of callbacks is counted and an event
   // is set after a certain number of callbacks. Audio parameters are also
   // checked.
-  void HandleCallbacks(rtc::Event* event, int num_callbacks) {
+  void HandleCallbacks(rtc::Event* event,
+                       AudioStream* audio_stream,
+                       int num_callbacks) {
     event_ = event;
+    audio_stream_ = audio_stream;
     num_callbacks_ = num_callbacks;
     if (play_mode()) {
       ON_CALL(*this, NeedMorePlayData(_, _, _, _, _, _, _, _))
@@ -114,6 +199,13 @@ class MockAudioTransport : public test::MockAudioTransport {
                 record_parameters_.frames_per_10ms_buffer());
     }
     rec_count_++;
+    // Write audio data to audio stream object if one has been injected.
+    if (audio_stream_) {
+      audio_stream_->Write(
+          rtc::MakeArrayView(static_cast<const int16_t*>(audio_buffer),
+                             samples_per_channel * channels),
+          channels);
+    }
     // Signal the event after given amount of callbacks.
     if (ReceivedEnoughCallbacks()) {
       event_->Set();
@@ -147,9 +239,17 @@ class MockAudioTransport : public test::MockAudioTransport {
     }
     play_count_++;
     samples_per_channel_out = samples_per_channel;
-    // Fill the audio buffer with zeros to avoid disturbing audio.
-    const size_t num_bytes = samples_per_channel * bytes_per_frame;
-    std::memset(audio_buffer, 0, num_bytes);
+    // Read audio data from audio stream object if one has been injected.
+    if (audio_stream_) {
+      audio_stream_->Read(
+          rtc::MakeArrayView(static_cast<int16_t*>(audio_buffer),
+                             samples_per_channel * channels),
+          channels);
+    } else {
+      // Fill the audio buffer with zeros to avoid disturbing audio.
+      const size_t num_bytes = samples_per_channel * bytes_per_frame;
+      std::memset(audio_buffer, 0, num_bytes);
+    }
     // Signal the event after given amount of callbacks.
     if (ReceivedEnoughCallbacks()) {
       event_->Set();
@@ -186,6 +286,7 @@ class MockAudioTransport : public test::MockAudioTransport {
  private:
   TransportType type_ = TransportType::kInvalid;
   rtc::Event* event_ = nullptr;
+  AudioStream* audio_stream_ = nullptr;
   size_t num_callbacks_ = 0;
   size_t play_count_ = 0;
   size_t rec_count_ = 0;
@@ -324,7 +425,7 @@ TEST_F(AudioDeviceTest, StartStopRecording) {
 TEST_F(AudioDeviceTest, StartPlayoutVerifyCallbacks) {
   SKIP_TEST_IF_NOT(requirements_satisfied());
   MockAudioTransport mock(TransportType::kPlay);
-  mock.HandleCallbacks(event(), kNumCallbacks);
+  mock.HandleCallbacks(event(), nullptr, kNumCallbacks);
   EXPECT_CALL(mock, NeedMorePlayData(_, _, _, _, NotNull(), _, _, _))
       .Times(AtLeast(kNumCallbacks));
   EXPECT_EQ(0, audio_device()->RegisterAudioCallback(&mock));
@@ -338,7 +439,7 @@ TEST_F(AudioDeviceTest, StartPlayoutVerifyCallbacks) {
 TEST_F(AudioDeviceTest, StartRecordingVerifyCallbacks) {
   SKIP_TEST_IF_NOT(requirements_satisfied());
   MockAudioTransport mock(TransportType::kRecord);
-  mock.HandleCallbacks(event(), kNumCallbacks);
+  mock.HandleCallbacks(event(), nullptr, kNumCallbacks);
   EXPECT_CALL(mock, RecordedDataIsAvailable(NotNull(), _, _, _, _, Ge(0u), 0, _,
                                             false, _))
       .Times(AtLeast(kNumCallbacks));
@@ -353,7 +454,7 @@ TEST_F(AudioDeviceTest, StartRecordingVerifyCallbacks) {
 TEST_F(AudioDeviceTest, StartPlayoutAndRecordingVerifyCallbacks) {
   SKIP_TEST_IF_NOT(requirements_satisfied());
   MockAudioTransport mock(TransportType::kPlayAndRecord);
-  mock.HandleCallbacks(event(), kNumCallbacks);
+  mock.HandleCallbacks(event(), nullptr, kNumCallbacks);
   EXPECT_CALL(mock, NeedMorePlayData(_, _, _, _, NotNull(), _, _, _))
       .Times(AtLeast(kNumCallbacks));
   EXPECT_CALL(mock, RecordedDataIsAvailable(NotNull(), _, _, _, _, Ge(0u), 0, _,
@@ -367,4 +468,40 @@ TEST_F(AudioDeviceTest, StartPlayoutAndRecordingVerifyCallbacks) {
   StopPlayout();
 }
 
+// Start playout and recording and store recorded data in an intermediate FIFO
+// buffer from which the playout side then reads its samples in the same order
+// as they were stored. Under ideal circumstances, a callback sequence would
+// look like: ...+-+-+-+-+-+-+-..., where '+' means 'packet recorded' and '-'
+// means 'packet played'. Under such conditions, the FIFO would contain max 1,
+// with an average somewhere in (0,1) depending on how long the packets are
+// buffered. However, under more realistic conditions, the size
+// of the FIFO will vary more due to an unbalance between the two sides.
+// This test tries to verify that the device maintains a balanced callback-
+// sequence by running in loopback for a few seconds while measuring the size
+// (max and average) of the FIFO. The size of the FIFO is increased by the
+// recording side and decreased by the playout side.
+TEST_F(AudioDeviceTest, RunPlayoutAndRecordingInFullDuplex) {
+  SKIP_TEST_IF_NOT(requirements_satisfied());
+  NiceMock<MockAudioTransport> mock(TransportType::kPlayAndRecord);
+  FifoAudioStream audio_stream;
+  mock.HandleCallbacks(event(), &audio_stream,
+                       kFullDuplexTimeInSec * kNumCallbacksPerSecond);
+  EXPECT_EQ(0, audio_device()->RegisterAudioCallback(&mock));
+  // Run both sides in mono to make the loopback packet handling less complex.
+  // The test works for stereo as well; the only requirement is that both sides
+  // use the same configuration.
+  EXPECT_EQ(0, audio_device()->SetStereoPlayout(false));
+  EXPECT_EQ(0, audio_device()->SetStereoRecording(false));
+  StartPlayout();
+  StartRecording();
+  event()->Wait(
+      std::max(kTestTimeOutInMilliseconds, 1000 * kFullDuplexTimeInSec));
+  StopRecording();
+  StopPlayout();
+  // This thresholds is set rather high to accomodate differences in hardware
+  // in several devices. The main idea is to capture cases where a very large
+  // latency is built up.
+  EXPECT_LE(audio_stream.average_size(), 5u);
+}
+
 }  // namespace webrtc