Add ALSA support to volume keys

chrome/browser/chromeos/audio_mixer_alsa.cc

+// Copyright (c) 2010 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 "chrome/browser/chromeos/audio_mixer_alsa.h"
+
+#include "base/logging.h"
+#include "base/task.h"
+
+namespace chromeos {
+
+// Connect to the ALSA mixer using their simple element API.  Init is performed
+// asynchronously on the worker thread.

[scherkus (not reviewing), 2010/12/22 22:31:58]

I think it'd be good to expand this to explain the relationship between master
and PCM

looking at the code it looks like "volume = master + PCM" but I'm not completely
sure

+
+// TODO(davej): Serialize volume/mute to preserve settings when restarting.
+
+typedef long alsa_long_t;  // 'long' is required for ALSA API calls.
+
+namespace {
+  const char* kMasterVolume = {"Master"};

[scherkus (not reviewing), 2010/12/22 22:31:58]

nits:
 - no need to have this indented
 - also do you need the { } ?  typically do without

[davejcool, 2010/12/23 03:09:02]

:-)  Microsoft + 1990's "C" = {"Funny Braces"}

+  const char* kPCMVolume = {"PCM"};
+}  // namespace
+
+AudioMixerAlsa::AudioMixerAlsa()
+    : min_volume_(-90),

[zhurunz, 2010/12/22 22:21:26]

Define a const for this.

[scherkus (not reviewing), 2010/12/22 22:31:58]

magic number alert!  do we have a constant for this somewhere?

[davejcool, 2010/12/23 03:09:02]

We do now!

+      max_volume_(0),
+      save_volume_(0),
+      mixer_state_lock_(),

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: no need to list variables that have default constructors

+      mixer_state_(UNINITIALIZED),
+      alsa_mixer_(NULL),
+      elem_master_(NULL),
+      elem_pcm_(NULL),
+      thread_(NULL) {

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: no need to list variables that have default constructors (scoped_ptr will
initialize to NULL)

+}
+
+AudioMixerAlsa::~AudioMixerAlsa() {
+  {
+    AutoLock lock(mixer_state_lock_);
+    mixer_state_ = SHUTTING_DOWN;
+    FreeAlsaMixer();
+  }
+
+  if (thread_ != NULL) {
+    thread_->Stop();
+    thread_.reset();
+  }
+
+  {
+      AutoLock lock(mixer_state_lock_);

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: over indented

do we need to lock + update state if the object is going away after this
function exits?

+      mixer_state_ = UNINITIALIZED;
+  }
+}
+
+bool AudioMixerAlsa::Init(InitDoneCallback* callback) {
+  if (!InitThread())
+    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,

[scherkus (not reviewing), 2010/12/22 22:31:58]

we only seem to use this thread for doing initialization... you don't have to do
the change now but perhaps there's an existing thread we can use to do
initialization instead of spinning one up and then not using it

[davejcool, 2010/12/23 03:09:02]

I just wanted the init to happen away from everything else (not doing it on the
UI or IO threads)... but if putting it on say the IO thread is OK, that would
simplify things.  Or how about killing the thread once initialization completes?

+    NewRunnableMethod(this, &AudioMixerAlsa::DoInit, callback));

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: indent by extra 2 spaces

