Add the ability to read/write to WAV files in FakeAudioDevice

webrtc/test/file_audio_device.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 "webrtc/test/file_audio_device.h"
+
+#include <algorithm>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/system_wrappers/include/event_wrapper.h"
+
+namespace webrtc {
+
+namespace {
+
+constexpr int kFrameLengthMs = 10;
+constexpr int kFramesPerSecond = 1000 / kFrameLengthMs;
+
+}  // namespace
+namespace test {
+
+FileAudioDevice::FileAudioDevice(const std::string& filename,
+                                 float speed, int sampling_frequency_in_hz)
+    : filename_(filename),

[kwiberg-webrtc, 2017/02/28 13:43:47]

Since you always copy the filename, take it by value instead of const reference.

[oprypin_webrtc, 2017/03/06 16:45:00]

Done.

+      sampling_frequency_in_hz_(sampling_frequency_in_hz),
+      num_samples_per_frame_(
+          rtc::CheckedDivExact(sampling_frequency_in_hz_, kFramesPerSecond)),
+      speed_(speed),
+      audio_callback_(nullptr),
+      wav_reader_(nullptr),
+      wav_writer_(nullptr),
+      playout_buffer_(num_samples_per_frame_, 0),
+      tick_(EventTimerWrapper::Create()),
+      thread_(FileAudioDevice::Run, this, "FileAudioDevice"),
+      done_reading_(true, true) {
+  RTC_DCHECK(

[kwiberg-webrtc, 2017/02/28 13:43:46]

Since this is test code, there's no reason not to use CHECK instead of DCHECK.

[oprypin_webrtc, 2017/03/06 16:45:00]

Just did the same as in FakeAudioDevice.
Changed to CHECK now.

+      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);
+}
+
+FileAudioDevice::~FileAudioDevice() {
+  StopPlayout();
+  StopRecording();
+  thread_.Stop();

[kwiberg-webrtc, 2017/02/28 13:43:46]

Why not stop the thread first? (Just asking---I don't know the answer. But
stopping threads before cleaning up stuff they might be using usually makes
sense.)

[oprypin_webrtc, 2017/03/06 16:45:00]

Just did the same as in FakeAudioDevice.
The order does not matter for thread safety, because all of this is done in a
lock. But maybe it's better to let the file be finalized immediately and then
wait for the thread to end?
Leaving as is.

+}
+
+int32_t FileAudioDevice::StartPlayout() {
+  rtc::CritScope cs(&lock_);
+  RTC_CHECK(!wav_reader_);
+  RTC_CHECK(!wav_writer_);
+  wav_writer_.reset(new WavWriter(filename_, sampling_frequency_in_hz_, 1));
+  return 0;
+}
+
+int32_t FileAudioDevice::StopPlayout() {
+  rtc::CritScope cs(&lock_);
+  wav_writer_.reset(nullptr);

[kwiberg-webrtc, 2017/02/28 13:43:47]

It's better to do either

  wav_writer_.reset();

or

  wav_writer_ = nullptr;

since they're shorter. The former is probably better on account of telling the
reader that wav_writer_ is a smart and not a raw pointer.

[oprypin_webrtc, 2017/03/06 16:45:00]

For some reason I thought `reset()` is a newer feature than `reset(ptr)` and the
rest of nullptr, but it isn't.
Done.

+  return 0;
+}
+
+int32_t FileAudioDevice::StartRecording() {
+  rtc::CritScope cs(&lock_);
+  RTC_CHECK(!wav_reader_);
+  RTC_CHECK(!wav_writer_);
+  done_reading_.Reset();
+  wav_reader_.reset(new WavReader(filename_));
+  return 0;
+}
+
+int32_t FileAudioDevice::StopRecording() {
+  rtc::CritScope cs(&lock_);
+  wav_reader_.reset(nullptr);  // This also finalizes and closes the file

[kwiberg-webrtc, 2017/02/28 13:43:46]

Maybe nothing to fix, but you don't have this comment in other places that reset
wav_reader_ and wav_writer_.

[oprypin_webrtc, 2017/03/06 16:45:00]

Hm, this is supposed to be a comment for WavWriter, because WavReader doesn't
actually finalize anything.

I will still keep it in only one place because it's the most important place
where a user of this class might look when they are not sure whether the file is
actually finalized.

+  return 0;
+}
+
+int32_t FileAudioDevice::Init() {
+  RTC_CHECK(tick_->StartTimer(true, kFrameLengthMs / speed_));
+  thread_.Start();
+  thread_.SetPriority(rtc::kHighPriority);

[kwiberg-webrtc, 2017/02/28 13:43:46]

Do these two in the other order? (Again, just the general observation that you
shouldn't have threads running unless you must.)

[oprypin_webrtc, 2017/03/06 16:45:00]

A code search shows that it's always done in this order (start thread, then set
priority). In fact, I think reversing the order causes an error.
Leaving as is.

I think I see your point about running threads. When I separate this into two
classes, it will make even less sense to have this thread permanently running,
so I guess I will start and stop it on demand.

+  return 0;
+}
+
+int32_t FileAudioDevice::RegisterAudioCallback(AudioTransport* callback) {
+  rtc::CritScope cs(&lock_);
+  RTC_DCHECK(callback || audio_callback_ != nullptr);
+  audio_callback_ = callback;
+  return 0;
+}
+
+bool FileAudioDevice::Playing() const {
+  rtc::CritScope cs(&lock_);
+  return static_cast<bool>(wav_writer_);
+}
+
+bool FileAudioDevice::Recording() const {
+  rtc::CritScope cs(&lock_);
+  return static_cast<bool>(wav_reader_);
+}
+
+bool FileAudioDevice::WaitForFileEnd(int milliseconds) {
+  RTC_DCHECK(Recording());
+  return done_reading_.Wait(milliseconds);
+}
+
+bool FileAudioDevice::Run(void* obj) {
+  static_cast<FileAudioDevice*>(obj)->ProcessAudio();
+  return true;
+}

[kwiberg-webrtc, 2017/02/28 13:43:46]

This doesn't need to be a member function. It can be a lambda.

[oprypin_webrtc, 2017/03/06 16:45:00]

I've tried turning it into a lambda, but getting this:

Check failed: spawned_thread_checker_.CalledOnValidThread()

[kwiberg-webrtc, 2017/03/07 10:11:57]

On what line? That makes no sense at all to me. A static member function is just
a function, and a captureless lambda is just a function.

I noted that your lambda returned void instead of bool, though. Would that make
a difference?

+
+void FileAudioDevice::ProcessAudio() {
+  {
+    rtc::CritScope cs(&lock_);
+    if (wav_reader_) {
+      // Capture 10ms of audio. 2 bytes per sample.
+      size_t samples_out = wav_reader_->ReadSamples(num_samples_per_frame_,

[kwiberg-webrtc, 2017/02/28 13:43:47]

const

[oprypin_webrtc, 2017/03/06 16:45:00]

Done.

+                                                    playout_buffer_.data());

[kwiberg-webrtc, 2017/02/28 13:43:47]

CHECK that samples_out <= playout_buffer_.size()?

[oprypin_webrtc, 2017/03/06 16:45:00]

Done.

+      if (samples_out) {
+        uint32_t new_mic_level = 0;

[kwiberg-webrtc, 2017/02/28 13:43:46]

Don't initialize if you don't have to.

[oprypin_webrtc, 2017/03/06 16:45:00]

Done.

+        audio_callback_->RecordedDataIsAvailable(
+            playout_buffer_.data(), samples_out, 2, 1,
+            sampling_frequency_in_hz_, 0, 0, 0, false, new_mic_level);
+      } else {
+        done_reading_.Set();
+      }
+    }
+    if (wav_writer_) {
+      size_t samples_out = 0;
+      int64_t elapsed_time_ms = -1;
+      int64_t ntp_time_ms = -1;

[kwiberg-webrtc, 2017/02/28 13:43:46]

Don't initialize here either. In particular, since you actually use samples_out
below, if you don't initialize it here you give MemorySanitizer the chance to
detect if NeedMorePlayData doesn't fill it in.

[oprypin_webrtc, 2017/03/06 16:45:00]

Thanks, good to know. Again I just took FakeAudioDevice as an example.

+      audio_callback_->NeedMorePlayData(
+          num_samples_per_frame_, 2, 1, sampling_frequency_in_hz_,
+          playout_buffer_.data(), samples_out, &elapsed_time_ms, &ntp_time_ms);
+      wav_writer_->WriteSamples(playout_buffer_.data(), samples_out);
+    }

[kwiberg-webrtc, 2017/02/28 13:43:47]

Since playout_buffer_ is only used locally in this function, consider allocating
a local buffer instead (because the more state your class has, the harder it is
to understand). Just allocating an int16_t[480] on the stack should be both
readable and fast.

[oprypin_webrtc, 2017/03/06 16:45:00]

I don't see how I can use an array of a constant size here, because
`num_samples_per_frame_` depends on some runtime numbers. I also don't want to
hardcode some maximal precalculated number.
And it's probably inefficient to use a vector here.
Keeping as is, waiting for comment.

[kwiberg-webrtc, 2017/03/07 10:11:57]

Just the sample rate, which is capped at 48 kHz. IIUC, 480 entries is as big as
you'll ever need.
Allocating a new std::vector each time would be bad for performance, yes. But
allocating an array of ~1 kB on the stack is *very* fast---it costs one
instruction to allocate and one to deallocate. And if you're already allocating
things on the stack, it's free.

+  }
+  tick_->Wait(WEBRTC_EVENT_INFINITE);
+}
+
+}  // namespace test
+}  // namespace webrtc