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..e180a00e68420f1a186c3bd9ed85c60605398404 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_++;

[miu, 2013/10/21 23:44:36]

Can we just increment idle_proxies_ after the if-statement, rather than
increment it and decrement back on failure?

[DaleCurtis, 2013/10/22 20:51:40]

Done.

 
   // 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_--;

[miu, 2013/10/21 23:44:36]

It doesn't look like idle_proxies_ is used for anything.  Can we get rid of it? 
Otherwise:

1. Is it enough to decrement by one here?  What if more than one idle stream is
closed below?

2. Is it ever true that idle_proxies_ is not exactly idle_streams_.size() after
an operation?  Can we just get rid of it?

[DaleCurtis, 2013/10/22 20:51:40]

idle_proxies_ will always be greater than idle_streams_.  There may be only a
single stream which is shared between any number of idle proxies.

I think it's useful in terms of debugging.  You're right though that the check
is currently missing.  I've added a DCHECK during destruction.

[miu, 2013/10/22 21:45:29]

Okay, so it's just for debug builds.

Just a thought, but don't waste the time unless you're bought into this:
Everywhere idle_proxies_ is incremented/decremented, you could wrap it inside a
DCHECK.  That way, it would be entirely compiled out of release builds.

Example:

  DCHECK_GT(idle_proxies_--, 0u);

[DaleCurtis, 2013/10/22 21:52:58]

Hmm, a nice thought, but I think it makes things a bit more confusing and
potentially awkward if later someone comes along and mucks with it outside of a
DCHECK() or changes something to a CHECK().

 
-  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(

[miu, 2013/10/21 23:44:36]

My two cents on the use of std::for_each() here: IMHO, for_each() is not a good
general replacement when you cannot use C++11 language features.  Especially
with Chromium style: You're forced to define the "helper code" far away from the
rest of the functionality.  Here, CloseStreamHelper() is defined at the top of
the file, about 100 lines away from the CloseStream() method itself.  That makes
it harder to read code and understand code dependencies.

Sean Parent's "raw loops" tech talk promotes the use of lambdas and auto typing
to solve the "distant helper code" problem, but we cannot yet use these in
Chromium code.  :(

So, it's up to you, but I think the old code was pretty clean and concise,
modified to be:

  while (idle_streams_.size() > 1) {
    idle_streams_.back()->Close();
    idle_streams_.pop_back();
  }

Also, consider Sean's major points on raw loops: That they are bad because it's
dangerous to place non-O(1) operations within loops, that code is otherwise
difficult to "prove" functionally, and that we should promote building a library
for code reuse by others.  Do any of these points apply here?

[DaleCurtis, 2013/10/22 20:51:40]

I played with this a few different ways and eventually concluded it is easier to
read w/o std::for_each in this case.  My main motivation for changing this was
to avoid the repeated pop_back() in favor of a single erase().  So I've isolated
that change instead.

+        idle_streams_.begin() + 1, idle_streams_.end(), CloseStreamHelper);
+    idle_streams_.erase(idle_streams_.begin() + 1, 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