+  return true;
+}
+
+bool AudioMixerAlsa::InitSync() {
+  if (!InitThread())
+    return false;
+  return InitializeAlsaMixer();
+}
+
+double AudioMixerAlsa::GetVolumeDb() const {
+  if (mixer_state_ != READY)
+    return kSilenceDb;
+
+  float vol_total = 0;
+  GetElementVolume(elem_master_, &vol_total);
+
+  float vol_pcm = 0;
+  if (elem_pcm_ && (GetElementVolume(elem_pcm_, &vol_pcm)))
+    vol_total += vol_pcm;
+
+  return vol_total;
+}
+
+void AudioMixerAlsa::GetVolumeLimits(double* vol_min, double* vol_max) {
+  if (!MixerReady())
+    return;
+  if (vol_min)
+    *vol_min = min_volume_;
+  if (vol_max)
+    *vol_max = max_volume_;
+}
+
+void AudioMixerAlsa::SetVolumeDb(double vol_db) {
+  if (!MixerReady())
+    return;
+  float actual_vol = 0;
+
+  // If a PCM volume slider exists, then first set the Master volume to the
+  // nearest volume >= requested volume, then adjust PCM volume down to get
+  // closer to the requested volume.  This allows for going to a quieter volume
+  // than the Master alone would allow, as well as give a finer resolution
+  // to the overall volume.
+
+  if (elem_pcm_) {
+    SetElementVolume(elem_master_, vol_db, &actual_vol, 0.9999f);
+    SetElementVolume(elem_pcm_, vol_db - actual_vol, NULL, 0.5f);
+  } else {
+    SetElementVolume(elem_master_, vol_db, NULL, 0.5f);
+  }
+}
+
+bool AudioMixerAlsa::IsMute() const {
+  if (!MixerReady())
+    return false;
+  int switched = 0;
+  GetElementSwitched(elem_master_, &switched);
+  return (switched) ? false : true;

[scherkus (not reviewing), 2010/12/22 22:31:58]

instead of having callees do the int<->bool conversion I'd suggest doing it
inside Set/GetElementSwitched() to keep callees looking cleaner

[davejcool, 2010/12/23 03:09:02]

Yup, removing the 'switched' language from Get/SetElement simplified this up as
well.

+}
+
+void AudioMixerAlsa::SetMute(bool mute) {
+  if (!MixerReady())
+    return;
+  int new_value = mute ? 0 : 1;

[scherkus (not reviewing), 2010/12/22 22:31:58]

ditto

+
+  // 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.
+
+  int old_value;
+  GetElementSwitched(elem_master_, &old_value);
+
+  if (old_value != new_value) {
+    if (!new_value) {
+      save_volume_ = GetVolumeDb();
+      SetVolumeDb(min_volume_);
+    } else {
+      SetVolumeDb(save_volume_);
+    }
+  }
+
+  SetElementSwitched(elem_master_, new_value);
+  if (elem_pcm_)
+    SetElementSwitched(elem_pcm_, new_value);
+}
+
+AudioMixerBase::State AudioMixerAlsa::CheckState() const {
+  AutoLock lock(mixer_state_lock_);
+  // If we think it's ready, verify it is actually so.
+  if ((mixer_state_ == READY) &&

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: condition can fit on one line

+      (alsa_mixer_ == NULL))

[zhurunz, 2010/12/22 22:21:26]

can be in one line.

+    mixer_state_ = IN_ERROR;
+  return mixer_state_;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private functions follow
+
+void AudioMixerAlsa::DoInit(InitDoneCallback* callback) {
+  bool success = InitializeAlsaMixer();
+
+  if (callback) {
+    callback->Run(success);
+    delete callback;
+  }
+}
+
+bool AudioMixerAlsa::InitThread() {
+  AutoLock lock(mixer_state_lock_);
+
+  if (mixer_state_ != UNINITIALIZED)
+    return false;
+
+  if (thread_ == NULL) {
+    thread_.reset(new base::Thread("AudioMixerAlsa"));
+    if (!thread_->Start()) {
+      thread_.reset();
+      return false;
+    }
+  }
+
+  mixer_state_ = INITIALIZING;
+  return true;
+}
+
+bool AudioMixerAlsa::InitializeAlsaMixer() {
+  AutoLock lock(mixer_state_lock_);

[scherkus (not reviewing), 2010/12/22 22:31:58]

sanity check: did you intend to lock for this entire function?

[davejcool, 2010/12/23 03:09:02]

In this case I did intend this.  The initialization is much more straightforward
than with Pulse, not to mention a lot faster (I timed it at 0.002 ms!)

+  if (mixer_state_ != INITIALIZING)
+    return false;
+
+  // ALSA hardware devices start at 0

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: end comment w/ period

+  int device_id_ = -1;
+  if ((snd_card_next(&device_id_) < 0) || (device_id_ < 0)) {

[scherkus (not reviewing), 2010/12/22 22:31:58]

so this selects the first ALSA device found on the machine?

[davejcool, 2010/12/23 03:09:02]

Oops... this was left over from earlier testing.  We're just using the default
sound device now.

+    LOG(ERROR) << "No ALSA Soundcards Found";
+    return false;
+  }
+
+  int err;
+  snd_mixer_t* handle = NULL;

[scherkus (not reviewing), 2010/12/22 22:31:58]

you may want to consider defining a scoped_ptr_malloc that calls
snd_mixer_close() when setting the ptr goes out of scope

[davejcool, 2010/12/23 03:09:02]

Thanks, I'll have learn more about that one.

+  char card[16];
+
+  snprintf(card, sizeof(card), "hw:%i", device_id_);

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: use base/stringprintf.h, std::string and c_str() instead

+
+  if ((err = snd_mixer_open(&handle, 0)) < 0) {
+    LOG(ERROR) << "ALSA mixer " << card << " open error: " << snd_strerror(err);
+    return NULL;
+  }

[zhurunz, 2010/12/22 22:21:26]

return false.
Same below.

+
+  if ((err = snd_mixer_attach(handle, card)) < 0) {
+    LOG(ERROR) << "ALSA Attach to card " << card << " failed: "
+               << snd_strerror(err);
+    snd_mixer_close(handle);
+    return NULL;
+  }
+
+  if ((err = snd_mixer_selem_register(handle, NULL, NULL)) < 0) {
+    LOG(ERROR) << "ALSA mixer register error: " << snd_strerror(err);
+    snd_mixer_close(handle);
+    return NULL;
+  }
+
+  err = snd_mixer_load(handle);
+  if (err < 0) {

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: for consistency might as well put snd_mixer_load(handle) inside the
condition

+    LOG(ERROR) << "ALSA mixer " << card << " load error: %s"
+               << snd_strerror(err);
+    snd_mixer_close(handle);
+    return NULL;
+  }
+
+  VLOG(1) << "Opened ALSA mixer " << card << " OK";
+
+  elem_master_ = FindElementWithName(handle, kMasterVolume);
+  if (elem_master_) {
+    alsa_long_t long_lo, long_hi;
+    snd_mixer_selem_get_playback_dB_range(elem_master_, &long_lo, &long_hi);
+    min_volume_ = static_cast<double>(long_lo) / 100.0;
+    max_volume_ = static_cast<double>(long_hi) / 100.0;
+  }
+
+  elem_pcm_ = FindElementWithName(handle, kPCMVolume);
+  if (elem_pcm_) {
+    alsa_long_t long_lo, long_hi;
+    snd_mixer_selem_get_playback_dB_range(elem_pcm_, &long_lo, &long_hi);
+    min_volume_ += static_cast<double>(long_lo) / 100.0;
+    max_volume_ += static_cast<double>(long_hi) / 100.0;
+  }
+
+  VLOG(1) << "ALSA volume range is " << min_volume_ << " dB to "
+          << max_volume_ << " dB";
+
+  alsa_mixer_ = handle;
+  mixer_state_ = READY;
+  return true;
+}
+
+void AudioMixerAlsa::FreeAlsaMixer() {
+  if (alsa_mixer_) {
+    snd_mixer_close(alsa_mixer_);
+    alsa_mixer_ = NULL;
+  }
+}
+
+snd_mixer_elem_t* AudioMixerAlsa::FindElementWithName(snd_mixer_t* handle,

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: these two arguments should be dropped to next line + indented by 4 spaces

+                                      const char* element_name) const {
+  snd_mixer_selem_id_t* sid;
+  snd_mixer_selem_id_alloca(&sid);
+  snd_mixer_selem_id_set_index(sid, 0);
+  snd_mixer_selem_id_set_name(sid, element_name);
+  snd_mixer_elem_t* elem = snd_mixer_find_selem(handle, sid);
+  if (!elem) {
+    LOG(ERROR) << "ALSA unable to find simple control "
+               << snd_mixer_selem_id_get_name(sid);
+  }
+  return elem;
+}
+
+bool AudioMixerAlsa::GetElementVolume(snd_mixer_elem_t* elem,
+                                      float* current_vol) const {
+  alsa_long_t long_vol;
+  snd_mixer_selem_get_playback_dB(elem, (snd_mixer_selem_channel_id_t)0,

[scherkus (not reviewing), 2010/12/22 22:31:58]

no c-style casts

+                                  &long_vol);
+  *current_vol = static_cast<float>(long_vol) / 100.0f;
+
+  return true;
+}
+
+bool AudioMixerAlsa::SetElementVolume(snd_mixer_elem_t* elem,
+                                      float new_vol,
+                                      float* actual_vol,
+                                      float rounding_bias) {
+  alsa_long_t vol_lo, vol_hi;
+  alsa_long_t db_lo_int, db_hi_int;

[zhurunz, 2010/12/22 22:21:26]

Better initialize them just in case those snd_xxx api failed.

+  snd_mixer_selem_get_playback_volume_range(elem, &vol_lo, &vol_hi);
+  snd_mixer_selem_get_playback_dB_range(elem, &db_lo_int, &db_hi_int);
+  float db_lo = static_cast<float>(db_lo_int) / 100.0f;
+  float db_hi = static_cast<float>(db_hi_int) / 100.0f;
+  float db_step = static_cast<float>(db_hi - db_lo) / (vol_hi - vol_lo);
+

[zhurunz, 2010/12/22 22:21:26]

check vol_hi != vol_lo

+  if (new_vol < db_lo)
+    new_vol = db_lo;
+
+  alsa_long_t value = static_cast<alsa_long_t>(rounding_bias +
+      (new_vol - db_lo) / db_step) + vol_lo;
+  snd_mixer_selem_set_playback_volume_all(elem, value);
+
+  VLOG(1) << "Set volume " << snd_mixer_selem_get_name(elem)
+          << " to " << new_vol << " ==> " << (value - vol_lo) * db_step + db_lo
+          << " dB";
+
+  if (actual_vol) {
+    alsa_long_t volume;
+    snd_mixer_selem_get_playback_volume(elem, (snd_mixer_selem_channel_id_t)0,
+                                        &volume);
+    *actual_vol = db_lo + (volume - vol_lo) * db_step;
+
+    VLOG(1) << "Actual volume " << snd_mixer_selem_get_name(elem)
+            << " now " << *actual_vol << " dB";
+  }
+  return true;
+}
+
+bool AudioMixerAlsa::GetElementSwitched(snd_mixer_elem_t* elem,

[scherkus (not reviewing), 2010/12/22 22:31:58]

this function always return true... maybe it should return the switch value
itself?

+                                        int * switched) const {

[zhurunz, 2010/12/22 22:21:26]

space after *

+  snd_mixer_selem_get_playback_switch(elem, (snd_mixer_selem_channel_id_t)0,

[zhurunz, 2010/12/22 22:21:26]

use static_cast to be consistent.

+                                      switched);
+  return true;
+}
+
+bool AudioMixerAlsa::SetElementSwitched(snd_mixer_elem_t* elem,

[scherkus (not reviewing), 2010/12/22 22:31:58]

this function always return true.. perhaps it should be void

[davejcool, 2010/12/23 03:09:02]

Ahh, of course!  In a previous incarnation the function could fail if the
element wasn't found, but now it's passed in.

+                                        int enabled) {
+  snd_mixer_selem_set_playback_switch_all(elem, enabled);
+
+  VLOG(1) << "Set playback switch" << snd_mixer_selem_get_name(elem)

[zhurunz, 2010/12/22 22:21:26]

space here after ' switch'?

+          << " to " << enabled;
+  return true;
+}
+
+bool AudioMixerAlsa::MixerReady() const {
+    AutoLock lock(mixer_state_lock_);

[scherkus (not reviewing), 2010/12/22 22:31:58]

nit: over indented

+    return (mixer_state_ == READY);
+}
+
+}  // namespace chromeos
+