Improve and simplify AudioOutputDispatcher.

media/audio/audio_output_dispatcher_impl.cc

Index: media/audio/audio_output_dispatcher_impl.cc
diff --git a/media/audio/audio_output_dispatcher_impl.cc b/media/audio/audio_output_dispatcher_impl.cc
index 84d5ac02c94dea74dcde3ede14e93326d6434e35..d5e77e9fba18d8e0640ef7fefc213674288dc3fd 100644
--- a/media/audio/audio_output_dispatcher_impl.cc
+++ b/media/audio/audio_output_dispatcher_impl.cc
@@ -15,39 +15,40 @@
 
 namespace media {
 
+// Helper function for use with std::for_each().
+static void CloseStreamHelper(AudioOutputStream* stream) {
+  stream->Close();
+}
+
 AudioOutputDispatcherImpl::AudioOutputDispatcherImpl(
     AudioManager* audio_manager,
     const AudioParameters& params,
     const std::string& output_device_id,
     const std::string& input_device_id,
     const base::TimeDelta& close_delay)
-    : AudioOutputDispatcher(audio_manager, params, output_device_id,
-          input_device_id),
-      pause_delay_(base::TimeDelta::FromMicroseconds(
-          2 * params.frames_per_buffer() * base::Time::kMicrosecondsPerSecond /
-          static_cast<float>(params.sample_rate()))),
-      paused_proxies_(0),
-      weak_this_(this),
+    : AudioOutputDispatcher(audio_manager,
+                            params,
+                            output_device_id,
+                            input_device_id),
+      idle_proxies_(0),
       close_timer_(FROM_HERE,
                    close_delay,
                    this,
-                   &AudioOutputDispatcherImpl::ClosePendingStreams) {
-}
+                   &AudioOutputDispatcherImpl::CloseIdleStreams) {}
 
 AudioOutputDispatcherImpl::~AudioOutputDispatcherImpl() {
   DCHECK(proxy_to_physical_map_.empty());
   DCHECK(idle_streams_.empty());
-  DCHECK(pausing_streams_.empty());
 }
 
 bool AudioOutputDispatcherImpl::OpenStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  paused_proxies_++;
+  idle_proxies_++;
 
   // Ensure that there is at least one open stream.
   if (idle_streams_.empty() && !CreateAndOpenStream()) {
-    paused_proxies_--;
+    idle_proxies_--;
     return false;
   }
 
@@ -64,23 +65,18 @@ bool AudioOutputDispatcherImpl::StartStream(
     return false;
 
   AudioOutputStream* physical_stream = idle_streams_.back();
-  DCHECK(physical_stream);
   idle_streams_.pop_back();
 
-  DCHECK_GT(paused_proxies_, 0u);
-  --paused_proxies_;
-
-  close_timer_.Reset();
-
-  // Schedule task to allocate streams for other proxies if we need to.
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &AudioOutputDispatcherImpl::OpenTask, weak_this_.GetWeakPtr()));
+  DCHECK_GT(idle_proxies_, 0u);
+  --idle_proxies_;
 
   double volume = 0;
   stream_proxy->GetVolume(&volume);
   physical_stream->SetVolume(volume);
   physical_stream->Start(callback);
   proxy_to_physical_map_[stream_proxy] = physical_stream;
+
+  close_timer_.Reset();
   return true;
 }
 
@@ -93,17 +89,10 @@ void AudioOutputDispatcherImpl::StopStream(AudioOutputProxy* stream_proxy) {
   proxy_to_physical_map_.erase(it);
 
   physical_stream->Stop();
+  ++idle_proxies_;
+  idle_streams_.push_back(physical_stream);
 
-  ++paused_proxies_;
-
-  pausing_streams_.push_front(physical_stream);
-
-  // Don't recycle stream until two buffers worth of time has elapsed.
-  message_loop_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&AudioOutputDispatcherImpl::StopStreamTask,
-                 weak_this_.GetWeakPtr()),
-      pause_delay_);
+  close_timer_.Reset();
 }
 
 void AudioOutputDispatcherImpl::StreamVolumeSet(AudioOutputProxy* stream_proxy,
@@ -116,54 +105,28 @@ void AudioOutputDispatcherImpl::StreamVolumeSet(AudioOutputProxy* stream_proxy,
   }
 }
 
