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/base/array_view.h"
+#include "webrtc/base/random.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/event_wrapper.h"
-#include "webrtc/system_wrappers/include/file_wrapper.h"
-#include "webrtc/test/gtest.h"
 
 namespace webrtc {

[peah-webrtc, 2017/01/26 13:20:11]

If you here add
namespace {

constexpr size_t kFrameLengthMs = 10;
constexpr size_t kFramesPerSecond = 1000 / kFrameLengthMs;

}  // namespace

[perkj_webrtc, 2017/01/29 13:25:19]

ok, but will use int
https://google.github.io/styleguide/cppguide.html#Integer_Types

 namespace test {
 
+class FakeAudioDevice::PulsedNoiseCapturer {
+ public:
+  PulsedNoiseCapturer(size_t num_samples, int16_t max_amplitude)
+      : fill_with_zero_(false),
+        random_generator_(1),
+        max_amplitude_(max_amplitude),
+        random_audio_(num_samples),
+        null_audio_(num_samples, 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_ ? null_audio_ : random_audio_;
+  }
+
+ private:
+  bool fill_with_zero_;
+  webrtc::Random random_generator_;
+  const int16_t max_amplitude_;
+  std::vector<int16_t> random_audio_;
+  std::vector<int16_t> null_audio_;

[peah-webrtc, 2017/01/26 13:20:11]

You probably should rename this to silent_audio_, since null is quite
well-defined in the area of programming and means something very different.

[perkj_webrtc, 2017/01/29 13:25:19]

Done.

+};
+
 FakeAudioDevice::FakeAudioDevice(Clock* clock,
-                                 const std::string& filename,
-                                 float speed)
-    : audio_callback_(NULL),
-      capturing_(false),
-      captured_audio_(),
-      playout_buffer_(),
+                                 float speed,
+                                 int sampling_frequency_in_hz)
+    : sampling_frequency_in_hz_(sampling_frequency_in_hz),
+      audio_callback_(NULL),
+      rendering_(false),
+      // Assuming 10ms audio packets.

[peah-webrtc, 2017/01/26 13:20:11]

This should be documented in the header file as it is a contract between the
class and the user of the code. 

I think you should remove the comment (as it is redundant) and document this in
the code instead. I'll add some suggestions in other comments.

Probably you should document that the sample rate can only be 8, 16, 32, 44.1,
or 48 kHz as well.
Preferably, you should add a DCHECK on the correctness of the sample rate here
in the constructor.

[perkj_webrtc, 2017/01/29 13:25:20]

Done.

+      playout_buffer_(sampling_frequency_in_hz_ / 100, 0),

[peah-webrtc, 2017/01/26 13:20:11]

playout_buffer_(rtc::CheckedDivExact(sampling_frequency_in_hz_,
kFramesPerSecond), 0),

[perkj_webrtc, 2017/01/29 13:25:20]

Done.

       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") {}

[peah-webrtc, 2017/01/26 13:20:11]

Please add
RTC_DCHECK(clock); 

(because clock should be a valid pointer, right?)

[perkj_webrtc, 2017/01/29 13:25:19]

now only used by the drifting clock and that checks.

 
 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_);
+  // Assuming 10ms audio packets.
+  capturer_.reset(new FakeAudioDevice::PulsedNoiseCapturer(
+      sampling_frequency_in_hz_ / 100, max_amplitude));

[peah-webrtc, 2017/01/26 13:20:11]

rtc::CheckedDivExact(sampling_frequency_in_hz_,
kFramesPerSecond)

[perkj_webrtc, 2017/01/29 13:25:20]

Done.

+}
+
+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, 10 / speed_));

[peah-webrtc, 2017/01/26 13:20:11]

RTC_CHECK(tick_->StartTimer(true, kFrameLengthMs / speed_));

[perkj_webrtc, 2017/01/29 13:25:19]

