Don't fallback if we've successfully opened a stream previously.

media/audio/audio_output_resampler.cc

Index: media/audio/audio_output_resampler.cc
diff --git a/media/audio/audio_output_resampler.cc b/media/audio/audio_output_resampler.cc
index 40c84a9e21469872102724bfdde1002d241cdaa2..11da5f8ed03721bd255a0e3bb492e08516d6c5bb 100644
--- a/media/audio/audio_output_resampler.cc
+++ b/media/audio/audio_output_resampler.cc
@@ -174,7 +174,8 @@ AudioOutputResampler::AudioOutputResampler(AudioManager* audio_manager,
     : AudioOutputDispatcher(audio_manager, input_params),
       io_ratio_(1),
       close_delay_(close_delay),
-      output_params_(output_params) {
+      output_params_(output_params),
+      streams_opened_(false) {
   DCHECK_EQ(output_params_.format(), AudioParameters::AUDIO_PCM_LOW_LATENCY);
 
   // Record UMA statistics for the hardware configuration.
@@ -197,7 +198,9 @@ AudioOutputResampler::AudioOutputResampler(AudioManager* audio_manager,
   Initialize();
 }
 
-AudioOutputResampler::~AudioOutputResampler() {}
+AudioOutputResampler::~AudioOutputResampler() {
+  DCHECK(callbacks_.empty());
+}
 
 void AudioOutputResampler::Initialize() {
   io_ratio_ = 1;
@@ -237,17 +240,25 @@ void AudioOutputResampler::Initialize() {
 }
 
 bool AudioOutputResampler::OpenStream() {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
   if (dispatcher_->OpenStream()) {

[scherkus (not reviewing), 2012/09/21 01:24:18]

I'm not a fan of the implicit stateness in this class/function w.r.t. low
latency vs fallback vs streams are open vs we've failed

It'd be preferable to have the states more explicit w/ the implicit parts
DCHECK'd but I don't think it's possible w/ today's AOD/AOP classes.

For example, something like the following would be nice as it goes through the
same path from the top of the function every time, but I don't think it's quite
equivalent.

while (true) {
  switch (mode_) {
    case kLowLatency:
      DCHECK_EQ(output_params are LOW_LATENCY);
      if (dispatcher_->Open())
        return true;
      if (fallback not disabled) {
        mode_ = kHighLatency;
        output_params_ = SetupFallbackParams();
        Initialize();
        continue;
      }
      return false;

    case kHighLatency:
      DCHECK_EQ(output_params are PCM_LINEAR);
      DCHECK(fallback flag is enabled)
      return dispatcher_->Open();
  }
}


Any ideas?

[DaleCurtis, 2012/09/21 02:57:27]

I'm not sure it's any more readable, but I've uploaded two patch sets, the first
(#3) using the old method and the second (#4) with a semi-state machine based
approach modeled after your example.  For ~40 lines of code, I don't think it
really matters which approach we use, but the new one will need extra scrubbing
if we want to land it since it's not as vetted.

+    streams_opened_ = true;
     // Only record the UMA statistic if we didn't fallback during construction.
     if (output_params_.format() == AudioParameters::AUDIO_PCM_LOW_LATENCY)
       UMA_HISTOGRAM_BOOLEAN("Media.FallbackToHighLatencyAudioPath", false);
     return true;
   }
 
-  // If we've already tried to open the stream in high latency mode, there's
-  // nothing more to be done.
-  if (output_params_.format() == AudioParameters::AUDIO_PCM_LINEAR)
+  // If we've already tried to open the stream in high latency mode or we've
+  // successfully opened a stream previously, there's nothing more to be done.
+  if (output_params_.format() == AudioParameters::AUDIO_PCM_LINEAR ||
+      streams_opened_ || !callbacks_.empty()) {
     return false;
+  }
+
+  DCHECK(!streams_opened_);
+  DCHECK(callbacks_.empty());
 
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableAudioFallback)) {
@@ -256,10 +267,6 @@ bool AudioOutputResampler::OpenStream() {
     return false;
   }
 
-  // TODO(dalecurtis): Is it better to recreate the whole |dispatcher_| ?  See
-  // http://crbug.com/149815
-  dispatcher_->CloseStream(NULL);
-
   DLOG(ERROR) << "Unable to open audio device in low latency mode.  Falling "
               << "back to high latency audio output.";
 
@@ -276,63 +283,59 @@ bool AudioOutputResampler::OpenStream() {
 bool AudioOutputResampler::StartStream(
     AudioOutputStream::AudioSourceCallback* callback,
     AudioOutputProxy* stream_proxy) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
   OnMoreDataResampler* resampler_callback = NULL;
-  {
-    base::AutoLock auto_lock(callbacks_lock_);
-    CallbackMap::iterator it = callbacks_.find(stream_proxy);
-    if (it == callbacks_.end()) {
-      resampler_callback = new OnMoreDataResampler(
-          io_ratio_, params_, output_params_);
-      callbacks_[stream_proxy] = resampler_callback;
-    } else {
-      resampler_callback = it->second;
-    }
-    resampler_callback->Start(callback);
+  CallbackMap::iterator it = callbacks_.find(stream_proxy);
+  if (it == callbacks_.end()) {
+    resampler_callback = new OnMoreDataResampler(
+        io_ratio_, params_, output_params_);
+    callbacks_[stream_proxy] = resampler_callback;
+  } else {
+    resampler_callback = it->second;
   }
+  resampler_callback->Start(callback);
   return dispatcher_->StartStream(resampler_callback, stream_proxy);
 }
 
 void AudioOutputResampler::StreamVolumeSet(AudioOutputProxy* stream_proxy,
                                            double volume) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
   dispatcher_->StreamVolumeSet(stream_proxy, volume);
 }
 
 void AudioOutputResampler::StopStream(AudioOutputProxy* stream_proxy) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
   dispatcher_->StopStream(stream_proxy);
 
   // Now that StopStream() has completed the underlying physical stream should
   // be stopped and no longer calling OnMoreData(), making it safe to Stop() the
   // OnMoreDataResampler.
-  {
-    base::AutoLock auto_lock(callbacks_lock_);
-    CallbackMap::iterator it = callbacks_.find(stream_proxy);
-    if (it != callbacks_.end())
-      it->second->Stop();
-  }
+  CallbackMap::iterator it = callbacks_.find(stream_proxy);
+  if (it != callbacks_.end())
+    it->second->Stop();
 }
 
 void AudioOutputResampler::CloseStream(AudioOutputProxy* stream_proxy) {

[scherkus (not reviewing), 2012/09/21 01:24:18]

to confirm.. we never have to reset streams_opened_ to false?

[DaleCurtis, 2012/09/21 02:57:27]

Nope, especially since AudioOutputDispatcherImpl may be holding on to streams
under the hood.  Once an open has succeeded we can't take the fallback path.

-  // Force StopStream() before CloseStream().
-  // TODO(dalecurtis): This shouldn't be necessary, but somewhere in the chain
-  // CloseStream() is occurring without a StopStream() which causes the callback
-  // provided by OnMoreDataResampler to go away before the output stream is
-  // ready.  http://crbug.com/150619
-  StopStream(stream_proxy);
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
   dispatcher_->CloseStream(stream_proxy);
 
   // We assume that StopStream() is always called prior to CloseStream(), so
   // that it is safe to delete the OnMoreDataResampler here.
-  {
-    base::AutoLock auto_lock(callbacks_lock_);
-    CallbackMap::iterator it = callbacks_.find(stream_proxy);
-    if (it != callbacks_.end()) {
-      delete it->second;
-      callbacks_.erase(it);
-    }
+  CallbackMap::iterator it = callbacks_.find(stream_proxy);
+  if (it != callbacks_.end()) {
+    delete it->second;
+    callbacks_.erase(it);
   }
 }
 
 void AudioOutputResampler::Shutdown() {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  // No AudioOutputProxy objects should hold a reference to us when we get
+  // to this stage.
+  DCHECK(HasOneRef()) << "Only the AudioManager should hold a reference";

[scherkus (not reviewing), 2012/09/21 01:24:18]

did this end up firing?

[DaleCurtis, 2012/09/21 02:57:27]

During testing when I messed up the AOP/AOC state machine, yes.  It's a check
which AODI used to enforce which it can't any more since AOR is the primary
interface and holds a single reference to AODI.

+
   dispatcher_->Shutdown();
   DCHECK(callbacks_.empty());
 }