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 92c7da4a9594738d6c4a99151d11b9efd698ba6f..585012bb384af9b8020c8ae75924d1a701afb443 100644
--- a/media/audio/audio_output_dispatcher_impl.cc
+++ b/media/audio/audio_output_dispatcher_impl.cc
@@ -21,36 +21,30 @@ AudioOutputDispatcherImpl::AudioOutputDispatcherImpl(
     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::CloseAllIdleStreams) {}
 
 AudioOutputDispatcherImpl::~AudioOutputDispatcherImpl() {
+  DCHECK_EQ(idle_proxies_, 0u);
   DCHECK(proxy_to_physical_map_.empty());
   DCHECK(idle_streams_.empty());
-  DCHECK(pausing_streams_.empty());
 }
 
 bool AudioOutputDispatcherImpl::OpenStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  paused_proxies_++;
-
   // Ensure that there is at least one open stream.
-  if (idle_streams_.empty() && !CreateAndOpenStream()) {
-    paused_proxies_--;
+  if (idle_streams_.empty() && !CreateAndOpenStream())
     return false;
-  }
 
+  ++idle_proxies_;
   close_timer_.Reset();
   return true;
 }
@@ -66,19 +60,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();
+  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;
 }
 
@@ -91,17 +84,10 @@ void AudioOutputDispatcherImpl::StopStream(AudioOutputProxy* stream_proxy) {
   proxy_to_physical_map_.erase(it);
 
   physical_stream->Stop();
+  ++idle_proxies_;
+  idle_streams_.push_back(physical_stream);

[scherkus (not reviewing), 2013/12/06 23:04:14]

what's the difference between idle_streams_ and idle_proxies_?

looking at the code ... it seems any time idle_streams_.push_back() is called we
increment idle_proxies_ and also looks like we try to decrement idle_proxies_
any time we close + idle_streams_.pop_back()

[DaleCurtis, 2013/12/07 00:33:33]

idle_streams_ may be empty when idle_proxies_ > 0.  Consider when the close
timer fires and kills all idle streams, it doesn't change the number of
outstanding idle_proxies_.

[scherkus (not reviewing), 2013/12/09 18:53:12]

I suppose I don't understand what an idle_proxy_ is supposed to represent or
what it's tracking, which makes it hard to figure out what's going on.

Is it supposed to refer to AudioOutputProxy or some other proxy-like object?

For example we increment idle_proxies_ in OpenStream() but there's no proxy-like
object in sight. Then in CloseStream() we pass in a proxy object but don't do
anything with it, but we decrement idle_proxies_ by one, then close a bunch of
idle_streams_.

Can you provide some more details?

[DaleCurtis, 2013/12/09 19:16:36]

An idle proxy is an AudioOutputProxy object in the kOpened state (see AOP
header); which correlates to immediately after AudioOutput(Proxy|Stream)::Open()
or AudioOutput(Proxy|Stream)::Stop().

OpenStream() and CloseStream() are really only used as hints as to when the
|idle_streams_| pool should be increased or decreased.

Yes, it's bad that OpenStream() and CloseStream() are not symmetric in terms of
parameters.  I want to fix that; just not in this CL.  The parameter should be
removed from CloseStream(), but AudioOutputResampler is currently using it to
share OnMoreDataConverter() allocations, so it's a bit more than a simple
deletion.

 
-  ++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,
@@ -114,54 +100,24 @@ 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 at least a single stream running until the close timer fires to help
+  // cycle time when streams are opened and closed repeatedly.
+  CloseIdleStreams(idle_proxies_ + 1);
+  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.
+  CloseAllIdleStreams();
 }
 
 bool AudioOutputDispatcherImpl::CreateAndOpenStream() {
@@ -175,26 +131,28 @@ bool AudioOutputDispatcherImpl::CreateAndOpenStream() {
     stream->Close();
     return false;
   }
+
   idle_streams_.push_back(stream);
   return true;
 }
 
-// This method is called by |close_timer_|.
-void AudioOutputDispatcherImpl::ClosePendingStreams() {
+void AudioOutputDispatcherImpl::CloseAllIdleStreams() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  while (!idle_streams_.empty()) {
-    idle_streams_.back()->Close();
-    idle_streams_.pop_back();
-  }
+  CloseIdleStreams(0);
+}
+
+void AudioOutputDispatcherImpl::CloseIdleStreams(int keep_alive) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  if (idle_streams_.size() <= static_cast<size_t>(keep_alive))
+    return;
+  for (size_t i = keep_alive; i < idle_streams_.size(); ++i)
+    idle_streams_[i]->Close();
+  idle_streams_.erase(idle_streams_.begin() + keep_alive, idle_streams_.end());
 }
 
 void AudioOutputDispatcherImpl::CloseStreamsForWedgeFix() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  while (!pausing_streams_.empty()) {
-    idle_streams_.push_back(pausing_streams_.back());
-    pausing_streams_.pop_back();
-  }
-  ClosePendingStreams();
+  CloseAllIdleStreams();
 }
 
 void AudioOutputDispatcherImpl::RestartStreamsForWedgeFix() {