Expose possible mixer opening errors (#1488)

* playback: handle errors when opening mixer

* chore: update CHANGELOG.md

* fix tests and typo

Author: photovoltex

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [connect] Moved all public items to the highest level (breaking)
 - [connect] Replaced Mercury usage in `Spirc` with Dealer
 - [metadata] Replaced `AudioFileFormat` with own enum. (breaking)
+- [playback] Changed trait `Mixer::open` to return `Result<Self, Error>` instead of `Self` (breaking)
+- [playback] Changed type alias `MixerFn` to return `Result<Arc<dyn Mixer>, Error>` instead of `Arc<dyn Mixer>` (breaking)
 
 ### Added
 

connect/README.md

@@ -55,7 +55,7 @@ async fn create_basic_spirc() -> Result<(), Error> {
         session,
         credentials,
         player,
-        mixer(MixerConfig::default())
+        mixer(MixerConfig::default())?
     ).await?;
 
     Ok(())

examples/play_connect.rs

@@ -50,7 +50,7 @@ async fn main() -> Result<(), Error> {
         })?;
 
     let session = Session::new(session_config, Some(cache));
-    let mixer = mixer_builder(mixer_config);
+    let mixer = mixer_builder(mixer_config)?;
 
     let player = Player::new(
         player_config,

playback/src/mixer/alsamixer.rs

@@ -5,9 +5,12 @@ use super::{Mixer, MixerConfig, VolumeCtrl};
 
 use alsa::ctl::{ElemId, ElemIface};
 use alsa::mixer::{MilliBel, SelemChannelId, SelemId};
+use alsa::Error as AlsaError;
 use alsa::{Ctl, Round};
 
-use std::ffi::CString;
+use librespot_core::Error;
+use std::ffi::{CString, NulError};
+use thiserror::Error;
 
 #[derive(Clone)]
 #[allow(dead_code)]
@@ -29,8 +32,30 @@ pub struct AlsaMixer {
 const SND_CTL_TLV_DB_GAIN_MUTE: MilliBel = MilliBel(-9999999);
 const ZERO_DB: MilliBel = MilliBel(0);
 
+#[derive(Debug, Error)]
+enum AlsaMixerError {
+    #[error("Could not open Alsa mixer. {0}")]
+    CouldNotOpen(AlsaError),
+    #[error("Could not find Alsa mixer control")]
+    CouldNotFindController,
+    #[error("Could not open Alsa softvol with that device. {0}")]
+    CouldNotOpenWithDevice(AlsaError),
+    #[error("Could not open Alsa softvol with that name. {0}")]
+    CouldNotOpenWithName(NulError),
+    #[error("Could not get Alsa softvol dB range. {0}")]
+    NoDbRange(AlsaError),
+    #[error("Could not convert Alsa raw volume to dB volume. {0}")]
+    CouldNotConvertRaw(AlsaError),
+}
+
+impl From<AlsaMixerError> for Error {
+    fn from(value: AlsaMixerError) -> Self {
+        Error::failed_precondition(value)
+    }
+}
+
 impl Mixer for AlsaMixer {
-    fn open(config: MixerConfig) -> Self {
+    fn open(config: MixerConfig) -> Result<Self, Error> {
         info!(
             "Mixing with Alsa and volume control: {:?} for device: {} with mixer control: {},{}",
             config.volume_ctrl, config.device, config.control, config.index,
@@ -39,10 +64,10 @@ impl Mixer for AlsaMixer {
         let mut config = config; // clone
 
         let mixer =
-            alsa::mixer::Mixer::new(&config.device, false).expect("Could not open Alsa mixer");
+            alsa::mixer::Mixer::new(&config.device, false).map_err(AlsaMixerError::CouldNotOpen)?;
         let simple_element = mixer
             .find_selem(&SelemId::new(&config.control, config.index))
-            .expect("Could not find Alsa mixer control");
+            .ok_or(AlsaMixerError::CouldNotFindController)?;
 
         // Query capabilities
         let has_switch = simple_element.has_playback_switch();
@@ -57,17 +82,17 @@ impl Mixer for AlsaMixer {
         // Query dB volume range -- note that Alsa exposes a different
         // API for hardware and software mixers
         let (min_millibel, max_millibel) = if is_softvol {
-            let control = Ctl::new(&config.device, false)
-                .expect("Could not open Alsa softvol with that device");
+            let control =
+                Ctl::new(&config.device, false).map_err(AlsaMixerError::CouldNotOpenWithDevice)?;
             let mut element_id = ElemId::new(ElemIface::Mixer);
             element_id.set_name(
                 &CString::new(config.control.as_str())
-                    .expect("Could not open Alsa softvol with that name"),
+                    .map_err(AlsaMixerError::CouldNotOpenWithName)?,
             );
             element_id.set_index(config.index);
             let (min_millibel, mut max_millibel) = control
                 .get_db_range(&element_id)
-                .expect("Could not get Alsa softvol dB range");
+                .map_err(AlsaMixerError::NoDbRange)?;
 
             // Alsa can report incorrect maximum volumes due to rounding
             // errors. e.g. Alsa rounds [-60.0..0.0] in range [0..255] to
@@ -97,7 +122,7 @@ impl Mixer for AlsaMixer {
                 debug!("Alsa mixer reported minimum dB as mute, trying workaround");
                 min_millibel = simple_element
                     .ask_playback_vol_db(min + 1)
-                    .expect("Could not convert Alsa raw volume to dB volume");
+                    .map_err(AlsaMixerError::CouldNotConvertRaw)?;
             }
             (min_millibel, max_millibel)
         };
@@ -150,7 +175,7 @@ impl Mixer for AlsaMixer {
         );
         debug!("Alsa forcing linear dB mapping: {}", use_linear_in_db);
 
-        Self {
+        Ok(Self {
             config,
             min,
             max,
@@ -161,7 +186,7 @@ impl Mixer for AlsaMixer {
             has_switch,
             is_softvol,
             use_linear_in_db,
-        }
+        })
     }
 
     fn volume(&self) -> u16 {

playback/src/mixer/mod.rs

@@ -1,19 +1,19 @@
-use std::sync::Arc;
-
 use crate::config::VolumeCtrl;
+use librespot_core::Error;
+use std::sync::Arc;
 
 pub mod mappings;
 use self::mappings::MappedCtrl;
 
 pub struct NoOpVolume;
 
 pub trait Mixer: Send + Sync {
-    fn open(config: MixerConfig) -> Self
+    fn open(config: MixerConfig) -> Result<Self, Error>
     where
         Self: Sized;
 
-    fn set_volume(&self, volume: u16);
     fn volume(&self) -> u16;
+    fn set_volume(&self, volume: u16);
 
     fn get_soft_volume(&self) -> Box<dyn VolumeGetter + Send> {
         Box::new(NoOpVolume)
@@ -57,10 +57,10 @@ impl Default for MixerConfig {
     }
 }
 
-pub type MixerFn = fn(MixerConfig) -> Arc<dyn Mixer>;
+pub type MixerFn = fn(MixerConfig) -> Result<Arc<dyn Mixer>, Error>;
 
-fn mk_sink<M: Mixer + 'static>(config: MixerConfig) -> Arc<dyn Mixer> {
-    Arc::new(M::open(config))
+fn mk_sink<M: Mixer + 'static>(config: MixerConfig) -> Result<Arc<dyn Mixer>, Error> {
+    Ok(Arc::new(M::open(config)?))
 }
 
 pub const MIXERS: &[(&str, MixerFn)] = &[

playback/src/mixer/softmixer.rs

@@ -1,10 +1,10 @@
-use portable_atomic::AtomicU64;
-use std::sync::atomic::Ordering;
-use std::sync::Arc;
-
 use super::VolumeGetter;
 use super::{MappedCtrl, VolumeCtrl};
 use super::{Mixer, MixerConfig};
+use librespot_core::Error;
+use portable_atomic::AtomicU64;
+use std::sync::atomic::Ordering;
+use std::sync::Arc;
 
 #[derive(Clone)]
 pub struct SoftMixer {
@@ -15,14 +15,14 @@ pub struct SoftMixer {
 }
 
 impl Mixer for SoftMixer {
-    fn open(config: MixerConfig) -> Self {
+    fn open(config: MixerConfig) -> Result<Self, Error> {
         let volume_ctrl = config.volume_ctrl;
         info!("Mixing with softvol and volume control: {:?}", volume_ctrl);
 
-        Self {
+        Ok(Self {
             volume: Arc::new(AtomicU64::new(f64::to_bits(0.5))),
             volume_ctrl,
-        }
+        })
     }
 
     fn volume(&self) -> u16 {

src/main.rs

@@ -1,14 +1,3 @@
-use std::{
-    env,
-    fs::create_dir_all,
-    ops::RangeInclusive,
-    path::{Path, PathBuf},
-    pin::Pin,
-    process::exit,
-    str::FromStr,
-    time::{Duration, Instant},
-};
-
 use data_encoding::HEXLOWER;
 use futures_util::StreamExt;
 #[cfg(feature = "alsa-backend")]
@@ -33,6 +22,16 @@ use librespot::{
 use librespot_oauth::OAuthClientBuilder;
 use log::{debug, error, info, trace, warn};
 use sha1::{Digest, Sha1};
+use std::{
+    env,
+    fs::create_dir_all,
+    ops::RangeInclusive,
+    path::{Path, PathBuf},
+    pin::Pin,
+    process::exit,
+    str::FromStr,
+    time::{Duration, Instant},
+};
 use sysinfo::{ProcessesToUpdate, System};
 use thiserror::Error;
 use url::Url;
@@ -1943,7 +1942,13 @@ async fn main() {
     }
 
     let mixer_config = setup.mixer_config.clone();
-    let mixer = (setup.mixer)(mixer_config);
+    let mixer = match (setup.mixer)(mixer_config) {
+        Ok(mixer) => mixer,
+        Err(why) => {
+            error!("{why}");
+            exit(1)
+        }
+    };
     let player_config = setup.player_config.clone();
 
     let soft_volume = mixer.get_soft_volume();