Save/Restore ALSA volume/mute

Save/Restore ALSA volume/mute

When the ALSA volume mixer is used (if PulseAudio is disabled), the current volume and mute settings will be stored in the browser prefs so they can be restored when starting a new session.

BUG=chromium-os:10470
TEST=Manual, volume and mute setting via keyboard should persist after rebooting.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71734

[davejcool, 2011-01-12 23:51:08]

This patch saves the current volume and mute settings into the browser prefs so
that on subsequent runs they can be restored.  PulseAudio had a specific module
for doing this which is not present when using ALSA alone.

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

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
chrome/browser/chromeos/audio_mixer_alsa.cc:448: if (!local_state) {
Could we add a util function to get local_state so the same code don't need
appear several times.

http://codereview.chromium.org/6118006/diff/2001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6118006/diff/2001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:167: #endif
Keep the list alpha sorted might be a good idea.

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

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
chrome/browser/chromeos/audio_mixer_alsa.cc:404: PrefService* local_state =
g_browser_process->local_state();
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:13]

I've simplified things quite a bit here by using PrefMember variables.

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
chrome/browser/chromeos/audio_mixer_alsa.cc:404: PrefService* local_state =
g_browser_process->local_state();
On 2011/01/13 00:54:52, scherkus wrote:
> 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...

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.

http://codereview.chromium.org/6118006/diff/2001/chrome/browser/chromeos/audi...
chrome/browser/chromeos/audio_mixer_alsa.cc:448: if (!local_state) {
On 2011/01/13 00:19:25, zhurunz wrote:
> Could we add a util function to get local_state so the same code don't need
> appear several times.

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

http://codereview.chromium.org/6118006/diff/2001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6118006/diff/2001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:167: #endif
On 2011/01/13 00:19:25, zhurunz wrote:
> Keep the list alpha sorted might be a good idea.

Since it wasn't sorted already, I wasn't sure where to put it.  But since "A"
belongs at the beginning, I put it at the top.

[Daniel Erat, 2011-01-13 21:37:21]

http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:289: if (volume_pref_.GetValue() !=
kPrefVolumeNotSet)
Could you do something like GetValue() <= kDefaultMinVolume or track whether the
pref is set or not in a bool or something?  Floating-point equality comparisons
scare me.

[Daniel Erat, 2011-01-13 21:37:45]

On 2011/01/13 21:37:21, Daniel Erat wrote:
>
http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
> File chrome/browser/chromeos/audio_mixer_alsa.cc (right):
> 
>
http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
> chrome/browser/chromeos/audio_mixer_alsa.cc:289: if (volume_pref_.GetValue()
!=
> kPrefVolumeNotSet)
> Could you do something like GetValue() <= kDefaultMinVolume or track whether
the
> pref is set or not in a bool or something?  Floating-point equality
comparisons
> scare me.

Uh, I meant GetValue() < kDefaultMinVolume.

[davejcool, 2011-01-14 00:14:11]

Small update... I'm using kSilenceDb as the minimum valid value, and defaulting
the pref volume to less than this for easy detection of being unset without
having a direct equality comparison.

http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/19001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:289: if (volume_pref_.GetValue() !=
kPrefVolumeNotSet)
On 2011/01/13 21:37:21, Daniel Erat wrote:
> Could you do something like GetValue() <= kDefaultMinVolume or track whether
the
> pref is set or not in a bool or something?  Floating-point equality
comparisons
> scare me.

I recall problems in the distant past with comparing doubles as well.  I do see
pref tests that verify doubles are equal when set and then read back.  But I
updated the comparison here anyway so there should be no question.

[scherkus (not reviewing), 2011-01-14 00:54:48]

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:123: //              switching the
element off can be guaranteed to mute it.
nit: no need to align comment here

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:186: void
AudioMixerAlsa::InitPrefs() {
might be helpful to add DCHECKs to methods that must be run on certain threads:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))

we have some similarly named functions (InitializeAlsaMixer, InitThread,
InitPrefs) but they must be run on certain threads

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:192:
prefs->RegisterRealPref(prefs::kAudioVolume, kPrefVolumeNotSet);
sanity check: do you know whether there are other chrome os prefs that get
registered outside of chromeos::Preferences class?

just want to make sure we don't start scattering prefs around, even though I'm
preeeeeeetty sure it's OK thing to do

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:272: void
AudioMixerAlsa::DoSetVolumeMute(double volume, bool mute) {
hmm I only see one callee.. perhaps inline into RestoreVolumeMuteOnUIThread()
and reverse the condition to return early?

do you have future plans for this function?

[davejcool, 2011-01-14 02:23:47]

Thanks for the careful reviews!  here are some responses...

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:186: void
AudioMixerAlsa::InitPrefs() {
On 2011/01/14 00:54:49, scherkus wrote:
> might be helpful to add DCHECKs to methods that must be run on certain
threads:
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))
> 
> we have some similarly named functions (InitializeAlsaMixer, InitThread,
> InitPrefs) but they must be run on certain threads

