alsa mixer: make volume computation more robust

Author: eladyn

src/alsa_mixer.rs

@@ -1,30 +1,38 @@
+use color_eyre::eyre::{self, Context, eyre};
 use librespot_playback::mixer::{Mixer, MixerConfig};
 use log::error;
-use std::error::Error;
+use std::sync::{Arc, Mutex, MutexGuard};
 
-#[derive(Clone)]
 pub struct AlsaMixer {
-    pub device: String,
-    pub mixer: String,
-    pub linear_scaling: bool,
+    pub mixer: Arc<Mutex<alsa::Mixer>>,
+    pub config: MixerConfig,
 }
 
 impl AlsaMixer {
-    fn set_volume_with_err(&self, volume: u16) -> Result<(), Box<dyn Error>> {
-        let mixer = alsa::mixer::Mixer::new(&self.device, false)?;
-
-        let selem_id = alsa::mixer::SelemId::new(&self.mixer, 0);
-        let elem = mixer.find_selem(&selem_id).ok_or_else(|| {
-            format!(
-                "Couldn't find selem with name '{}'.",
-                selem_id.get_name().unwrap_or("unnamed")
+    fn get_selem<'a>(
+        &'a self,
+        lock: &'a MutexGuard<'a, alsa::Mixer>,
+    ) -> eyre::Result<alsa::mixer::Selem<'a>> {
+        let selem_id = alsa::mixer::SelemId::new(&self.config.control, self.config.index);
+        let selem = lock.find_selem(&selem_id).ok_or_else(|| {
+            eyre!(
+                "No control with name '{}' in alsa device '{}'",
+                self.config.control,
+                self.config.device,
             )
         })?;
-
+        Ok(selem)
+    }
+    fn set_volume_with_err(&self, volume: u16) -> eyre::Result<()> {
+        let lock = self.mixer.lock().expect("lock shouldn't be poisoned");
+        let elem = self.get_selem(&lock)?;
         let (min, max) = elem.get_playback_volume_range();
 
         let volume_steps = (max - min) as f64;
-        let normalised_volume = if self.linear_scaling {
+        let normalised_volume = if matches!(
+            self.config.volume_ctrl,
+            librespot_playback::config::VolumeCtrl::Linear
+        ) {
             (((volume as f64) / (u16::MAX as f64)) * volume_steps) as i64 + min
         } else {
             ((volume as f64 + 1.0).log((u16::MAX as f64) + 1.0) * volume_steps).floor() as i64 + min
@@ -33,39 +41,41 @@ impl AlsaMixer {
         elem.set_playback_volume_all(normalised_volume)?;
         Ok(())
     }
+
+    fn get_volume_with_err(&self) -> eyre::Result<u16> {
+        let lock = self.mixer.lock().expect("lock shouldn't be poisoned");
+        let elem = self.get_selem(&lock)?;
+        let (min, max) = elem.get_playback_volume_range();
+        let volume_steps = (max - min) as f64;
+        let vol = elem.get_playback_volume(alsa::mixer::SelemChannelId::mono())?;
+        let normalized_volume = if matches!(
+            self.config.volume_ctrl,
+            librespot_playback::config::VolumeCtrl::Linear
+        ) {
+            ((vol - min) as f64 * u16::MAX as f64 / volume_steps).floor() as u16
+        } else {
+            ((u16::MAX as f64 + 1.0).powf(((vol - min) as f64) / volume_steps) - 1.0).floor() as u16
+        };
+        Ok(normalized_volume)
+    }
 }
 
 impl Mixer for AlsaMixer {
-    fn open(_: MixerConfig) -> Result<AlsaMixer, librespot_core::Error> {
+    fn open(config: MixerConfig) -> Result<AlsaMixer, librespot_core::Error> {
+        let mixer = alsa::Mixer::new(&config.device, false)
+            .wrap_err("failed to open mixer")
+            .map_err(librespot_core::Error::invalid_argument)?;
         Ok(AlsaMixer {
-            device: "default".to_string(),
-            mixer: "Master".to_string(),
-            linear_scaling: false,
+            mixer: Arc::new(Mutex::new(mixer)),
+            config,
         })
     }
 
     fn volume(&self) -> u16 {
-        let selem_id = alsa::mixer::SelemId::new(&self.mixer, 0);
-        let mixer = alsa::mixer::Mixer::new(&self.device, false).ok();
-        let vol = mixer
-            .as_ref()
-            .and_then(|mixer| mixer.find_selem(&selem_id))
-            .and_then(|elem| {
-                let (min, max) = elem.get_playback_volume_range();
-                elem.get_playback_volume(alsa::mixer::SelemChannelId::mono())
-                    .ok()
-                    .map(|volume| {
-                        (((volume - min) as f64 / (max - min) as f64) * (u16::MAX as f64)).floor()
-                            as u16
-                    })
-            });
-        match vol {
-            Some(vol) => vol,
-            None => {
-                error!(
-                    "Couldn't read volume from alsa device with name \"{}\".",
-                    self.device
-                );
+        match self.get_volume_with_err() {
+            Ok(vol) => vol,
+            Err(err) => {
+                error!("failed to get volume from alsa device: {err:?}");
                 0
             }
         }
@@ -74,7 +84,7 @@ impl Mixer for AlsaMixer {
     fn set_volume(&self, volume: u16) {
         match self.set_volume_with_err(volume) {
             Ok(_) => (),
-            Err(e) => error!("Couldn't set volume: {:?}", e),
+            Err(err) => error!("Couldn't set volume of alsa device: {err:?}"),
         }
     }
 }

src/setup.rs

@@ -25,22 +25,35 @@ pub(crate) fn initial_state(
             }
             #[cfg(feature = "alsa_backend")]
             config::VolumeController::Alsa | config::VolumeController::AlsaLinear => {
-                let audio_device = config.audio_device.clone();
-                let control_device = config.alsa_config.control.clone();
-                let mixer = config.alsa_config.mixer.clone();
+                let device = config
+                    .alsa_config
+                    .mixer
+                    .as_deref()
+                    .or(config.audio_device.as_deref())
+                    .unwrap_or("default")
+                    .to_string();
+                let control = config
+                    .alsa_config
+                    .control
+                    .as_deref()
+                    .unwrap_or("Master")
+                    .to_string();
                 info!("Using alsa volume controller.");
-                let linear = matches!(
+                use librespot_playback::config::VolumeCtrl;
+                let volume_ctrl = if matches!(
                     config.volume_controller,
                     config::VolumeController::AlsaLinear
-                );
-                Arc::new(alsa_mixer::AlsaMixer {
-                    device: control_device
-                        .clone()
-                        .or_else(|| audio_device.clone())
-                        .unwrap_or_else(|| "default".to_string()),
-                    mixer: mixer.clone().unwrap_or_else(|| "Master".to_string()),
-                    linear_scaling: linear,
-                })
+                ) {
+                    VolumeCtrl::Linear
+                } else {
+                    VolumeCtrl::Log(0.0) /* this value is ignored */
+                };
+                Arc::new(alsa_mixer::AlsaMixer::open(MixerConfig {
+                    device,
+                    control,
+                    index: 0,
+                    volume_ctrl,
+                })?)
             }
             _ => {
                 info!("Using software volume controller.");