Save/Restore ALSA volume/mute

chrome/browser/chromeos/audio_mixer_alsa.cc

Index: chrome/browser/chromeos/audio_mixer_alsa.cc
diff --git a/chrome/browser/chromeos/audio_mixer_alsa.cc b/chrome/browser/chromeos/audio_mixer_alsa.cc
index 7f96fa4f2cd9daaa67abbc0bdc61c63940f78256..f9aa52e210e1a02145af2fd1dce977f49e717806 100644
--- a/chrome/browser/chromeos/audio_mixer_alsa.cc
+++ b/chrome/browser/chromeos/audio_mixer_alsa.cc
@@ -8,6 +8,10 @@
 
 #include "base/logging.h"
 #include "base/task.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/common/pref_names.h"
 
 namespace chromeos {
 
@@ -19,8 +23,6 @@ namespace chromeos {
 // adjusting that as well.  If the PCM element has more volume steps, it allows
 // for finer granularity in the total volume.
 
-// TODO(davej): Serialize volume/mute to preserve settings when restarting.
-
 typedef long alsa_long_t;  // 'long' is required for ALSA API calls.
 
 namespace {
@@ -29,6 +31,7 @@ const char* kMasterVolume = "Master";
 const char* kPCMVolume = "PCM";
 const double kDefaultMinVolume = -90.0;
 const double kDefaultMaxVolume = 0.0;
+const double kPrefVolumeNotSet = -999.0;
 
 }  // namespace
 
@@ -90,10 +93,13 @@ bool AudioMixerAlsa::GetVolumeLimits(double* vol_min, double* vol_max) {
 }
 
 void AudioMixerAlsa::SetVolumeDb(double vol_db) {
-  AutoLock lock(mixer_state_lock_);
-  if (mixer_state_ != READY)
-    return;
-  DoSetVolumeDb_Locked(vol_db);
+  {
+    AutoLock lock(mixer_state_lock_);
+    if (mixer_state_ != READY)
+      return;
+    DoSetVolumeDb_Locked(vol_db);
+  }
+  SaveVolume(vol_db);
 }
 
 bool AudioMixerAlsa::IsMute() const {
@@ -104,30 +110,34 @@ bool AudioMixerAlsa::IsMute() const {
 }
 
 void AudioMixerAlsa::SetMute(bool mute) {
-  AutoLock lock(mixer_state_lock_);
-  if (mixer_state_ != READY)
-    return;
+  {
+    AutoLock lock(mixer_state_lock_);
+
+    if (mixer_state_ != READY)
+      return;
 
-  // Set volume to minimum on mute, since switching the element off does not
-  // always mute as it should.
+    // Set volume to minimum on mute, since switching the element off does not
+    // always mute as it should.
 
-  // TODO(davej): Setting volume to minimum can be removed once switching the
-  //              element off can be guaranteed to work.
+    // TODO(davej): Remove save_volume_ and setting volume to minimum if
+    //              switching the element off can be guaranteed to mute it.
 
-  bool old_value = GetElementMuted_Locked(elem_master_);
+    bool old_value = GetElementMuted_Locked(elem_master_);
 
-  if (old_value != mute) {
-    if (mute) {
-      save_volume_ = DoGetVolumeDb_Locked();
-      DoSetVolumeDb_Locked(min_volume_);
-    } else {
-      DoSetVolumeDb_Locked(save_volume_);
+    if (old_value != mute) {
+      if (mute) {
+        save_volume_ = DoGetVolumeDb_Locked();
+        DoSetVolumeDb_Locked(min_volume_);
+      } else {
+        DoSetVolumeDb_Locked(save_volume_);
+      }
     }
-  }
 
-  SetElementMuted_Locked(elem_master_, mute);
-  if (elem_pcm_)
-    SetElementMuted_Locked(elem_pcm_, mute);
+    SetElementMuted_Locked(elem_master_, mute);
+    if (elem_pcm_)
+      SetElementMuted_Locked(elem_pcm_, mute);
+  }
+  SaveMute(mute);
 }
 
 AudioMixer::State AudioMixerAlsa::GetState() const {
@@ -144,6 +154,12 @@ AudioMixer::State AudioMixerAlsa::GetState() const {
 void AudioMixerAlsa::DoInit(InitDoneCallback* callback) {
   bool success = InitializeAlsaMixer();
 
+  if (success) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        NewRunnableMethod(this, &AudioMixerAlsa::RestoreVolumeMute));
+  }
+
   if (callback) {
     callback->Run(success);
     delete callback;
@@ -241,6 +257,24 @@ void AudioMixerAlsa::FreeAlsaMixer() {
   }
 }
 
+void AudioMixerAlsa::DoSetVolumeMute(double volume, bool mute) {
+  AutoLock lock(mixer_state_lock_);
+  if (mixer_state_ != READY)
+    return;
+
+  VLOG(1) << "Setting volume to " << volume << "and mute to " << mute;
+  if (mute) {
+    save_volume_ = volume;
+    DoSetVolumeDb_Locked(min_volume_);
+  } else {
+    DoSetVolumeDb_Locked(volume);
+  }
+
+  SetElementMuted_Locked(elem_master_, mute);
+  if (elem_pcm_)
+    SetElementMuted_Locked(elem_pcm_, mute);
+}
+
 double AudioMixerAlsa::DoGetVolumeDb_Locked() const {
   double vol_total = 0.0;
   GetElementVolume_Locked(elem_master_, &vol_total);
@@ -362,5 +396,62 @@ void AudioMixerAlsa::SetElementMuted_Locked(snd_mixer_elem_t* elem, bool mute) {
           << " to " << enabled;
 }
 
+void AudioMixerAlsa::RestoreVolumeMute() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!g_browser_process)
+    return;
+
+  PrefService* local_state = g_browser_process->local_state();

[scherkus (not reviewing), 2011/01/13 00:54:52]

take a look at PrefMember which wraps up all these stuff for you in a
super-easy-to-use member variable!

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prefs/pref_mem...

You may also want to pass in the PrefService object via constructor instead of
using g_browser_process (dependency injection kinda thing)

Take a look at chromeos::Preference as a example:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chromeos/prefe...

[davejcool, 2011/01/13 21:31:14]

Thanks for the PrefMember pointer!  This greatly simplfied things.  I was able
to wrap up all the init and default logic into a single InitPrefs() function.

But passing in the PrefService* to the init didn't make sense since it would
have to be passed in again up the chain through AudioHandler and
SystemKeyEventListener which have no need of prefs before we get to BrowserInit
which uses prefs.

There are also many examples of classes which just obtain the PrefService* as
needed the same we we do here.

+  if (!local_state) {
+    LOG(ERROR) << "Cannot get local_state for prefs, not initializing volume";
+    return;
+  }
+
+  if (!local_state->FindPreference(prefs::kAudioVolume)) {
+    local_state->RegisterRealPref(prefs::kAudioVolume, kPrefVolumeNotSet);
+    local_state->RegisterBooleanPref(prefs::kAudioMute, false);
+  }
+
+  DCHECK(local_state->FindPreference(prefs::kAudioVolume));
+  double pref_volume = local_state->GetReal(prefs::kAudioVolume);
+
+  if (pref_volume != kPrefVolumeNotSet) {
+    DCHECK(local_state->FindPreference(prefs::kAudioMute));
+    bool pref_mute = local_state->GetBoolean(prefs::kAudioMute);
+
+    thread_->message_loop()->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &AudioMixerAlsa::DoSetVolumeMute,
+                          pref_volume, pref_mute));
+  }
+}
+
+void AudioMixerAlsa::SaveVolume(double volume) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!g_browser_process)
+    return;
+
+  PrefService* local_state = g_browser_process->local_state();
+  if (!local_state) {
+    LOG(ERROR) << "Cannot get local_state to store volume in prefs";
+    return;
+  }
+  DCHECK(local_state->FindPreference(prefs::kAudioVolume));
+  local_state->SetReal(prefs::kAudioVolume, volume);
+}
+
+void AudioMixerAlsa::SaveMute(bool mute) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!g_browser_process)
+    return;
+
+  PrefService* local_state = g_browser_process->local_state();
+  if (!local_state) {

[zhurunz, 2011/01/13 00:19:25]

Could we add a util function to get local_state so the same code don't need
appear several times.

[davejcool, 2011/01/13 21:31:14]

By using the PrefMember class, I was able to encapsulate all the special bits
into a single InitPrefs() function.

+    LOG(ERROR) << "Cannot get local_state to store mute in prefs";
+    return;
+  }
+  DCHECK(local_state->FindPreference(prefs::kAudioMute));
+  local_state->SetBoolean(prefs::kAudioMute, mute);
+}
+
 }  // namespace chromeos