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

webrtc/test/fake_audio_device.h

Index: webrtc/test/fake_audio_device.h
diff --git a/webrtc/test/fake_audio_device.h b/webrtc/test/fake_audio_device.h
index 4daeab43650066c258d0bb172a12d14f36fe64c3..9e9fcdc7dbea05394be4be847685cdf91ce1706a 100644
--- a/webrtc/test/fake_audio_device.h
+++ b/webrtc/test/fake_audio_device.h
@@ -14,7 +14,10 @@
 #include <string>
 #include <vector>
 
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/buffer.h"
 #include "webrtc/base/criticalsection.h"
+#include "webrtc/base/event.h"
 #include "webrtc/base/platform_thread.h"
 #include "webrtc/modules/audio_device/include/fake_audio_device.h"
 #include "webrtc/typedefs.h"
@@ -28,19 +31,73 @@ namespace test {
 // FakeAudioDevice implements an AudioDevice module that can act both as a
 // capturer and a renderer. It will use 10ms audio frames.
 class FakeAudioDevice : public FakeAudioDeviceModule {
+ private:
+  class Streamer {
+   public:
+    explicit Streamer(int sampling_frequency_in_hz);
+    int SamplingFrequency() const {
+      return sampling_frequency_in_hz_;
+    }
+    size_t SamplesPerFrame() const {
+      return num_samples_per_frame_;
+    }
+
+   private:
+    const int sampling_frequency_in_hz_;
+    const size_t num_samples_per_frame_;
+  };

[kwiberg-webrtc, 2017/03/13 10:18:22]

I still think it's wrong to have this one here---see reply elsewhere.

[oprypin_webrtc, 2017/03/13 13:55:14]

Done.

+
  public:
+  class Capturer : public Streamer {
+   public:
+    using Streamer::Streamer;

[kwiberg-webrtc, 2017/03/13 10:18:22]

This using statement is an implementation detail that's best left out of the the
header. I think that'll happen naturally if you follow my advice about Streamer.

[oprypin_webrtc, 2017/03/13 13:55:14]

Done.

+    virtual ~Capturer() {}
+    // Should capture and return some data (limited to SamplesPerFrame), or
+    // return an empty ArrayView when the capture finishes.

[kwiberg-webrtc, 2017/03/13 10:18:22]

This comment is out of date. Also, instead of "Should capture", just write
"Captures"---it's shorter and means the same thing in a context such as this
one.

[oprypin_webrtc, 2017/03/13 13:55:14]

Done.

+    virtual bool Capture(rtc::BufferT<int16_t>* buffer) = 0;
+  };
+
+  class Renderer : public Streamer {
+   public:
+    using Streamer::Streamer;
+    virtual ~Renderer() {}
+    // Should render the passed audio data and return true if the renderer wants
+    // to keep receiving data, or false otherwise.
+    virtual bool Render(rtc::ArrayView<const int16_t> data) = 0;
+  };
+
   // Creates a new FakeAudioDevice. When capturing or playing, 10 ms audio
   // frames will be processed every 100ms / |speed|.
   // |sampling_frequency_in_hz| can be 8, 16, 32, 44.1 or 48kHz.
   // When recording is started, it will generates a signal where every second
   // frame is zero and every second frame is evenly distributed random noise
   // with max amplitude |max_amplitude|.
-  FakeAudioDevice(float speed,
-                  int sampling_frequency_in_hz,
-                  int16_t max_amplitude);
+  FakeAudioDevice(std::unique_ptr<Capturer> capturer,
+                  std::unique_ptr<Renderer> renderer,
+                  float speed = 1);
   ~FakeAudioDevice() override;
 
- private:
+  // Returns a Capturer instance that generates a signal where every second
+  // frame is zero and every second frame is evenly distributed random noise
+  // with max amplitude |max_amplitude|.
+  static std::unique_ptr<Capturer> CreatePulsedNoiseCapturer(
+      int16_t max_amplitude, int sampling_frequency_in_hz);
+
+  // Returns a Capturer instance that gets its data from a file.
+  static std::unique_ptr<Capturer> CreateWavFileReader(

[kwiberg-webrtc, 2017/03/13 10:18:22]

CreateWavFileCapturer?

+      std::string filename, int sampling_frequency_in_hz);
+
+  // Returns a Capturer instance that gets its data from a file.
+  // Automatically detects sample rate.
+  static std::unique_ptr<Capturer> CreateWavFileReader(std::string filename);
+
+  // Returns a Renderer instance that writes its data to a file.
+  static std::unique_ptr<Renderer> CreateWavFileWriter(

[kwiberg-webrtc, 2017/03/13 10:18:22]

CreateWavFileRenderer?

[oprypin_webrtc, 2017/03/13 13:55:14]

I avoided this name because it would confuse even myself. This class does not
render WAV files.

+      std::string filename, int sampling_frequency_in_hz);
+
+  static std::unique_ptr<Renderer> CreateDiscarder(

[kwiberg-webrtc, 2017/03/13 10:18:22]

CreateDiscardRenderer?

[oprypin_webrtc, 2017/03/13 13:55:14]

Done.

+      int sampling_frequency_in_hz);
+
   int32_t Init() override;
   int32_t RegisterAudioCallback(AudioTransport* callback) override;
 
@@ -52,22 +109,30 @@ class FakeAudioDevice : public FakeAudioDeviceModule {
   bool Playing() const override;
   bool Recording() const override;
 
+  // Blocks until the Renderer refuses to receive data.
+  // Returns false if |timeout_ms| passes before that happens.
+  bool WaitForPlayoutEnd(int timeout_ms = rtc::Event::kForever);
+  // Blocks until the Recorder stops producing data.
+  // Returns false if |timeout_ms| passes before that happens.
+  bool WaitForRecordingEnd(int timeout_ms = rtc::Event::kForever);
+
+ private:
   static bool Run(void* obj);
   void ProcessAudio();
 
-  const int sampling_frequency_in_hz_;
-  const size_t num_samples_per_frame_;
+  const std::unique_ptr<Capturer> capturer_ GUARDED_BY(lock_);
+  const std::unique_ptr<Renderer> renderer_ GUARDED_BY(lock_);
   const float speed_;
 
   rtc::CriticalSection lock_;
   AudioTransport* audio_callback_ GUARDED_BY(lock_);
   bool rendering_ GUARDED_BY(lock_);
   bool capturing_ GUARDED_BY(lock_);
-
-  class PulsedNoiseCapturer;
-  const std::unique_ptr<PulsedNoiseCapturer> capturer_ GUARDED_BY(lock_);
+  rtc::Event done_rendering_;
+  rtc::Event done_capturing_;
 
   std::vector<int16_t> playout_buffer_ GUARDED_BY(lock_);
+  rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_);

[kwiberg-webrtc, 2017/03/13 10:18:22]

As I said before, since the sample rate is at most 48kHz, you can easily
allocate this one on the stack when you need it.

[oprypin_webrtc, 2017/03/13 13:55:14]

Now that it's a Buffer, it manages a heap allocated array regardless of where I
put it. I don't think recreating the buffer every time would be a good idea.

[kwiberg-webrtc, 2017/03/13 14:22:26]

Since allocating an array on the stack is ~free, the amortized allocation cost
is the same for a stack allocated array and a persistent heap allocated array
(which is what this rtc::Buffer ends up being). The advantage with having the
array on the stack is that it doesn't tie up memory between ProcessAudio()
calls, and that stack memory has a higher chance than the persistent heap array
of already being in the cache.

[oprypin_webrtc, 2017/03/13 15:35:20]

I don't see how I can have an array on the stack when using a BufferT. Unless
you're talking about playout_buffer_ (vector)...

[kwiberg-webrtc, 2017/03/14 10:02:03]

Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need an
old-fashioned C array or a std::array.

(BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I guess in
principle you could give it a custom allocator that gets its memory from an
array on the stack, but don't go there...)

[oprypin_webrtc, 2017/03/14 11:58:12]

I still don't understand how I can make an improvement here.

[kwiberg-webrtc, 2017/03/14 13:46:46]

By removing this member, and doing something like this in ProcessAudio():

if (capturing_) {
  // Capture 10ms of audio. 2 bytes per sample.
  int16_t recording_buffer[kMaxRecordingBufferSize];
  size_t recorded_size;
  const bool keep_capturing = capturer_->Capture(recording_buffer,
                                                 &recorded_size);
  uint32_t new_mic_level;
  if (recorded_size > 0) {
    audio_callback_->RecordedDataIsAvailable(
        recording_buffer, recorded_size, 2, 1,
        capturer_->SamplingFrequency(), 0, 0, 0, false, new_mic_level);
  }
  if (!keep_capturing) {
    capturing_ = false;
    done_capturing_.Set();
  }
}

This would require Capture to have a signature such as bool(ArrayView<int16_t>,
size_t*), though, which is different from bool(BufferT<int16_t>*) that I've
pushed for elsewhere in this review. I hadn't realized the discrepancy---sorry!
No wonder you're confused...

The solution you've already coded, the one with Buffer, is easier to read
(because you don't have to manage the memory manually); the solution with a
stack-allocated array probably has better performance because it uses less
memory and is more cache friendly.

Use whichever one you like.

[oprypin_webrtc, 2017/03/14 14:01:40]

I understand now. So you meant that a stack allocated array can be used instead
of a BufferT, but I did not realize that because I was totally set on using the
BufferT.
Keeping BufferT.

[kwiberg-webrtc, 2017/03/14 14:08:56]

Well, no wonder you were set on using BufferT, because elsewhere in the same
review I was busy trying to convince you to do precisely that...

 
   std::unique_ptr<EventTimerWrapper> tick_;
   rtc::PlatformThread thread_;