The ALSA calls do not need to be called on the UI thread, but any prefs access
must be on the UI thread.  Since there were alraedy DCHECKs in prefs for this, I
didn't put them here.  But I guess it doesn't hurt to have some extra checks
around.

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:192:
prefs->RegisterRealPref(prefs::kAudioVolume, kPrefVolumeNotSet);
On 2011/01/14 00:54:49, scherkus wrote:
> sanity check: do you know whether there are other chrome os prefs that get
> registered outside of chromeos::Preferences class?
> 
> just want to make sure we don't start scattering prefs around, even though I'm
> preeeeeeetty sure it's OK thing to do

It looked like the preferences in chromeos::Preferences were for items in the
Settings screen.  Several chromeos classes register their own prefs as well, but
all seem to be passed in the PrefService* in from browser::RegisterLocalState()
and RegisterUserPrefs().  I didn't really want to touch so many files with this,
but I could mimic the others by having a RegisterPrefs() function that is called
by RegisterLocalState().

http://codereview.chromium.org/6118006/diff/25001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:272: void
AudioMixerAlsa::DoSetVolumeMute(double volume, bool mute) {
On 2011/01/14 00:54:49, scherkus wrote:
> hmm I only see one callee.. perhaps inline into RestoreVolumeMuteOnUIThread()
> and reverse the condition to return early?
> 
> do you have future plans for this function?

Originally I had it calling DoSetVolumeMute on the worker thread, so maybe I
should have it do that, since the more that is done away from the UI thread the
better?

[davejcool, 2011-01-14 21:48:19]

And here's an update addressing Andrew's latest comments.

[scherkus (not reviewing), 2011-01-14 23:38:50]

LGTM w/ one tiny nit

feel free to checkin!

http://codereview.chromium.org/6118006/diff/31001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/31001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:152: if
(!local_state->FindPreference(prefs::kAudioVolume)) {
nit: what about checking for kAudioMute?

[davejcool, 2011-01-15 03:49:07]

The case of simply muting and restarting would not restore the mute (since
volume was still unset).  Now all is fixed by handling invalid mute and invalid
volume independently.  When restoring, if the value is invalid, the current
mixer value is used.

http://codereview.chromium.org/6118006/diff/31001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio_mixer_alsa.cc (right):

http://codereview.chromium.org/6118006/diff/31001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio_mixer_alsa.cc:152: if
(!local_state->FindPreference(prefs::kAudioVolume)) {
On 2011/01/14 23:38:50, scherkus wrote:
> nit: what about checking for kAudioMute?

Hmmmm... This unraveled an issue with the case of the user only does a mute,
restarts, and now we don't restore the mute because volume is still invalid.  So
to fix this, both volume and mute prefs are each handled individually.  This
means the stored pref for mute can be On, Off, or Unset, which is why I'm
changing this to storing an int instead of a bool.

[NEHoban, 2011-02-11 20:06:41]

Will these only save the "master volume" and "mute" settings? I need something
to save all mixer settings, as most of mine default to 00 at boot.
If so, how do I apply your patches?
Thanks. I'm new at this. I just like the idea of Chrome-os.

[davejcool, 2011-02-11 20:31:39]

Hi,

The only user adjustable audio settings on Chrome-os currently are volume
and mute, so these are getting saved the same way other user preferences for
a browser session are saved.

What other mixer settings are you looking to save?  Another method could be
to add an upstart script to save and restore all mixer settings, or if you
can do a bash shell script, take a look at the alsactl command, which if
used at shutdown and startup can save and restore all your settings.  If you
want to make default settings, see
http://codereview.chromium.org/6410072/which just added an upstart
script to set some initial mixer settings.

If you've sync'd to the latest Chromium and are building the chrome-os
version of Chromium from local source, you should have this change already.
 If you have an older Chromium checkout, then download the patch set for
this change to a file, and do a 'patch -p1 < patchfile' while in the /src
folder.  If your checkout isn't too old, it might work.

-David.

On Fri, Feb 11, 2011 at 12:06 PM, <NEHoban@gmail.com> wrote:

> Will these only save the "master volume" and "mute" settings? I need
> something
> to save all mixer settings, as most of mine default to 00 at boot.
> If so, how do I apply your patches?
> Thanks. I'm new at this. I just like the idea of Chrome-os.
>
> http://codereview.chromium.org/6118006/
>