Moved the fake input stream's processing onto the audio worker thread.

media/audio/fake_audio_input_stream.cc

Index: media/audio/fake_audio_input_stream.cc
diff --git a/media/audio/fake_audio_input_stream.cc b/media/audio/fake_audio_input_stream.cc
index 4632b258eda2abaf261bb7dba214ab6aba15d8bb..679052d31c17f515eb22629732bab65251be82e8 100644
--- a/media/audio/fake_audio_input_stream.cc
+++ b/media/audio/fake_audio_input_stream.cc
@@ -5,16 +5,20 @@
 #include "media/audio/fake_audio_input_stream.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file.h"
+#include "base/files/file_path.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
+#include "base/time/time.h"
 #include "media/audio/audio_manager_base.h"
+#include "media/audio/sounds/wav_audio_handler.h"
 #include "media/base/audio_bus.h"
+#include "media/base/audio_converter.h"
 #include "media/base/media_switches.h"
 
-using base::TimeTicks;
-using base::TimeDelta;
-
 namespace media {
 
 namespace {
@@ -102,95 +106,37 @@ static base::LazyInstance<BeepContext> g_beep_context =
 
 }  // namespace
 
-AudioInputStream* FakeAudioInputStream::MakeFakeStream(
-    AudioManagerBase* manager,
-    const AudioParameters& params) {
-  return new FakeAudioInputStream(manager, params);
-}
-
-FakeAudioInputStream::FakeAudioInputStream(AudioManagerBase* manager,
-                                           const AudioParameters& params)
-    : audio_manager_(manager),
-      callback_(NULL),
-      buffer_size_((params.channels() * params.bits_per_sample() *
-                    params.frames_per_buffer()) /
-                   8),
-      params_(params),
-      task_runner_(manager->GetTaskRunner()),
-      callback_interval_(base::TimeDelta::FromMilliseconds(
-          (params.frames_per_buffer() * 1000) / params.sample_rate())),
-      beep_duration_in_buffers_(kBeepDurationMilliseconds *
-                                params.sample_rate() /
-                                params.frames_per_buffer() /
-                                1000),
-      beep_generated_in_buffers_(0),
-      beep_period_in_frames_(params.sample_rate() / kBeepFrequency),
-      audio_bus_(AudioBus::Create(params)),
-      wav_file_read_pos_(0),
-      weak_factory_(this) {
-  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
-}
-
-FakeAudioInputStream::~FakeAudioInputStream() {}
-
-bool FakeAudioInputStream::Open() {
-  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
-  buffer_.reset(new uint8[buffer_size_]);
-  memset(buffer_.get(), 0, buffer_size_);
-  audio_bus_->Zero();
-
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kUseFileForFakeAudioCapture)) {
-    OpenInFileMode(base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
-        switches::kUseFileForFakeAudioCapture));
-  }
-
-  return true;
-}
-
-void FakeAudioInputStream::Start(AudioInputCallback* callback)  {
-  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
-  DCHECK(!callback_);
-  callback_ = callback;
-  last_callback_time_ = TimeTicks::Now();
-
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&FakeAudioInputStream::DoCallback, weak_factory_.GetWeakPtr()),
-      callback_interval_);
-}
-
-void FakeAudioInputStream::DoCallback() {
-  DCHECK(callback_);
-
-  const TimeTicks now = TimeTicks::Now();
-  base::TimeDelta next_callback_time =
-      last_callback_time_ + callback_interval_ * 2 - now;
-
-  // If we are falling behind, try to catch up as much as we can in the next
-  // callback.
-  if (next_callback_time < base::TimeDelta())
-    next_callback_time = base::TimeDelta();
+class FakeAudioInputStream::FileSource : public AudioConverter::InputCallback,

[phoglund_chromium, 2015/02/18 16:38:15]

It felt wrong to toss back all the state into the input stream - most of this
state should be accessed only on the worker thread, so I took the opportunity to
abstract out the two kinds of sources. That way it's easier to maintain the
input stream in the future and not mix up the general state, like the audio bus,
and the worker-thread-only state like the wav file handler.

[DaleCurtis, 2015/02/18 19:04:49]

Good idea. Take a look at media/audio/simple_sources.h I think maybe this code
(and BeepSource) should be added there and just use the
AudioOutputStream::AudioSourceCallback() interface for simplicity. This code
could then be used to drive an output stream too.

[phoglund_chromium, 2015/02/19 15:44:10]

Sure, done.

+                                         public FakeAudioInputStream::Source {
+ public:
+  FileSource(const AudioParameters& params);
+  ~FileSource() override {}
 
-  if (PlayingFromFile()) {
-    PlayFile();
-  } else {
-    PlayBeep();
-  }
+  void Open(const base::FilePath& path_to_wav_file);
+  void PlayInto(AudioBus* audio_bus, int* buffer_size) override;
 
-  last_callback_time_ = now;
+ private:
+  AudioParameters params_;
+  scoped_ptr<uint8[]> wav_file_data_;
+  scoped_ptr<media::WavAudioHandler> wav_audio_handler_;
+  scoped_ptr<media::AudioConverter> file_audio_converter_;
+  int wav_file_read_pos_;
+  int buffer_size_;
+
+  // Provides audio data from wav_audio_handler_ into the file audio converter.
+  double ProvideInput(AudioBus* audio_bus,
+                      base::TimeDelta buffer_delay) override;
+};
 
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&FakeAudioInputStream::DoCallback, weak_factory_.GetWeakPtr()),
-      next_callback_time);
+FakeAudioInputStream::FileSource::FileSource(const AudioParameters& params)
+    : params_(params),
+      wav_file_read_pos_(0),
+      buffer_size_(params.channels() * params.bits_per_sample() *
+                   params.frames_per_buffer() / 8) {
 }
 
