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..a2c05f40a9492fb3304679def17bb4cb3490e995 100644
--- a/webrtc/modules/audio_device/audio_device_unittest.cc
+++ b/webrtc/modules/audio_device/audio_device_unittest.cc
@@ -10,6 +10,7 @@
 
 #include <cstring>
 
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/scoped_ref_ptr.h"
@@ -51,6 +52,10 @@ namespace {
 static const 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;
+// Average number of audio callbacks per second assuming 10ms packet size.
+static const size_t kNumCallbacksPerSecond = 100;
+// Run the full-duplex test during this time (unit is in seconds).
+static const int kFullDuplexTimeInSec = 5;

[kwiberg-webrtc, 2017/04/03 08:40:11]

These two can be constexpr instead of const. Not that important in this specific
case, but a good habit to get into.

[henrika_webrtc, 2017/04/03 15:14:30]

Done.

 
 enum class TransportType {
   kInvalid,
@@ -58,8 +63,107 @@ 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 AudioStreamInterface {

[kwiberg-webrtc, 2017/04/03 08:40:12]

Suggestion: Just "AudioStream". The "Interface" suffix isn't necessary (but if
you really like it, keep it).

[henrika_webrtc, 2017/04/03 15:14:30]

Removed "Interface"

+ public:
+  virtual void Write(const void* source,
+                     size_t samples_per_channel,
+                     size_t channels) = 0;
+  virtual void Read(void* destination,
+                    size_t samples_per_channel,
+                    size_t channels) = 0;

[kwiberg-webrtc, 2017/04/03 08:40:12]

Can you make this a concrete type instead of void? That makes the code easier to
read, and the compiler will catch more errors for you.

Also, strongly consider replacing the raw pointer with ArrayView or something
that keeps track of the length. That makes the code much easier to read, since
you don't have to read the docs to see how long the array is supposed to be.
(And in this case, as in so many others, there aren't even any docs...)

[henrika_webrtc, 2017/04/03 15:14:29]

Done.

+
+  AudioStreamInterface() = default;

[kwiberg-webrtc, 2017/04/03 08:40:11]

This is a pure interface. No constructor needed.

[henrika_webrtc, 2017/04/03 15:14:30]

Done.

+  virtual ~AudioStreamInterface() = 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 AudioStreamInterface {
+ public:
+  FifoAudioStream() : fifo_(new AudioBufferList()) {
+    EXPECT_NE(fifo_.get(), nullptr);
+  }
+  virtual ~FifoAudioStream() { Flush(); }
+
+  void Write(const void* source,
+             size_t samples_per_channel,
+             size_t channels) override {
+    EXPECT_EQ(channels, 1u);
+    const size_t samples_per_buffer = samples_per_channel * channels;
+    const size_t bytes_per_buffer = sizeof(int16_t) * samples_per_buffer;
+    int16_t* memory = new int16_t[samples_per_buffer];

[kwiberg-webrtc, 2017/04/03 08:40:11]

Raw pointers shouldn't own stuff. Use unique_ptr instead. (This is a good place
to use WrapUnique.)

Also, it's less error prone to use rtc::Buffer or std::vector or something that
also tracks its length, instead of a raw heap array. Especially in test code,
where performance isn't critical...

[henrika_webrtc, 2017/04/03 15:14:29]

Switched to unique_ptr. I am unable to see any real benefit by using WrapUniqe.
Can you elaborate?

[kwiberg-webrtc, 2017/04/03 20:44:01]

You avoid having to name the type twice. That's more of an advantage for types
with names longer than "int16_t"...

However, I'll have to retract my suggestion, because WrapUnique doesn't work for
array types. You could use MakeUnique:

  auto buffer = rtc::MakeUnique<int16_t[]>(source.size());

but for some reason this is specified to value-initialize the array (a.k.a.
filling it with zeros). Personally, I would probably not use MakeUnique here.

+    memcpy(static_cast<int16_t*>(&memory[0]), source, bytes_per_buffer);

[kwiberg-webrtc, 2017/04/03 08:40:12]

This cast looks unnecessary. (But if you use rtc::Buffer instead, this memcpy
will go away entirely, since you can give the data as a constructor argument.)

[henrika_webrtc, 2017/04/03 15:14:30]

Let me add first round without using rtc::Buffer first.

+    size_t size(0);
+    {
+      rtc::CritScope lock(&lock_);
+      fifo_->push_back(memory);
+      size = fifo_->size();
+    }

[kwiberg-webrtc, 2017/04/03 08:40:11]

You can avoid mutating the size variable:

const size_t size = [&]{
  rtc::CritScope lock(&lock_);
  fifo_->push_back(memory);
  return fifo_->size();
}();

[henrika_webrtc, 2017/04/03 15:14:29]

Yes we can :-)

+    if (size > max_size_) {
+      max_size_ = size;
+    }
+    write_count_++;
+    written_elements_ += size;
+  }
+
+  void Read(void* destination,
+            size_t samples_per_channel,
+            size_t channels) override {
+    EXPECT_EQ(channels, 1u);
+    const size_t bytes_per_buffer =
+        sizeof(int16_t) * samples_per_channel * channels;
+    rtc::CritScope lock(&lock_);
+    if (fifo_->empty()) {
+      memset(destination, 0, bytes_per_buffer);
+    } else {
+      int16_t* memory = fifo_->front();

[kwiberg-webrtc, 2017/04/03 08:40:11]

Again, definitely use unique_ptr instead of a raw pointer, and consider using
rtc::Buffer instead.

[henrika_webrtc, 2017/04/03 15:14:30]

Using unique_ptr to see if you like it :-)

+      memcpy(destination, static_cast<int16_t*>(&memory[0]), bytes_per_buffer);
+      delete memory;
+      fifo_->pop_front();
+    }
+  }
+
+  size_t size() const { return fifo_->size(); }

[kwiberg-webrtc, 2017/04/03 08:40:11]

You should take lock_ before touching fifo_. See the request for lock
annotations below...

[henrika_webrtc, 2017/04/03 15:14:30]

Done.

+
+  size_t max_size() const { return max_size_; }
+
+  size_t average_size() const {
+    return 0.5 + static_cast<float>(written_elements_ / write_count_);
+  }
+
+ private:
+  void Flush() {
+    if (fifo_) {
+      while (!fifo_->empty()) {
+        delete fifo_->front();
+        fifo_->pop_front();
+      }
+      fifo_->clear();
+    }
+  }
+
+  using AudioBufferList = std::list<int16_t*>;

[kwiberg-webrtc, 2017/04/03 08:40:12]

This should be std::list<std::unique_ptr<int16_t[]>>, or
std::list<rtc::BufferT<int16_t>>. Raw pointers shouldn't own things.

Once you do this, you can get rid of the Flush method.

[henrika_webrtc, 2017/04/03 15:14:29]

Done.

+  rtc::CriticalSection lock_;
+  std::unique_ptr<AudioBufferList> fifo_;

[kwiberg-webrtc, 2017/04/03 08:40:11]

Why the indirection? Can't you have the AudioBufferList as a normal class
member?

Also, I don't think the AudioBufferList alias is very useful. You can probably
get rid of it without making the code any harder to read.

[henrika_webrtc, 2017/04/03 15:14:29]

Acknowledged.

+  size_t write_count_ = 0;
+  size_t max_size_ = 0;
+  size_t written_elements_ = 0;

[kwiberg-webrtc, 2017/04/03 08:40:12]

Please annotate which variables are protected by what.

Also, for the variables that aren't protected by the lock, consider using a race
checker or thread checker. (But if performance isn't an issue, consider
protecting everything with lock_ for simplicity.)

[henrika_webrtc, 2017/04/03 15:14:30]

Done.

+};
+
 // Mocks the AudioTransport object and proxies actions for the two callbacks
 // (RecordedDataIsAvailable and NeedMorePlayData) to different implementations
 // of AudioStreamInterface.
