From be37402421167ec3721aa7f6fd1103851475517b Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 14 Jul 2025 17:39:33 +0200 Subject: [PATCH] Expose possible mixer opening errors (#1488) * playback: handle errors when opening mixer * chore: update CHANGELOG.md * fix tests and typo --- CHANGELOG.md | 2 ++ connect/README.md | 2 +- examples/play_connect.rs | 2 +- playback/src/mixer/alsamixer.rs | 47 +++++++++++++++++++++++++-------- playback/src/mixer/mod.rs | 14 +++++----- playback/src/mixer/softmixer.rs | 14 +++++----- src/main.rs | 29 +++++++++++--------- 7 files changed, 71 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cca28afe..fdac644d 100644 --- a/CHANGELOG.md +++ b/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` instead of `Self` (breaking) +- [playback] Changed type alias `MixerFn` to return `Result, Error>` instead of `Arc` (breaking) ### Added diff --git a/connect/README.md b/connect/README.md index 127474d7..015cce3d 100644 --- a/connect/README.md +++ b/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(()) diff --git a/examples/play_connect.rs b/examples/play_connect.rs index 26e52022..bd57df7d 100644 --- a/examples/play_connect.rs +++ b/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, diff --git a/playback/src/mixer/alsamixer.rs b/playback/src/mixer/alsamixer.rs index 52be1085..33ad64c4 100644 --- a/playback/src/mixer/alsamixer.rs +++ b/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 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 { 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 { diff --git a/playback/src/mixer/mod.rs b/playback/src/mixer/mod.rs index 83d00853..86252217 100644 --- a/playback/src/mixer/mod.rs +++ b/playback/src/mixer/mod.rs @@ -1,6 +1,6 @@ -use std::sync::Arc; - use crate::config::VolumeCtrl; +use librespot_core::Error; +use std::sync::Arc; pub mod mappings; use self::mappings::MappedCtrl; @@ -8,12 +8,12 @@ use self::mappings::MappedCtrl; pub struct NoOpVolume; pub trait Mixer: Send + Sync { - fn open(config: MixerConfig) -> Self + fn open(config: MixerConfig) -> Result 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 { Box::new(NoOpVolume) @@ -57,10 +57,10 @@ impl Default for MixerConfig { } } -pub type MixerFn = fn(MixerConfig) -> Arc; +pub type MixerFn = fn(MixerConfig) -> Result, Error>; -fn mk_sink(config: MixerConfig) -> Arc { - Arc::new(M::open(config)) +fn mk_sink(config: MixerConfig) -> Result, Error> { + Ok(Arc::new(M::open(config)?)) } pub const MIXERS: &[(&str, MixerFn)] = &[ diff --git a/playback/src/mixer/softmixer.rs b/playback/src/mixer/softmixer.rs index 6d32edc4..bf5e26ed 100644 --- a/playback/src/mixer/softmixer.rs +++ b/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 { 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 { diff --git a/src/main.rs b/src/main.rs index 85755658..38f8391a 100644 --- a/src/main.rs +++ b/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();