-void FakeAudioInputStream::OpenInFileMode(const base::FilePath& wav_filename) {
-  CHECK(!wav_filename.empty())
-      << "You must pass the file to use as argument to --"
-      << switches::kUseFileForFakeAudioCapture << ".";
-
+void FakeAudioInputStream::FileSource::Open(
+    const base::FilePath& wav_filename) {
   // Read the file, and put its data in a scoped_ptr so it gets deleted later.
   size_t file_length = 0;
   wav_file_data_ = ReadWavFile(wav_filename, &file_length);
@@ -212,22 +158,68 @@ void FakeAudioInputStream::OpenInFileMode(const base::FilePath& wav_filename) {
   file_audio_converter_->AddInput(this);
 }
 
-bool FakeAudioInputStream::PlayingFromFile() {
-  return wav_audio_handler_.get() != nullptr;
-}
+void FakeAudioInputStream::FileSource::PlayInto(AudioBus* audio_bus,
+                                                int* buffer_size) {
+  DCHECK(wav_audio_handler_.get() != nullptr);
 
-void FakeAudioInputStream::PlayFile() {
   // Stop playing if we've played out the whole file.
   if (wav_audio_handler_->AtEnd(wav_file_read_pos_))
     return;
 
-  file_audio_converter_->Convert(audio_bus_.get());
-  callback_->OnData(this, audio_bus_.get(), buffer_size_, 1.0);
+  // This pulls data from ProvideInput.
+  file_audio_converter_->Convert(audio_bus);
+  *buffer_size = buffer_size_;
 }
 
-void FakeAudioInputStream::PlayBeep() {
+double FakeAudioInputStream::FileSource::ProvideInput(
+    AudioBus* audio_bus_into_converter,
+    base::TimeDelta buffer_delay) {
+  // Unfilled frames will be zeroed by CopyTo.
+  size_t bytes_written;
+  wav_audio_handler_->CopyTo(audio_bus_into_converter, wav_file_read_pos_,
+                             &bytes_written);
+  wav_file_read_pos_ += bytes_written;
+  return 1.0;
+};
+
+class FakeAudioInputStream::BeepingSource
+    : public FakeAudioInputStream::Source {
+ public:
+  BeepingSource(const AudioParameters& params);
+  ~BeepingSource() override {}
+
+  void PlayInto(AudioBus* audio_bus, int* buffer_size) override;
+ private:
+  scoped_ptr<uint8[]> buffer_;
+  int buffer_size_;
+  AudioParameters params_;
+  base::TimeTicks last_callback_time_;
+  base::TimeDelta interval_from_last_beep_;
+  int beep_duration_in_buffers_;
+  int beep_generated_in_buffers_;
+  int beep_period_in_frames_;
+};
+
+FakeAudioInputStream::BeepingSource::BeepingSource(
+    const AudioParameters& params)
+    : buffer_size_(params.channels() * params.bits_per_sample() *
+                   params.frames_per_buffer() / 8),
+      params_(params),
+      beep_duration_in_buffers_(kBeepDurationMilliseconds *
+                                params.sample_rate() /
+                                params.frames_per_buffer() /
+                                1000),
+      beep_generated_in_buffers_(0),
+      beep_period_in_frames_(params.sample_rate() / kBeepFrequency) {
+  buffer_.reset(new uint8[buffer_size_]);
+  memset(buffer_.get(), 0, buffer_size_);
+}
+
+void FakeAudioInputStream::BeepingSource::PlayInto(AudioBus* audio_bus,
+                                                   int* buffer_size) {
   // Accumulate the time from the last beep.
-  interval_from_last_beep_ += TimeTicks::Now() - last_callback_time_;
+  interval_from_last_beep_ += base::TimeTicks::Now() - last_callback_time_;
+  last_callback_time_ = base::TimeTicks::Now();
 
   memset(buffer_.get(), 0, buffer_size_);
   bool should_beep = false;
@@ -235,7 +227,7 @@ void FakeAudioInputStream::PlayBeep() {
     BeepContext* beep_context = g_beep_context.Pointer();
     if (beep_context->automatic_beep()) {
       base::TimeDelta delta = interval_from_last_beep_ -
-          TimeDelta::FromMilliseconds(kAutomaticBeepIntervalInMs);
+          base::TimeDelta::FromMilliseconds(kAutomaticBeepIntervalInMs);
       if (delta > base::TimeDelta()) {
         should_beep = true;
         interval_from_last_beep_ = delta;
@@ -270,19 +262,73 @@ void FakeAudioInputStream::PlayBeep() {
       beep_generated_in_buffers_ = 0;
   }
 
-  audio_bus_->FromInterleaved(
-      buffer_.get(), audio_bus_->frames(), params_.bits_per_sample() / 8);
-  callback_->OnData(this, audio_bus_.get(), buffer_size_, 1.0);
+  *buffer_size = buffer_size_;

[phoglund_chromium, 2015/02/18 16:38:15]

Not sure about this: I do feel the sources know better than the input stream
what the buffer size is, but maybe the stream should compute the buffer size and
hand to the sources instead.

[DaleCurtis, 2015/02/18 19:04:49]

I don't think you need this, it seems like an incorrect remnant from when we
used a uint8[] instead of an AudioBus to transfer the data. I think it should be
removed.

[phoglund_chromium, 2015/02/19 15:44:11]

Just passing 0 seems to work so I'll go with that.

+  audio_bus->FromInterleaved(
+      buffer_.get(), audio_bus->frames(), params_.bits_per_sample() / 8);
+}
+
+AudioInputStream* FakeAudioInputStream::MakeFakeStream(
+    AudioManagerBase* manager,
+    const AudioParameters& params) {
+  return new FakeAudioInputStream(manager, params);
+}
+
+FakeAudioInputStream::FakeAudioInputStream(AudioManagerBase* manager,
+                                           const AudioParameters& params)
+    : audio_manager_(manager),
+      callback_(NULL),
+      fake_audio_worker_(manager->GetWorkerTaskRunner(), params),
+      params_(params),
+      audio_bus_(AudioBus::Create(params)) {
+  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
+}
+
+FakeAudioInputStream::~FakeAudioInputStream() {
+  DCHECK(!callback_);
+}
+
+bool FakeAudioInputStream::Open() {
+  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
+  audio_bus_->Zero();
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kUseFileForFakeAudioCapture)) {
+    base::FilePath path_to_wav_file =
+        base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+            switches::kUseFileForFakeAudioCapture);
+    CHECK(!path_to_wav_file.empty())
+        << "You must pass the file to use as argument to --"
+        << switches::kUseFileForFakeAudioCapture << ".";
+
+    FileSource* file_source = new FileSource(params_);
+    audio_source_.reset(file_source);
+    audio_manager_->GetWorkerTaskRunner()->PostTask(

[phoglund_chromium, 2015/02/18 16:38:15]

This is the file loading I talked about earlier. An alternative could be to hand
the worker a callback and make it schedule the run (like
worker->Open(callback_that_does_the_actual_opening) or something) but I didn't
see the benefit. Another alternative would be to make the file source load the
file on its first callback, but that seemed uglier. 

The above assumes that the Open is guaranteed to execute before the first
callback.

[DaleCurtis, 2015/02/18 19:04:49]

I think loading the file source on the first callback is fine and no worse than
the code here. I'd simply choose the source on first callback via something like
if (!audio_source_) audio_source_ = ChooseSource() or similar.

[phoglund_chromium, 2015/02/19 15:44:11]

Done.

+        FROM_HERE,
+        base::Bind(&FileSource::Open,
+                   base::Unretained(file_source),
+                   path_to_wav_file));
+  } else {
+    audio_source_.reset(new BeepingSource(params_));
+  }
+  return true;
+}
+
+void FakeAudioInputStream::Start(AudioInputCallback* callback)  {
+  DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
+  callback_ = callback;
+  fake_audio_worker_.Start(base::Bind(
+      &FakeAudioInputStream::ReadAudioFromSource, base::Unretained(this)));
 }
 
 void FakeAudioInputStream::Stop() {
   DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
-  weak_factory_.InvalidateWeakPtrs();
+  fake_audio_worker_.Stop();
   callback_ = NULL;
 }
 
 void FakeAudioInputStream::Close() {
   DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
+  DCHECK(!callback_);
   audio_manager_->ReleaseInputStream(this);
 }
 
@@ -313,20 +359,17 @@ bool FakeAudioInputStream::GetAutomaticGainControl() {
   return false;
 }
 
-// static
+void FakeAudioInputStream::ReadAudioFromSource() {
+  DCHECK(audio_manager_->GetWorkerTaskRunner()->BelongsToCurrentThread());
+  DCHECK(callback_);
+  int buffer_size;
+  audio_source_->PlayInto(audio_bus_.get(), &buffer_size);
+  callback_->OnData(this, audio_bus_.get(), buffer_size, 1.0);

[DaleCurtis, 2015/02/18 19:04:49]

Why are you returning the buffer_size like this? I don't see why you'd want to
use it as the hardware_delay. I think you can just use 0 here and drop the
buffer_size parameter on the source interface. 

[phoglund_chromium, 2015/02/19 15:44:11]

Done.

+}
+
 void FakeAudioInputStream::BeepOnce() {
   BeepContext* beep_context = g_beep_context.Pointer();
   beep_context->SetBeepOnce(true);
 }
 
-double FakeAudioInputStream::ProvideInput(AudioBus* audio_bus_into_converter,
-                                          base::TimeDelta buffer_delay) {
-  // Unfilled frames will be zeroed by CopyTo.
-  size_t bytes_written;
-  wav_audio_handler_->CopyTo(audio_bus_into_converter, wav_file_read_pos_,
-                             &bytes_written);
-  wav_file_read_pos_ += bytes_written;
-  return 1.0;
-};
-
 }  // namespace media