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

media/audio/fake_audio_input_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 {
 
 // These values are based on experiments for local-to-local
 // PeerConnection to demonstrate audio/video synchronization.
 const int kBeepDurationMilliseconds = 20;
 const int kBeepFrequency = 400;
 
 // Intervals between two automatic beeps.
@@ skipping 67 matching lines @@
   scoped_ptr<media::WavAudioHandler> wav_audio_handler(
       new media::WavAudioHandler(wav_data));
   return wav_audio_handler.Pass();
 }
 
 static base::LazyInstance<BeepContext> g_beep_context =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-AudioInputStream* FakeAudioInputStream::MakeFakeStream(
-    AudioManagerBase* manager,
-    const AudioParameters& params) {
-  return new FakeAudioInputStream(manager, params);
+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 {}
+
+  void Open(const base::FilePath& path_to_wav_file);
+  void PlayInto(AudioBus* audio_bus, int* buffer_size) override;
+
+ 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;
+};
+
+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) {
 }
 
-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();
-
-  if (PlayingFromFile()) {
-    PlayFile();
-  } else {
-    PlayBeep();
-  }
-
-  last_callback_time_ = now;
-
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&FakeAudioInputStream::DoCallback, weak_factory_.GetWeakPtr()),
-      next_callback_time);
-}
-
-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);
   wav_audio_handler_ = CreateWavAudioHandler(
       wav_filename, wav_file_data_.get(), file_length, params_);
 
   // Hook us up so we pull in data from the file into the converter. We need to
   // modify the wav file's audio parameters since we'll be reading small slices
   // of it at a time and not the whole thing (like 10 ms at a time).
   AudioParameters file_audio_slice(
       wav_audio_handler_->params().format(),
       wav_audio_handler_->params().channel_layout(),
       wav_audio_handler_->params().sample_rate(),
       wav_audio_handler_->params().bits_per_sample(),
       params_.frames_per_buffer());
 
   file_audio_converter_.reset(
       new AudioConverter(file_audio_slice, params_, false));
   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;
   {
     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;
       }
     } else {
       should_beep = beep_context->beep_once();
       beep_context->SetBeepOnce(false);
     }
   }
 
@@ skipping 14 matching lines @@
       memset(buffer_.get() + position, 128, high_bytes);
       // Then leave low values in the buffer with |high_bytes|.
       position += high_bytes * 2;
     }
 
     ++beep_generated_in_buffers_;
     if (beep_generated_in_buffers_ >= beep_duration_in_buffers_)
       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);
 }
 
 double FakeAudioInputStream::GetMaxVolume() {
   DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
   return 1.0;
 }
 
 void FakeAudioInputStream::SetVolume(double volume) {
   DCHECK(audio_manager_->GetTaskRunner()->BelongsToCurrentThread());
@@ skipping 10 matching lines @@
 }
 
 bool FakeAudioInputStream::SetAutomaticGainControl(bool enabled) {
   return false;
 }
 
 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