Refactor FakeAudioDevice to have separate methods for starting recording and playout.

webrtc/test/fake_audio_device.cc

 /*
  *  Copyright (c) 2013 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 "webrtc/test/fake_audio_device.h"
 
 #include <algorithm>
 
-#include "webrtc/base/platform_thread.h"
-#include "webrtc/modules/media_file/media_file_utility.h"
-#include "webrtc/system_wrappers/include/clock.h"
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/checks.h"
+#include "webrtc/base/random.h"
 #include "webrtc/system_wrappers/include/event_wrapper.h"
-#include "webrtc/system_wrappers/include/file_wrapper.h"
-#include "webrtc/test/gtest.h"
 
 namespace webrtc {
+
+namespace {
+
+constexpr int kFrameLengthMs = 10;
+constexpr int kFramesPerSecond = 1000 / kFrameLengthMs;
+
+}  // namespace
 namespace test {
 
-FakeAudioDevice::FakeAudioDevice(Clock* clock,
-                                 const std::string& filename,
-                                 float speed)
-    : audio_callback_(NULL),
-      capturing_(false),
-      captured_audio_(),
-      playout_buffer_(),
+// Assuming 10ms audio packets..
+class FakeAudioDevice::PulsedNoiseCapturer {
+ public:
+  PulsedNoiseCapturer(int num_samples_per_frame, int16_t max_amplitude)
+      : fill_with_zero_(false),
+        random_generator_(1),
+        max_amplitude_(max_amplitude),
+        random_audio_(num_samples_per_frame),
+        silent_audio_(num_samples_per_frame, 0) {
+    RTC_DCHECK_GT(max_amplitude, 0);
+  }
+
+  rtc::ArrayView<const int16_t> Capture() {
+    fill_with_zero_ = !fill_with_zero_;
+    if (!fill_with_zero_) {
+      std::generate(random_audio_.begin(), random_audio_.end(), [&]() {
+        return random_generator_.Rand(-max_amplitude_, max_amplitude_);
+      });
+    }
+    return fill_with_zero_ ? silent_audio_ : random_audio_;
+  }
+
+ private:
+  bool fill_with_zero_;
+  webrtc::Random random_generator_;

[peah-webrtc, 2017/01/30 06:45:08]

Since you are in the namespace webrtc, I don't think webrtc:: is needed.

[perkj_webrtc, 2017/01/30 12:02:12]

Done.

+  const int16_t max_amplitude_;
+  std::vector<int16_t> random_audio_;
+  std::vector<int16_t> silent_audio_;
+};
+
+FakeAudioDevice::FakeAudioDevice(float speed, int sampling_frequency_in_hz)
+    : sampling_frequency_in_hz_(sampling_frequency_in_hz),
+      num_samples_per_frame_(
+          rtc::CheckedDivExact(sampling_frequency_in_hz_, kFramesPerSecond)),
+      audio_callback_(nullptr),
+      rendering_(false),
+      playout_buffer_(num_samples_per_frame_, 0),
       speed_(speed),
-      last_playout_ms_(-1),
-      clock_(clock, speed),
       tick_(EventTimerWrapper::Create()),
-      thread_(FakeAudioDevice::Run, this, "FakeAudioDevice"),
-      file_utility_(new ModuleFileUtility(0)),
-      input_stream_(FileWrapper::Create()) {
-  memset(captured_audio_, 0, sizeof(captured_audio_));
-  memset(playout_buffer_, 0, sizeof(playout_buffer_));
-  // Open audio input file as read-only and looping.
-  EXPECT_TRUE(input_stream_->OpenFile(filename.c_str(), true)) << filename;
+      thread_(FakeAudioDevice::Run, this, "FakeAudioDevice") {
+  RTC_DCHECK(
+      sampling_frequency_in_hz == 8000 || sampling_frequency_in_hz == 16000 ||
+      sampling_frequency_in_hz == 32000 || sampling_frequency_in_hz == 44100 ||
+      sampling_frequency_in_hz == 48000);
 }
 
 FakeAudioDevice::~FakeAudioDevice() {
-  Stop();
-
+  StopPlayout();
+  StopRecording();
   thread_.Stop();
 }
 
+int32_t FakeAudioDevice::StartPlayout() {
+  rtc::CritScope cs(&lock_);
+  rendering_ = true;
+  return 0;
+}
+
+int32_t FakeAudioDevice::StopPlayout() {
+  rtc::CritScope cs(&lock_);
+  rendering_ = false;
+  return 0;
+}
+
+void FakeAudioDevice::StartRecordingPulsedNoise(int16_t max_amplitude) {
+  rtc::CritScope cs(&lock_);
+  capturer_.reset(new FakeAudioDevice::PulsedNoiseCapturer(
+      num_samples_per_frame_, max_amplitude));
+}
+
+int32_t FakeAudioDevice::StopRecording() {
+  rtc::CritScope cs(&lock_);
+  capturer_.reset();
+  return 0;
+}
+
 int32_t FakeAudioDevice::Init() {
-  rtc::CritScope cs(&lock_);
-  if (file_utility_->InitPCMReading(*input_stream_.get()) != 0)
-    return -1;
-
-  if (!tick_->StartTimer(true, 10 / speed_))
-    return -1;
+  RTC_CHECK(tick_->StartTimer(true, kFrameLengthMs / speed_));
   thread_.Start();
   thread_.SetPriority(rtc::kHighPriority);
   return 0;
 }
 
 int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) {

[peah-webrtc, 2017/01/30 06:45:08]

Since this is not used anywhere in the class and is private, do you need to
override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I guess
you should just be able to omit this implementation. Or did I miss anything
there?

[perkj_webrtc, 2017/01/30 12:02:12]

? audio_callback_ is used.

[peah-webrtc, 2017/01/30 17:06:39]

I cannot see that but I'm probably missing something obvious here.
Afaics:
1) RegisterAudioCallback is private which means that it may only be used locally
within this class. Since it is never used within this class, it should be unused
code, right?
2) audio_callback_ is initialized to nullptr on line 64 and the only place it
can be set is within this method. Since this method is private and not called
from within this class, I cannot see that it it can be set at all. 

Since  RegisterAudioCallback is never used, and audio_callback_ is never set to
anything but nullptr, it seems like the playout part of this class is still
broken. Is that correct?

[perkj_webrtc, 2017/01/31 06:50:35]

audio_callback_ is used both for recording an playout. 
FakeAudioDevice::RegisterAudioCallback is private, but it is public in the the
AudioDevice interface which it implements.
I tried to make all methods private that I implement from the AudioDevice
interface that is not intended to be used directly by a test that instantiate
and start a FakeAudioDevice. However, webrtc VoiceEngine use a pointer to a
AudioDevice so for the voice engine it is still public.
This distinction is quite common but come up for discussion from time to time. 

[peah-webrtc, 2017/01/31 16:37:32]

True, of course, my mistake. Thanks for the explanation!
Acknowledged. 

   rtc::CritScope cs(&lock_);
+  RTC_DCHECK(callback || audio_callback_ != nullptr);

[peah-webrtc, 2017/01/30 06:45:08]

This DCHECK I don't follow. It basically checks that
callback is not falsy (nullptr) and that audio_callback_ != nullptr, right? (as
|| has a higher precedence than != (or did I get that wrong?).

I guess that what you want to check is RTC_DCHECK(callback || audio_callback_)?
(which is the same, but more consistent)

Another thing that I don't get with this DCHECK is that it allows
RegisterAudioCallback to be called with callback = nullptr if
audio_callback_==nullptr, but not otherwise. Is that intended?

[perkj_webrtc, 2017/01/30 12:02:12]

No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to reset
audio_callback_ to nullptr.

[peah-webrtc, 2017/01/30 17:06:39]

In my mind that is a too strict restriction as that DCHECK will fail if an
attempt is made to reset an audio callback which is already null, which should
be a very harmless thing to do. This probably merits an motivation in the code.

[perkj_webrtc, 2017/01/31 06:50:35]

that is not harmfull but is probably not what the user of an AudioDevice
interface should do and thus we can force that by a dcheck. 

[peah-webrtc, 2017/01/31 16:37:32]

Acknowledged.

   audio_callback_ = callback;
   return 0;
 }
 
 bool FakeAudioDevice::Playing() const {
   rtc::CritScope cs(&lock_);
-  return capturing_;
+  return rendering_;
 }
 
 int32_t FakeAudioDevice::PlayoutDelay(uint16_t* delay_ms) const {

[peah-webrtc, 2017/01/30 06:45:08]

Since PlayoutDelay, and it is not used inside this class, you don't need to
override it and can instead rely on the default implementation in the base class
(or did I miss anything there?)

[perkj_webrtc, 2017/01/30 12:02:12]

We could move the implementation to the base class if we think its worth it. but
to be safe delay_ms should be set to 0 to not affect a/v sync tests. 

+  RTC_DCHECK(delay_ms);
   *delay_ms = 0;
   return 0;
 }
 
 bool FakeAudioDevice::Recording() const {
   rtc::CritScope cs(&lock_);
-  return capturing_;
+  return !!capturer_;
 }
 
 bool FakeAudioDevice::Run(void* obj) {
-  static_cast<FakeAudioDevice*>(obj)->CaptureAudio();
+  static_cast<FakeAudioDevice*>(obj)->ProcessAudio();
   return true;
 }
 
-void FakeAudioDevice::CaptureAudio() {
+void FakeAudioDevice::ProcessAudio() {
   {
     rtc::CritScope cs(&lock_);
-    if (capturing_) {
-      int bytes_read = file_utility_->ReadPCMData(
-          *input_stream_.get(), captured_audio_, kBufferSizeBytes);
-      if (bytes_read <= 0)
-        return;
-      // 2 bytes per sample.
-      size_t num_samples = static_cast<size_t>(bytes_read / 2);
-      uint32_t new_mic_level;
-      EXPECT_EQ(0,
-                audio_callback_->RecordedDataIsAvailable(captured_audio_,
-                                                         num_samples,
-                                                         2,
-                                                         1,
-                                                         kFrequencyHz,
-                                                         0,
-                                                         0,
-                                                         0,
-                                                         false,
-                                                         new_mic_level));
-      size_t samples_needed = kFrequencyHz / 100;
-      int64_t now_ms = clock_.TimeInMilliseconds();
-      uint32_t time_since_last_playout_ms = now_ms - last_playout_ms_;
-      if (last_playout_ms_ > 0 && time_since_last_playout_ms > 0) {
-        samples_needed = std::min(
-            static_cast<size_t>(kFrequencyHz / time_since_last_playout_ms),
-            kBufferSizeBytes / 2);
-      }
+    if (capturer_) {
+      // Capture 10ms of audio. 2 bytes per sample.
+      rtc::ArrayView<const int16_t> audio_data = capturer_->Capture();
+      uint32_t new_mic_level = 0;
+      RTC_CHECK_EQ(

[peah-webrtc, 2017/01/30 06:45:09]

Do we really want to crash this code if the return code differs from 0? 

[perkj_webrtc, 2017/01/30 12:02:12]

yes I think so. We can not use gtest.

+          0, audio_callback_->RecordedDataIsAvailable(
+                 audio_data.data(), audio_data.size(), 2, 1,
+                 sampling_frequency_in_hz_, 0, 0, 0, false, new_mic_level));
+    }
+    if (rendering_) {
       size_t samples_out = 0;
       int64_t elapsed_time_ms = -1;
       int64_t ntp_time_ms = -1;
-      EXPECT_EQ(0,
-                audio_callback_->NeedMorePlayData(samples_needed,
-                                                  2,
-                                                  1,
-                                                  kFrequencyHz,
-                                                  playout_buffer_,
-                                                  samples_out,
-                                                  &elapsed_time_ms,
-                                                  &ntp_time_ms));
+      RTC_CHECK_EQ(0, audio_callback_->NeedMorePlayData(

[peah-webrtc, 2017/01/30 06:45:08]

Do we really want to crash this code if the return code differs from 0? 

[perkj_webrtc, 2017/01/30 12:02:12]

dito

+                          num_samples_per_frame_, 2, 1,
+                          sampling_frequency_in_hz_, playout_buffer_.data(),
+                          samples_out, &elapsed_time_ms, &ntp_time_ms));
     }
   }
   tick_->Wait(WEBRTC_EVENT_INFINITE);
 }
 
-void FakeAudioDevice::Start() {
-  rtc::CritScope cs(&lock_);
-  capturing_ = true;
-}
 
-void FakeAudioDevice::Stop() {
-  rtc::CritScope cs(&lock_);
-  capturing_ = false;
-}
 }  // namespace test
 }  // namespace webrtc