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

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 77a74bac8f6d1bc962cf8b8aceea9313f9c50bec..86ef89a8badc0a1e0dd1a4ffa973b9ba3472a881 100644
--- a/webrtc/test/fake_audio_device.h
+++ b/webrtc/test/fake_audio_device.h
@@ -23,17 +23,25 @@ namespace webrtc {
 
 class Clock;
 class EventTimerWrapper;
-class FileWrapper;
-class ModuleFileUtility;
 
 namespace test {
 
 class FakeAudioDevice : public FakeAudioDeviceModule {

[peah-webrtc, 2017/01/24 12:42:21]

Would it make sense to change the name of the class? Before it was a fake device
that read any audio data from a file. 
Now it is a fake device that generates audio data as a sinusoid. Based on that I
think it can be possible to make the class name more descriptive, such as
"SinusoidFakeAudioDevice" (or something much better than my suggestion).

[perkj_webrtc, 2017/01/24 14:38:15]

VoiceEngine seems to use just one device and it is is used for both playout and
capturing. So then I guess it does not really make sence to call this something
that just applies to capturing. 

  public:
-  FakeAudioDevice(Clock* clock, const std::string& filename, float speed);
+  // Creates a new FakeAudioDevice. |speed| controls how much faster or slower
+  // time elapse compared to the system clock. It can be used to simulate
+  // clock drift. 1.0 means that the system clock will be used.
+  FakeAudioDevice(Clock* clock, float speed);

[peah-webrtc, 2017/01/24 12:42:21]

I guess that speed is the multiplying factor applied to the rate, right? 
Would it not be more straightforward to have this in percent with a sign? When I
talk about drift I typically talk in terms of percent faster and slower. If the
drift would be specified in percent, I imagine it would be simpler for the user
to use this code.

Or is the idea that this in practice always should be used with the
DriftingClock class?

WDYT?

[perkj_webrtc, 2017/01/24 14:38:15]

I think that this class and the interface it implements needs some love by
someone working on audio.
Having separate interfaces for capturing and playout would be one idea.
I don't care about drift and would like to not change it in this cl. But I am
pretty sure you are right.

How about I move the recording parts that I have chagnged into a separate inner
class that the FakeAudioAudioDevice can use. If it turns out that more
implementations of a capturers are needed it is straight forward to do so.

I leave the playout parts untouched, including the drifting thingies.



 

+  ~FakeAudioDevice() override;
 
-  virtual ~FakeAudioDevice();
+  int32_t StartPlayout() override;
+  int32_t StopPlayout() override;
 
+  // Generates a sine tone with |frequency_in_hz| and |peak_to_peak|.
+  void StartRecordingSine(int frequency_in_hz, uint16_t peak_to_peak);
+  int32_t StopRecording() override;
+
+ private:
   int32_t Init() override;
   int32_t RegisterAudioCallback(AudioTransport* callback) override;
 
@@ -41,29 +49,31 @@ class FakeAudioDevice : public FakeAudioDeviceModule {
   int32_t PlayoutDelay(uint16_t* delay_ms) const override;
   bool Recording() const override;
 
-  void Start();
-  void Stop();
-
- private:
   static bool Run(void* obj);
-  void CaptureAudio();
+  void ProcessAudio();
 
   static const uint32_t kFrequencyHz = 16000;

[peah-webrtc, 2017/01/24 12:42:21]

This restricts the rate of the audio to be 16 kHz. Is that desired?

If it is, please document that, either by nameing the class accordingly or by
adding comments.

[perkj_webrtc, 2017/01/24 14:38:15]

I don't know. I am just trying to make sure I can use this class for a new test
and make sure it does not need a file.

   static const size_t kBufferSizeBytes = 2 * kFrequencyHz;
 
-  AudioTransport* audio_callback_;
-  bool capturing_;
+  rtc::CriticalSection lock_;
+  AudioTransport* audio_callback_ GUARDED_BY(lock_);
+  bool recording_ GUARDED_BY(lock_);
+  bool playing_ GUARDED_BY(lock_);
+
+  // Used for recording.
+  float normalized_frequency_ GUARDED_BY(lock_);
+  float peak_to_peak_ GUARDED_BY(lock_);
+  float n_;

[peah-webrtc, 2017/01/24 12:42:21]

Please change n_ to something more verbose.

[perkj_webrtc, 2017/01/24 14:38:15]

sine_step_?

   int8_t captured_audio_[kBufferSizeBytes];
+
+  // Used for playout.
   int8_t playout_buffer_[kBufferSizeBytes];
   const float speed_;
   int64_t last_playout_ms_;
 
   DriftingClock clock_;
   std::unique_ptr<EventTimerWrapper> tick_;
-  rtc::CriticalSection lock_;
   rtc::PlatformThread thread_;
-  std::unique_ptr<ModuleFileUtility> file_utility_;
-  std::unique_ptr<FileWrapper> input_stream_;
 };
 }  // namespace test
 }  // namespace webrtc