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

media/audio/audio_output_resampler.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_resampler.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
@@ skipping 156 matching lines @@
       frames_per_buffer);
 }
 
 AudioOutputResampler::AudioOutputResampler(AudioManager* audio_manager,
                                            const AudioParameters& input_params,
                                            const AudioParameters& output_params,
                                            const base::TimeDelta& close_delay)
     : 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.
   RecordStats(output_params);
 
   // Immediately fallback if we're given invalid output parameters.  This may
   // happen if the OS provided us junk values for the hardware configuration.
   if (!output_params_.IsValid()) {
     LOG(ERROR) << "Invalid audio output parameters received; using fallback "
                << "path. Channels: " << output_params_.channels() << ", "
                << "Sample Rate: " << output_params_.sample_rate() << ", "
                << "Bits Per Sample: " << output_params_.bits_per_sample()
                << ", Frames Per Buffer: " << output_params_.frames_per_buffer();
     // Record UMA statistics about the hardware which triggered the failure so
     // we can debug and triage later.
     RecordFallbackStats(output_params);
     output_params_ = SetupFallbackParams(input_params, output_params);
   }
 
   Initialize();
 }
 
-AudioOutputResampler::~AudioOutputResampler() {}
+AudioOutputResampler::~AudioOutputResampler() {
+  DCHECK(callbacks_.empty());
+}
 
 void AudioOutputResampler::Initialize() {
   io_ratio_ = 1;
 
   // TODO(dalecurtis): Add channel remixing.  http://crbug.com/138762
   DCHECK_EQ(params_.channels(), output_params_.channels());
   // Only resample or rebuffer if the input parameters don't match the output
   // parameters to avoid any unnecessary work.
   if (params_.channels() != output_params_.channels() ||
       params_.sample_rate() != output_params_.sample_rate() ||
@@ skipping 19 matching lines @@
     DVLOG(1) << "Input and output params are the same; in pass-through mode.";
   }
 
   // TODO(dalecurtis): All this code should be merged into AudioOutputMixer once
   // we've stabilized the issues there.
   dispatcher_ = new AudioOutputDispatcherImpl(
       audio_manager_, output_params_, close_delay_);
 }
 
 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)) {
     LOG(ERROR) << "Open failed and automatic fallback to high latency audio "
                << "path is disabled, aborting.";
     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.";
 
   // Record UMA statistics about the hardware which triggered the failure so
   // we can debug and triage later.
   RecordFallbackStats(output_params_);
   output_params_ = SetupFallbackParams(params_, output_params_);
   Initialize();
 
   // Retry, if this fails, there's nothing left to do but report the error back.
   return dispatcher_->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());
 }
 
 OnMoreDataResampler::OnMoreDataResampler(
     double io_ratio, const AudioParameters& input_params,
     const AudioParameters& output_params)
     : io_ratio_(io_ratio),
       source_callback_(NULL),
       outstanding_audio_bytes_(0),
@@ skipping 124 matching lines @@
     source_callback_->OnError(stream, code);
 }
 
 void OnMoreDataResampler::WaitTillDataReady() {
   base::AutoLock auto_lock(source_lock_);
   if (source_callback_ && !outstanding_audio_bytes_)
     source_callback_->WaitTillDataReady();
 }
 
 }  // namespace media