Done.

   thread_.Start();
   thread_.SetPriority(rtc::kHighPriority);
   return 0;
 }
 
 int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) {

[peah-webrtc, 2017/01/26 13:20:11]

Is a callback allowed to be null? If it is not, please add a DCHECK on that.

[perkj_webrtc, 2017/01/29 13:25:19]

seems like its called with null  to reconfigure. added dcheck for this.

   rtc::CritScope cs(&lock_);
   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 {
   *delay_ms = 0;

[peah-webrtc, 2017/01/26 13:20:11]

Please add 
RTC_DCHECK(delay_ms);

[perkj_webrtc, 2017/01/29 13:25:19]

A chrome reviewer would say no since it will generate a crash in any case.

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

That is interesting. Can one always rely on this crashing in a controlled way?
What about debuggability, can one always rely on it being straightforward to
locate the location of the crash?

(I'm fine with not using the DCHECK, but curious about the impact.)

   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);
+    if (capturer_) {
+      // Capture 10ms of audio. 2 bytes per sample.
+      rtc::ArrayView<const int16_t> audio_data = capturer_->Capture();
       uint32_t new_mic_level;

[peah-webrtc, 2017/01/26 13:25:18]

Please initialize new_mic_level.

[perkj_webrtc, 2017/01/29 13:25:20]

Done.

-      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;
+      RTC_CHECK_EQ(
+          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_) {
+      // Assuming 10ms audio packet size.
+      size_t samples_needed = sampling_frequency_in_hz_ / 100;

[peah-webrtc, 2017/01/26 13:20:12]

With the code construct below, you can skip the comment, ensure that you don't
get any wrong number of samples and can avoid having related constants in the
code.

size_t samples_needed = rtc::CheckedDivExact(sampling_frequency_in_hz_,
kFramesPerSecond);

[perkj_webrtc, 2017/01/29 13:25:20]

I removed all this weird stuff since the clock has never been used.
last_playout_ms_ has not been updated. I now always request the same number of
samples.

       int64_t now_ms = clock_.TimeInMilliseconds();
       uint32_t time_since_last_playout_ms = now_ms - last_playout_ms_;

[peah-webrtc, 2017/01/26 13:20:12]

I don't see you ever updating last_playout_ms_. How does this really work then?
Afaics, the if-statement below should always be false (since last_playout_ms_ is
initialized to -1, which in turn means that samples_needed is always 10 ms in
length, right?

[perkj_webrtc, 2017/01/29 13:25:19]

Acknowledged.

       if (last_playout_ms_ > 0 && time_since_last_playout_ms > 0) {

[peah-webrtc, 2017/01/26 13:20:11]

It would actually be fine to initialize  last_playout_ms_ to 0 (instead of to -1
as it is now).

In that case you can reduce the if-statement to
if (time_since_last_playout_ms > 0) {

Furthermore, with that there is no risk that time_since_last_playout_ms wraps
around due to last_playout_ms_ being -1 and now_ms being 0 (which I think it can
be according to the specification of DriftingClock).

[perkj_webrtc, 2017/01/29 13:25:20]

Acknowledged.

-        samples_needed = std::min(
-            static_cast<size_t>(kFrequencyHz / time_since_last_playout_ms),
-            kBufferSizeBytes / 2);
+        samples_needed =
+            std::min(static_cast<size_t>(sampling_frequency_in_hz_ /

[peah-webrtc, 2017/01/26 13:20:11]

If you change sampling_frequency_in_hz_ to be size_t, you do not need the
static_cast. 

However, I guess you cannot do that due to the NeedMorePlayData call, right? 

Then I think you should instead change time_since_last_playout_ms to be int.

That would shorten this code line significantly.

[perkj_webrtc, 2017/01/29 13:25:19]

NeedMorePlayData actually use size_t but style guide recommend int... 

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

I think the style guide leaves some room for interpretation there. I always get
the feedback that lengths and sizes should be stored as size_t (and hence
consistently write my code accordingly). Afaics, the style guide says it is ok
to do so, but advises that due to the storage requirements int's should be used
when you know that the data fits into an int (right?).

In this case, I personally would use that freedom in the style guide to choose a
variable type that avoids the extra code needed for the cast.

+                                         time_since_last_playout_ms),
+                     playout_buffer_.size());
       }
       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(
+                          samples_needed, 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