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..def670f06bc0b5b192da33f7b244f1c7e8e7d2bd 100644
--- a/webrtc/test/fake_audio_device.h
+++ b/webrtc/test/fake_audio_device.h
@@ -14,7 +14,9 @@
 #include <string>
 #include <vector>
 
+#include "webrtc/base/array_view.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 +30,82 @@ 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_;
+  };
+
+  class PulsedNoiseCapturer;
+  class WavFileReader;
+
+  class Discarder;
+  class WavFileWriter;

[kwiberg-webrtc, 2017/03/09 10:04:11]

Why do you need to mention these four in the .h file?

[oprypin_webrtc, 2017/03/10 10:44:27]

Done. Together with the change to anonymous namespaces, these mentions are now
obsolete anyway.

+
  public:
+  class Capturer : public Streamer {
+   public:
+    using Streamer::Streamer;
+    // Should capture and return some data (limited to SamplesPerFrame), or
+    // return an empty ArrayView when the capture finishes.
+    virtual rtc::ArrayView<const int16_t> Capture() = 0;

[kwiberg-webrtc, 2017/03/09 10:04:11]

It's not usually a good idea to return an ArrayView, because then the caller has
to know somehow for how long the pointed-to data is going to remain valid.

There are several alternatives. I would suggest passing an rtc::Buffer*, and
letting Capture() replace its contents.

[oprypin_webrtc, 2017/03/10 10:44:27]

Done.

+    virtual ~Capturer() {}

[kwiberg-webrtc, 2017/03/09 10:04:11]

Put the destructor before the regular methods (just after the constructors, but
you don't have any).

[oprypin_webrtc, 2017/03/10 10:44:27]

Done.

+  };
+
+  class Renderer : public Streamer {
+   public:
+    using Streamer::Streamer;
+    // 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;
+    virtual ~Renderer() {}
+  };

[kwiberg-webrtc, 2017/03/09 10:04:10]

Unless you have a very good reason, please make Capturer and Renderer pure
interfaces. Non-pure interfaces are almost always more trouble than they are
worth.

I think a good arrangement here would be to turn Streamer into a pure interface
too, and have Capturer and Renderer inherit from Streamer. Then in the .cc file,
have a StreamerImpl class that has the shared methods and state that everyone
needs.

[oprypin_webrtc, 2017/03/10 10:44:27]

I just want to get rid of this num_samples_per_frame_ computation (facilitating
the storage for sampling frequency is also nice) and this is the best way I
found to have it in only one place. I agree that it's messy but I can't come up
with any better way to do it.

I don't understand the comment about StreamerImpl, but seems like this
introduces some complexity and some code duplication in order to eliminate code
duplication.

I'd like to discuss this more, kept as is for now.

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

I have two alternative suggestions. The first one is to not have Streamer in the
.h file at all, but just define purely abstract classes Capturer and Renderer;
all the shared code would then be in the implementations in the .cc file.

The second alternative is to have a purely abstract Streamer in the header, plus
purely abstract classes Capturer and Renderer that inherit from it. Then in the
.cc file, make an implementation of Streamer, and have the implementations of
Capturer and Renderer inherit from it.

The second alternative minimizes code duplication, but the first minimizes
complexity in the header. My opinion is that minimizing complexity in the header
is more important, but feel free to argue if you feel otherwise.

+
   // 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(std::unique_ptr<Capturer> capturer,
+                  std::unique_ptr<Renderer> renderer,
+                  float speed = 1);

[kwiberg-webrtc, 2017/03/09 10:04:10]

The default speed is 0.1x realtime?

[oprypin_webrtc, 2017/03/10 10:44:27]

I'm quite sure this is just realtime, because call_test.cc specifies 1 when
creating this object. My CL doesn't touch the logic of what the speed means, and
this line...
    StartTimer(true, kFrameLengthMs / speed_)
... (`kFrameLengthMs` being 10) seems to suggest that "frames will be processed
every 10ms / |speed|" which would make more sense then.

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

OK, then either the existing comment is wrong, or I'm very confused---a speed of
1 ought to mean we process one 10 ms frame every 10 ms, no? If this is just the
speed relative to realtime, say something like "|speed| is the number of 10 ms
frames we process every 10 ms; a value >1 is faster than real time, and <1 is
slower."

   FakeAudioDevice(float speed,
                   int sampling_frequency_in_hz,
                   int16_t max_amplitude);
   ~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(
+      int sampling_frequency_in_hz, int16_t max_amplitude);

[kwiberg-webrtc, 2017/03/09 10:04:11]

The sample rate is the last argument in all the other Create functions.

[oprypin_webrtc, 2017/03/10 10:44:27]

Done. Made it the last here as well.

+
+  // Returns a Capturer instance that gets its data from a file.
+  static std::unique_ptr<Capturer> CreateWavFileReader(
+      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(
+      std::string filename, int sampling_frequency_in_hz);
+
+  static std::unique_ptr<Renderer> CreateDiscarder(
+      int sampling_frequency_in_hz);
+
   int32_t Init() override;
   int32_t RegisterAudioCallback(AudioTransport* callback) override;
 
@@ -52,20 +117,25 @@ class FakeAudioDevice : public FakeAudioDeviceModule {
   bool Playing() const override;
   bool Recording() const override;
 
+  // Block until the Renderer refuses to receive data.
+  bool WaitForPlayoutEnd(int timeout_ms = rtc::Event::kForever);
+  // Block until the Recorder stops producing data.
+  bool WaitForRecordingEnd(int timeout_ms = rtc::Event::kForever);

[kwiberg-webrtc, 2017/03/09 10:04:10]

What do these two return?

[oprypin_webrtc, 2017/03/10 10:44:27]

Done. Added comment.

+
+ 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_);