Improve and simplify AudioOutputDispatcher.

media/audio/audio_output_dispatcher_impl.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/audio_output_dispatcher_impl.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_output_proxy.h"
 
 namespace media {
 
 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::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())
+    return false;
 
-  // Ensure that there is at least one open stream.
-  if (idle_streams_.empty() && !CreateAndOpenStream()) {
-    paused_proxies_--;
-    return false;
-  }
-
+  ++idle_proxies_;
   close_timer_.Reset();
   return true;
 }
 
 bool AudioOutputDispatcherImpl::StartStream(
     AudioOutputStream::AudioSourceCallback* callback,
     AudioOutputProxy* stream_proxy) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(proxy_to_physical_map_.find(stream_proxy) ==
          proxy_to_physical_map_.end());
 
   if (idle_streams_.empty() && !CreateAndOpenStream())
     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;
 }
 
 void AudioOutputDispatcherImpl::StopStream(AudioOutputProxy* stream_proxy) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   AudioStreamMap::iterator it = proxy_to_physical_map_.find(stream_proxy);
   DCHECK(it != proxy_to_physical_map_.end());
   AudioOutputStream* physical_stream = it->second;
   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,
                                                 double volume) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   AudioStreamMap::iterator it = proxy_to_physical_map_.find(stream_proxy);
   if (it != proxy_to_physical_map_.end()) {
     AudioOutputStream* physical_stream = it->second;
     physical_stream->SetVolume(volume);
   }
 }
 
-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(idle_proxies_, 0u);
+  --idle_proxies_;
 
-  DCHECK_GT(paused_proxies_, 0u);
-  paused_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() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   AudioOutputStream* stream = audio_manager_->MakeAudioOutputStream(
       params_, output_device_id_, input_device_id_);
   if (!stream)
     return false;
 
   if (!stream->Open()) {
     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() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   // Should only be called when the dispatcher is used with fake streams which
   // don't need to be shutdown or restarted.
   CHECK_EQ(params_.format(), AudioParameters::AUDIO_FAKE);
 }
 
 }  // namespace media