Sometimes pulseaudio is restarted if system goes into hibernation or sleep and resumes. When

this happens, we were not reconnecting to pulse, and so could not adjust volume from that point on.

BUG=4017
TEST=In shell, kill the pulseaudio process.  It will auto-restart and volume keys should still work

Review URL: http://codereview.chromium.org/2959015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53367 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 114 insertions, 89 deletions

diff --git a/chrome/browser/chromeos/audio_handler.cc b/chrome/browser/chromeos/audio_handler.cc
index f3157ee..e027d42 100644
--- a/chrome/browser/chromeos/audio_handler.cc
+++ b/chrome/browser/chromeos/audio_handler.cc
@@ -21,6 +21,8 @@ const double kMaxVolumeDb = 6.0;
 // A value of less than one adjusts quieter volumes in larger steps (giving
 // finer resolution in the higher volumes).
 const double kVolumeBias = 0.5;
+// If a connection is lost, we try again this many times
+const int kMaxReconnectTries = 4;
 
 }  // namespace
 
@@ -28,11 +30,9 @@ const double kVolumeBias = 0.5;
 // handles the volume level logic.
 
 // TODO(davej): Serialize volume/mute for next startup?
-// TODO(davej): Should we try to regain a connection if for some reason all was
-// initialized fine, but later IsValid() returned false?  Maybe N retries?
 