-void AudioOutputDispatcherImpl::StopStreamTask() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-
-  if (pausing_streams_.empty())
-    return;
-
-  AudioOutputStream* stream = pausing_streams_.back();
-  pausing_streams_.pop_back();
-  idle_streams_.push_back(stream);
-  close_timer_.Reset();
-}
-
 void AudioOutputDispatcherImpl::CloseStream(AudioOutputProxy* stream_proxy) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  while (!pausing_streams_.empty()) {
-    idle_streams_.push_back(pausing_streams_.back());
-    pausing_streams_.pop_back();
-  }
-
-  DCHECK_GT(paused_proxies_, 0u);
-  paused_proxies_--;
+  DCHECK_GT(idle_proxies_, 0u);
+  idle_proxies_--;
 
-  while (idle_streams_.size() > paused_proxies_) {
-    idle_streams_.back()->Close();
-    idle_streams_.pop_back();
+  // Leave a single stream running until the close timer fires to help cycle
+  // time when streams are opened and closed repeatedly.
+  if (idle_streams_.size() > 1) {
+    std::for_each(

[scherkus (not reviewing), 2013/10/17 19:19:53]

do you have a strong desire to use std::for_each()? It's not very prevalent in
Chromium. We typically stick w/ plain old C++ for loops.

[DaleCurtis, 2013/10/17 21:11:55]

I found this reasoning behind avoiding "raw loops" pretty sound:
http://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning

I find it easier to parse as well as being less lines of code.

[scherkus (not reviewing), 2013/10/17 23:01:47]

had a sneaking suspicion -- I watched it too :)
do you plan on converting for loops to for_each() in all your future CLs? i.e.,
is this something we'll be seeing more of?

[DaleCurtis, 2013/10/17 23:09:10]

Where it makes sense yes.  I'm on the fence about the utility of doing a
wholesale pass through media/.  It's probably useful and I can file a bug for it
if you agree.

+        idle_streams_.begin(), --idle_streams_.end(), CloseStreamHelper);
+    idle_streams_.erase(idle_streams_.begin(), --idle_streams_.end());
   }
+  close_timer_.Reset();
 }
 
 void AudioOutputDispatcherImpl::Shutdown() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  // Cancel any pending tasks to close paused streams or create new ones.
-  weak_this_.InvalidateWeakPtrs();
-
-  // No AudioOutputProxy objects should hold a reference to us when we get
-  // to this stage.
-  DCHECK(HasOneRef()) << "Only the AudioManager should hold a reference";
-
-  AudioOutputStreamList::iterator it = idle_streams_.begin();
-  for (; it != idle_streams_.end(); ++it)
-    (*it)->Close();
-  idle_streams_.clear();
-
-  it = pausing_streams_.begin();
-  for (; it != pausing_streams_.end(); ++it)
-    (*it)->Close();
-  pausing_streams_.clear();
+  // Close all idle streams immediately.  The |close_timer_| will handle
+  // invalidating any outstanding tasks upon its destruction.
+  CloseIdleStreams();
 }
 
 bool AudioOutputDispatcherImpl::CreateAndOpenStream() {
@@ -177,29 +140,15 @@ bool AudioOutputDispatcherImpl::CreateAndOpenStream() {
     stream->Close();
     return false;
   }
+
   idle_streams_.push_back(stream);
   return true;
 }
 
-void AudioOutputDispatcherImpl::OpenTask() {
+void AudioOutputDispatcherImpl::CloseIdleStreams() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  // Make sure that we have at least one stream allocated if there
-  // are paused streams.
-  if (paused_proxies_ > 0 && idle_streams_.empty() &&
-      pausing_streams_.empty()) {
-    CreateAndOpenStream();
-  }
-
-  close_timer_.Reset();
-}
-
-// This method is called by |close_timer_|.
-void AudioOutputDispatcherImpl::ClosePendingStreams() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  while (!idle_streams_.empty()) {
-    idle_streams_.back()->Close();
-    idle_streams_.pop_back();
-  }
+  std::for_each(idle_streams_.begin(), idle_streams_.end(), CloseStreamHelper);
+  idle_streams_.clear();
 }
 
 }  // namespace media