@@ -72,8 +176,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,
+                       AudioStreamInterface* audio_stream,
+                       int num_callbacks) {
     event_ = event;
+    audio_stream_ = audio_stream;
     num_callbacks_ = num_callbacks;
     if (play_mode()) {
       ON_CALL(*this, NeedMorePlayData(_, _, _, _, _, _, _, _))
@@ -114,6 +221,10 @@ 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(audio_buffer, samples_per_channel, channels);
+    }
     // Signal the event after given amount of callbacks.
     if (ReceivedEnoughCallbacks()) {
       event_->Set();
@@ -147,9 +258,14 @@ 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(audio_buffer, samples_per_channel, 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 +302,7 @@ class MockAudioTransport : public test::MockAudioTransport {
  private:
   TransportType type_ = TransportType::kInvalid;
   rtc::Event* event_ = nullptr;
+  AudioStreamInterface* audio_stream_ = nullptr;
   size_t num_callbacks_ = 0;
   size_t play_count_ = 0;
   size_t rec_count_ = 0;
@@ -324,7 +441,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 +455,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 +470,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 +484,37 @@ 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 only contain
+// one packet on average. However, under more realistic conditions, the size

[kwiberg-webrtc, 2017/04/03 08:40:11]

Max 1, average somewhere in (0,1) depending on how long the packets are
buffered.

[henrika_webrtc, 2017/04/03 15:14:30]

Tried to rewrite as you suggest but I might have misunderstood your intention.
Please check again.

[kwiberg-webrtc, 2017/04/03 20:44:01]

It may be better to just say

  "Under such conditions, the FIFO would contain at most one packet."

That the average would be in the range (0,1) is a trivial consequence of that;
we'll probably just confuse people if we try to mention it...

+// 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);
+  std::unique_ptr<FifoAudioStream> audio_stream(new FifoAudioStream());

[kwiberg-webrtc, 2017/04/03 08:40:12]

Why the indirection? It looks like you could put the FifoAudioStream directly on
the stack.

[henrika_webrtc, 2017/04/03 15:14:29]

Done.

+  mock.HandleCallbacks(event(), audio_stream.get(),
+                       kFullDuplexTimeInSec * kNumCallbacksPerSecond);
+  EXPECT_EQ(0, audio_device()->RegisterAudioCallback(&mock));
+  // Run both sides in mono to make the loopback packet handling less complex.
+  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);

[kwiberg-webrtc, 2017/04/03 08:40:11]

This looks like a test that can be expected to flake sometimes. Can that be
avoided, or won't it be a problem?

[henrika_webrtc, 2017/04/03 15:14:29]

I use rather strict conditions to be able to tun this test. No ALSA, no TSAN,
MSAN etc. It should be a very rare case to end up with more than five in the
list (famous last words...)

[kwiberg-webrtc, 2017/04/03 20:44:01]

Acknowledged.

+}
+
 }  // namespace webrtc