-double AudioHandler::GetVolumePercent() const {
-  if (!SanityCheck())
+double AudioHandler::GetVolumePercent() {
+  if (!VerifyMixerConnection())
     return 0;
 
   return VolumeDbToPercent(mixer_->GetVolumeDb());
@@ -41,7 +41,7 @@ double AudioHandler::GetVolumePercent() const {
 // Set volume using our internal 0-100% range.  Notice 0% is a special case of
 // silence, so we set the mixer volume to kSilenceDb instead of kMinVolumeDb.
 void AudioHandler::SetVolumePercent(double volume_percent) {
-  if (!SanityCheck())
+  if (!VerifyMixerConnection())
     return;
   DCHECK(volume_percent >= 0.0);
 
@@ -55,7 +55,7 @@ void AudioHandler::SetVolumePercent(double volume_percent) {
 }
 
 void AudioHandler::AdjustVolumeByPercent(double adjust_by_percent) {
-  if (!SanityCheck())
+  if (!VerifyMixerConnection())
     return;
 
   DLOG(INFO) << "Adjusting Volume by " << adjust_by_percent << " percent";
@@ -79,15 +79,15 @@ void AudioHandler::AdjustVolumeByPercent(double adjust_by_percent) {
     mixer_->SetVolumeDb(new_volume);
 }
 
-bool AudioHandler::IsMute() const {
-  if (!SanityCheck())
+bool AudioHandler::IsMute() {
+  if (!VerifyMixerConnection())
     return false;
 
   return mixer_->IsMute();
 }
 
 void AudioHandler::SetMute(bool do_mute) {
-  if (!SanityCheck())
+  if (!VerifyMixerConnection())
     return;
 
   DLOG(INFO) << "Setting Mute to " << do_mute;
@@ -101,7 +101,8 @@ void AudioHandler::OnMixerInitialized(bool success) {
 }
 
 AudioHandler::AudioHandler()
-    : connected_(false) {
+    : connected_(false),
+      reconnect_tries_(0) {
   mixer_.reset(new PulseAudioMixer());
   if (!mixer_->Init(NewCallback(this, &AudioHandler::OnMixerInitialized))) {
     LOG(ERROR) << "Unable to connect to PulseAudio";
@@ -111,20 +112,36 @@ AudioHandler::AudioHandler()
 AudioHandler::~AudioHandler() {
 };
 
-inline bool AudioHandler::SanityCheck() const {
-  if (!mixer_->IsValid()) {
+bool AudioHandler::VerifyMixerConnection() {
+  PulseAudioMixer::State mixer_state = mixer_->CheckState();
+  if (mixer_state == PulseAudioMixer::READY)
+    return true;
+  if (connected_) {
+    // Something happened and the mixer is no longer valid after having been
+    // initialized earlier.
+    connected_ = false;
+    LOG(ERROR) << "Lost connection to PulseAudio";
+  } else {
+    LOG(ERROR) << "Mixer not valid";
+  }
+
+  if ((mixer_state == PulseAudioMixer::INITIALIZING) ||
+      (mixer_state == PulseAudioMixer::SHUTTING_DOWN))
+    return false;
+
+  if (reconnect_tries_ < kMaxReconnectTries) {
+    reconnect_tries_++;
+    LOG(INFO) << "Re-connecting to PulseAudio attempt " << reconnect_tries_
+              << "/" << kMaxReconnectTries;
+    mixer_.reset(new PulseAudioMixer());
+    connected_ = mixer_->InitSync();
     if (connected_) {
-      // Something happened and the mixer is no longer valid after having been
-      // initialized earlier.
-      // TODO(davej): Try to reconnect now?
-      connected_ = false;
-      LOG(ERROR) << "Lost connection to PulseAudio";
-    } else {
-      LOG(ERROR) << "Mixer not valid";
+      reconnect_tries_ = 0;
+      return true;
     }
-    return false;
+    LOG(ERROR) << "Unable to re-connect to PulseAudio";
   }
-  return true;
+  return false;
 }
 
 // VolumeDbToPercent() and PercentToVolumeDb() conversion functions allow us
diff --git a/chrome/browser/chromeos/audio_handler.h b/chrome/browser/chromeos/audio_handler.h
index 7e443a6..1394b2a6 100644
--- a/chrome/browser/chromeos/audio_handler.h
+++ b/chrome/browser/chromeos/audio_handler.h
@@ -22,7 +22,7 @@ class AudioHandler {
   // Volume may go above 100% if another process changes PulseAudio's volume.
   // Returns default of 0 on error.  This function will block until the volume
   // is retrieved or fails.  Blocking call.
-  double GetVolumePercent() const;
+  double GetVolumePercent();
 
   // Set volume level from 0-100%.  Volumes above 100% are OK and boost volume,
   // although clipping will occur more at higher volumes.  Volume gets quieter
@@ -36,7 +36,7 @@ class AudioHandler {
 
   // Just returns true if mute, false if not or an error occurred.
   // Blocking call.
-  bool IsMute() const;
+  bool IsMute();
 
   // Mutes all audio.  Non-blocking call.
   void SetMute(bool do_mute);
@@ -50,14 +50,15 @@ class AudioHandler {
 
   AudioHandler();
   virtual ~AudioHandler();
-  inline bool SanityCheck() const;
+  bool VerifyMixerConnection();
 
   // Conversion between our internal scaling (0-100%) and decibels.
   static double VolumeDbToPercent(double volume_db);
   static double PercentToVolumeDb(double volume_percent);
 
   scoped_ptr<PulseAudioMixer> mixer_;
-  mutable bool connected_;  // Mutable for sanity checking only
+  bool connected_;
+  int reconnect_tries_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioHandler);
 };
diff --git a/chrome/browser/chromeos/pulse_audio_mixer.cc b/chrome/browser/chromeos/pulse_audio_mixer.cc
index a091c8c..cef8716 100644
--- a/chrome/browser/chromeos/pulse_audio_mixer.cc
+++ b/chrome/browser/chromeos/pulse_audio_mixer.cc
@@ -58,27 +58,24 @@ PulseAudioMixer::~PulseAudioMixer() {
 }
 
 bool PulseAudioMixer::Init(InitDoneCallback* callback) {
-  // Just start up worker thread, then post the task of starting up, which can
-  // block for 200-500ms, so best not to do it on this thread.
-  if (mixer_state_ != UNINITIALIZED)
+  if (!InitThread())
     return false;
 
-  mixer_state_ = INITIALIZING;
-  if (thread_ == NULL) {
-    thread_.reset(new base::Thread("PulseAudioMixer"));
-    if (!thread_->Start()) {
-      thread_.reset();
-      return false;
-    }
-  }
+  // Post the task of starting up, which can block for 200-500ms,
+  // so best not to do it on the caller's thread.
   thread_->message_loop()->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &PulseAudioMixer::DoInit,
-                        callback));
+    NewRunnableMethod(this, &PulseAudioMixer::DoInit, callback));
   return true;
 }
 
+bool PulseAudioMixer::InitSync() {
+  if (!InitThread())
+    return false;
+  return PulseAudioInit();
+}
+
 double PulseAudioMixer::GetVolumeDb() const {
-  if (!PulseAudioValid())
+  if (CheckState() != READY)
     return pa_sw_volume_to_dB(0);  // this returns -inf
   AudioInfo data;
   GetAudioInfo(&data);
@@ -87,7 +84,7 @@ double PulseAudioMixer::GetVolumeDb() const {
 
 void PulseAudioMixer::GetVolumeDbAsync(GetVolumeCallback* callback,
                                        void* user) {
-  if (!PulseAudioValid())
+  if (CheckState() != READY)
     return;
   thread_->message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(this,
@@ -96,7 +93,7 @@ void PulseAudioMixer::GetVolumeDbAsync(GetVolumeCallback* callback,
 }
 
 void PulseAudioMixer::SetVolumeDb(double vol_db) {
-  if (!PulseAudioValid())
+  if (CheckState() != READY)
     return;
 
   // last_channels_ determines the number of channels on the main output device,
@@ -118,7 +115,7 @@ void PulseAudioMixer::SetVolumeDb(double vol_db) {
 }
 
 bool PulseAudioMixer::IsMute() const {
-  if (!PulseAudioValid())
+  if (CheckState() != READY)
     return false;
   AudioInfo data;
   GetAudioInfo(&data);
@@ -126,7 +123,7 @@ bool PulseAudioMixer::IsMute() const {
 }
 
 void PulseAudioMixer::SetMute(bool mute) {
-  if (!PulseAudioValid())
+  if (CheckState() != READY)
     return;
   pa_operation* pa_op;
   pa_threaded_mainloop_lock(pa_mainloop_);
@@ -136,14 +133,13 @@ void PulseAudioMixer::SetMute(bool mute) {
   pa_threaded_mainloop_unlock(pa_mainloop_);
 }
 
-bool PulseAudioMixer::IsValid() const {
-  if (mixer_state_ == READY)
-    return true;
-  if (!pa_context_)
-    return false;
-  if (pa_context_get_state(pa_context_) != PA_CONTEXT_READY)
-    return false;
-  return true;
+PulseAudioMixer::State PulseAudioMixer::CheckState() const {
+  AutoLock lock(mixer_state_lock_);
+  // If we think it's ready, verify it is actually so.
+  if ((mixer_state_ == READY) &&
+      (pa_context_get_state(pa_context_) != PA_CONTEXT_READY))
+    mixer_state_ = IN_ERROR;
+  return mixer_state_;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -161,6 +157,24 @@ void PulseAudioMixer::DoGetVolume(GetVolumeCallback* callback,
   delete callback;
 }
 
+bool PulseAudioMixer::InitThread() {
+  {
+    AutoLock lock(mixer_state_lock_);
+    if (mixer_state_ != UNINITIALIZED)
+      return false;
+    mixer_state_ = INITIALIZING;
+  }
+
+  if (thread_ == NULL) {
+    thread_.reset(new base::Thread("PulseAudioMixer"));
+    if (!thread_->Start()) {
+      thread_.reset();
+      return false;
+    }
+  }
+  return true;
+}
+
 struct ConnectToPulseCallbackData {
   PulseAudioMixer* instance;
   bool connect_done;
@@ -256,8 +270,10 @@ bool PulseAudioMixer::PulseAudioInit() {
 
     last_channels_ = 0;
     GetDefaultPlaybackDevice();
-    mixer_state_ = READY;
+    if (device_id_ == kInvalidDeviceId)
+      break;
 
+    set_mixer_state(READY);
     return true;
   }
 
@@ -269,9 +285,8 @@ bool PulseAudioMixer::PulseAudioInit() {
 void PulseAudioMixer::PulseAudioFree() {
   if (!pa_mainloop_)
     return;
-
   DCHECK_NE(mixer_state_, UNINITIALIZED);
-  mixer_state_ = SHUTTING_DOWN;
+  set_mixer_state(SHUTTING_DOWN);
 
   if (pa_context_) {
     pa_threaded_mainloop_lock(pa_mainloop_);
@@ -285,25 +300,7 @@ void PulseAudioMixer::PulseAudioFree() {
   pa_threaded_mainloop_free(pa_mainloop_);
   pa_mainloop_ = NULL;
 
-  mixer_state_ = UNINITIALIZED;
-}
-
-bool PulseAudioMixer::PulseAudioValid() const {
-  if (mixer_state_ != READY)
-    return false;
-  if (!pa_context_) {
-    DLOG(ERROR) << "Trying to use PulseAudio when no context";
-    return false;
-  }
-  if (pa_context_get_state(pa_context_) != PA_CONTEXT_READY) {
-    LOG(ERROR) << "PulseAudio context not ready ("
-               << pa_context_get_state(pa_context_) << ")";
-    return false;
-  }
-  if (device_id_ == kInvalidDeviceId)
-    return false;
-
-  return true;
+  set_mixer_state(UNINITIALIZED);
 }
 
 void PulseAudioMixer::CompleteOperationAndUnlock(pa_operation* pa_op) const {
diff --git a/chrome/browser/chromeos/pulse_audio_mixer.h b/chrome/browser/chromeos/pulse_audio_mixer.h
index 223fe33..d3ce54a 100644
--- a/chrome/browser/chromeos/pulse_audio_mixer.h
+++ b/chrome/browser/chromeos/pulse_audio_mixer.h
@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/lock.h"
 #include "base/scoped_ptr.h"
 #include "base/thread.h"
 
@@ -20,6 +21,14 @@ namespace chromeos {
 
 class PulseAudioMixer {
  public:
+  enum State {
+    UNINITIALIZED = 0,
+    INITIALIZING,
+    READY,
+    SHUTTING_DOWN,
+    IN_ERROR
+  };
+
   PulseAudioMixer();
   ~PulseAudioMixer();
 
@@ -28,6 +37,9 @@ class PulseAudioMixer {
   typedef Callback1<bool>::Type InitDoneCallback;
   bool Init(InitDoneCallback* callback);
 
+  // Blocking init call guarantees PulseAudio is started before returning.
+  bool InitSync();
+
   // Blocking call. Returns a default of -inf on error.
   double GetVolumeDb() const;
 
@@ -45,20 +57,14 @@ class PulseAudioMixer {
   // Non-Blocking call.
   void SetMute(bool mute);
 
-  // Call any time to see if we have a valid working connection to PulseAudio.
-  // Non-blocking call.
-  bool IsValid() const;
+  // Returns READY if we have a valid working connection to PulseAudio.
+  // This can return IN_ERROR if we lose the connection, even after an original
+  // successful init.  Non-blocking call.
+  State CheckState() const;
 
  private:
   struct AudioInfo;
 
-  enum State {
-    UNINITIALIZED = 0,
-    INITIALIZING,
-    READY,
-    SHUTTING_DOWN
-  };
-
   // These are the tasks to be run in the background on the worker thread.
   void DoInit(InitDoneCallback* callback);
   void DoGetVolume(GetVolumeCallback* callback, void* user);
@@ -66,6 +72,9 @@ class PulseAudioMixer {
   static void ConnectToPulseCallbackThunk(pa_context* c, void* userdata);
   void OnConnectToPulseCallback(pa_context* c, bool* connect_done);
 
+  // Helper function to just get our messsage loop thread going
+  bool InitThread();
+
   // This goes through sequence of connecting to the default PulseAudio server.
   // We will block until we either have a valid connection or something failed.
   // If a connection is lost for some reason, delete and recreate the object.
@@ -74,11 +83,6 @@ class PulseAudioMixer {
   // PulseAudioFree.  Disconnect from server.
   void PulseAudioFree();
 
-  // Check if the PA system is ready for communication, as well as if a default
-  // device is available to talk to.  This can return false if we lose the
-  // connection, even after an original successful init.
-  bool PulseAudioValid() const;
-
   // Iterates the PA mainloop and only returns once an operation has completed
   // (successfully or unsuccessfully).  This call only blocks the worker thread.
   void CompleteOperationAndUnlock(pa_operation* pa_op) const;
@@ -101,12 +105,18 @@ class PulseAudioMixer {
                                    int eol,
                                    void* userdata);
 
+  void set_mixer_state(State state) {
+    AutoLock lock(mixer_state_lock_);
+    mixer_state_ = state;
+  }
+
   // The PulseAudio index of the main device being used.
   mutable int device_id_;
 
   // Set to the number of channels on the main device.
   int last_channels_;
-  State mixer_state_;
+  mutable Lock mixer_state_lock_;
+  mutable State mixer_state_;
 
   // Cached contexts for use in PulseAudio calls.
   pa_context* pa_context_;