From 8c480b7e39e911ddbfc4d564932d09dbdd7b6af6 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Sun, 5 Dec 2021 13:00:33 -0600 Subject: [PATCH 1/7] Fix Command line arguments incorrectly echoed in TRACE Fix up for #886 Closes: #898 And... * Don't silently ignore non-Unicode while parsing env vars. * Iterating over `std::env::args` will panic! on invalid unicode. Let's not do that. `getopts` will catch missing args and exit if those args are required after our error message about the arg not being valid unicode. * Gaurd against empty strings. There are a few places while parsing options strings that we don't immediately evaluate their validity let's at least makes sure that they are not empty if present. * `args` is only used in `get_setup` it doesn't need to be in main. * Nicer help header. * Get rid of `use std::io::{stderr, Write};` and just use `rpassword::prompt_password_stderr`. * Get rid of `get_credentials` it was clunky, ugly and only used once. There is no need for it to be a separate function. * Handle an empty password prompt and password prompt parsing errors. * + Other random misc clean ups. --- src/main.rs | 415 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 240 insertions(+), 175 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2dec56ae..789654ff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,7 +26,6 @@ mod player_event_handler; use player_event_handler::{emit_sink_event, run_program_on_events}; use std::env; -use std::io::{stderr, Write}; use std::ops::RangeInclusive; use std::path::Path; use std::pin::Pin; @@ -40,27 +39,16 @@ fn device_id(name: &str) -> String { } fn usage(program: &str, opts: &getopts::Options) -> String { - let brief = format!("Usage: {} [options]", program); + let repo_home = env!("CARGO_PKG_REPOSITORY"); + let desc = env!("CARGO_PKG_DESCRIPTION"); + let version = get_version_string(); + let brief = format!( + "{}\n\n{}\n\n{}\n\nUsage: {} []", + version, desc, repo_home, program + ); opts.usage(&brief) } -fn arg_to_var(arg: &str) -> String { - // To avoid name collisions environment variables must be prepended - // with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`. - format!("LIBRESPOT_{}", arg.to_uppercase().replace("-", "_")) -} - -fn env_var_present(arg: &str) -> bool { - env::var(arg_to_var(arg)).is_ok() -} - -fn env_var_opt_str(option: &str) -> Option { - match env::var(arg_to_var(option)) { - Ok(value) => Some(value), - Err(_) => None, - } -} - fn setup_logging(quiet: bool, verbose: bool) { let mut builder = env_logger::Builder::new(); match env::var("RUST_LOG") { @@ -102,29 +90,6 @@ fn list_backends() { } } -pub fn get_credentials Option>( - username: Option, - password: Option, - cached_credentials: Option, - prompt: F, -) -> Option { - if let Some(username) = username { - if let Some(password) = password { - return Some(Credentials::with_password(username, password)); - } - - match cached_credentials { - Some(credentials) if username == credentials.username => Some(credentials), - _ => { - let password = prompt(&username)?; - Some(Credentials::with_password(username, password)) - } - } - } else { - cached_credentials - } -} - #[derive(Debug, Error)] pub enum ParseFileSizeError { #[error("empty argument")] @@ -218,9 +183,10 @@ struct Setup { emit_sink_events: bool, } -fn get_setup(args: &[String]) -> Setup { - const VALID_NORMALISATION_KNEE_RANGE: RangeInclusive = 0.0..=2.0; +fn get_setup() -> Setup { + const VALID_INITIAL_VOLUME_RANGE: RangeInclusive = 0..=100; const VALID_VOLUME_RANGE: RangeInclusive = 0.0..=100.0; + const VALID_NORMALISATION_KNEE_RANGE: RangeInclusive = 0.0..=2.0; const VALID_NORMALISATION_PREGAIN_RANGE: RangeInclusive = -10.0..=10.0; const VALID_NORMALISATION_THRESHOLD_RANGE: RangeInclusive = -10.0..=0.0; const VALID_NORMALISATION_ATTACK_RANGE: RangeInclusive = 1..=500; @@ -596,25 +562,72 @@ fn get_setup(args: &[String]) -> Setup { "PORT", ); + let args: Vec<_> = std::env::args_os() + .filter_map(|s| match s.into_string() { + Ok(valid) => Some(valid), + Err(s) => { + eprintln!( + "Command line argument was not valid Unicode and will not be evaluated: {:?}", + s + ); + None + } + }) + .collect(); + let matches = match opts.parse(&args[1..]) { Ok(m) => m, - Err(f) => { - eprintln!( - "Error parsing command line options: {}\n{}", - f, - usage(&args[0], &opts) - ); + Err(e) => { + eprintln!("Error parsing command line options: {}", e); + println!("\n{}", usage(&args[0], &opts)); exit(1); } }; - let opt_present = |opt| matches.opt_present(opt) || env_var_present(opt); + let stripped_env_key = |k: &str| { + k.trim_start_matches("LIBRESPOT_") + .replace("_", "-") + .to_lowercase() + }; + + let env_vars: Vec<_> = env::vars_os().filter_map(|(k, v)| { + let mut env_var = None; + if let Ok(key) = k.into_string() { + if key.starts_with("LIBRESPOT_") { + let stripped_key = stripped_env_key(&key); + // Only match against long option/flag names. + // Something like LIBRESPOT_V for example is + // not valid because there are both -v and -V flags + // but env vars are assumed to be all uppercase. + let len = stripped_key.chars().count(); + if len > 1 && matches.opt_defined(&stripped_key) { + match v.into_string() { + Ok(value) => { + env_var = Some((key, value)); + }, + Err(s) => { + eprintln!("Environment variable was not valid Unicode and will not be evaluated: {}={:?}", key, s); + } + } + } + } + } + + env_var + }) + .collect(); + + let opt_present = + |opt| matches.opt_present(opt) || env_vars.iter().any(|(k, _)| stripped_env_key(k) == opt); let opt_str = |opt| { if matches.opt_present(opt) { matches.opt_str(opt) } else { - env_var_opt_str(opt) + env_vars + .iter() + .find(|(k, _)| stripped_env_key(k) == opt) + .map(|(_, v)| v.to_string()) } }; @@ -632,55 +645,43 @@ fn get_setup(args: &[String]) -> Setup { info!("{}", get_version_string()); - let librespot_env_vars: Vec = env::vars_os() - .filter_map(|(k, v)| { - let mut env_var = None; - if let Some(key) = k.to_str() { - if key.starts_with("LIBRESPOT_") { - if matches!(key, "LIBRESPOT_PASSWORD" | "LIBRESPOT_USERNAME") { - // Don't log creds. - env_var = Some(format!("\t\t{}=XXXXXXXX", key)); - } else if let Some(value) = v.to_str() { - env_var = Some(format!("\t\t{}={}", key, value)); - } - } - } - - env_var - }) - .collect(); - - if !librespot_env_vars.is_empty() { + if !env_vars.is_empty() { trace!("Environment variable(s):"); - for kv in librespot_env_vars { - trace!("{}", kv); + for (k, v) in &env_vars { + if matches!(k.as_str(), "LIBRESPOT_PASSWORD" | "LIBRESPOT_USERNAME") { + trace!("\t\t{}=\"XXXXXXXX\"", k); + } else if v.is_empty() { + trace!("\t\t{}=", k); + } else { + trace!("\t\t{}=\"{}\"", k, v); + } } } - let cmd_args = &args[1..]; + let args_len = args.len(); - let cmd_args_len = cmd_args.len(); - - if cmd_args_len > 0 { + if args_len > 1 { trace!("Command line argument(s):"); - for (index, key) in cmd_args.iter().enumerate() { - if key.starts_with('-') || key.starts_with("--") { - if matches!(key.as_str(), "--password" | "-p" | "--username" | "-u") { - // Don't log creds. - trace!("\t\t{} XXXXXXXX", key); - } else { - let mut value = "".to_string(); - let next = index + 1; - if next < cmd_args_len { - let next_key = cmd_args[next].clone(); - if !next_key.starts_with('-') && !next_key.starts_with("--") { - value = next_key; - } - } + for (index, key) in args.iter().enumerate() { + let opt = key.trim_start_matches('-'); - trace!("\t\t{} {}", key, value); + if index > 0 + && &args[index - 1] != key + && matches.opt_defined(opt) + && matches.opt_present(opt) + { + if matches!(opt, PASSWORD | PASSWORD_SHORT | USERNAME | USERNAME_SHORT) { + // Don't log creds. + trace!("\t\t{} \"XXXXXXXX\"", key); + } else { + let value = matches.opt_str(opt).unwrap_or_else(|| "".to_string()); + if value.is_empty() { + trace!("\t\t{}", key); + } else { + trace!("\t\t{} \"{}\"", key, value); + } } } } @@ -707,7 +708,7 @@ fn get_setup(args: &[String]) -> Setup { let backend = audio_backend::find(backend_name).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", BACKEND, BACKEND_SHORT, opt_str(BACKEND).unwrap_or_default() @@ -720,7 +721,10 @@ fn get_setup(args: &[String]) -> Setup { .as_deref() .map(|format| { AudioFormat::from_str(format).unwrap_or_else(|_| { - error!("Invalid `--{}` / `-{}`: {}", FORMAT, FORMAT_SHORT, format); + error!( + "Invalid `--{}` / `-{}`: \"{}\"", + FORMAT, FORMAT_SHORT, format + ); println!( "Valid `--{}` / `-{}` values: F64, F32, S32, S24, S24_3, S16", FORMAT, FORMAT_SHORT @@ -743,9 +747,17 @@ fn get_setup(args: &[String]) -> Setup { feature = "rodio-backend", feature = "portaudio-backend" ))] - if device == Some("?".into()) { - backend(device, format); - exit(0); + if let Some(ref value) = device { + if value == "?" { + backend(device, format); + exit(0); + } else if value.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + DEVICE, DEVICE_SHORT + ); + exit(1); + } } #[cfg(not(any( @@ -774,7 +786,7 @@ fn get_setup(args: &[String]) -> Setup { let mixer = mixer::find(mixer_type.as_deref()).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", MIXER_TYPE, MIXER_TYPE_SHORT, opt_str(MIXER_TYPE).unwrap_or_default() @@ -807,7 +819,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|index| { index.parse::().unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", ALSA_MIXER_INDEX, ALSA_MIXER_INDEX_SHORT, index ); println!("Default: {}", mixer_default_config.index); @@ -822,6 +834,15 @@ fn get_setup(args: &[String]) -> Setup { #[cfg(feature = "alsa-backend")] let control = opt_str(ALSA_MIXER_CONTROL).unwrap_or(mixer_default_config.control); + #[cfg(feature = "alsa-backend")] + if control.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + ALSA_MIXER_CONTROL, ALSA_MIXER_CONTROL_SHORT + ); + exit(1); + } + #[cfg(not(feature = "alsa-backend"))] let control = mixer_default_config.control; @@ -829,7 +850,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|range| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", VOLUME_RANGE, VOLUME_RANGE_SHORT, range ); println!( @@ -871,7 +892,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|volume_ctrl| { VolumeCtrl::from_str_with_range(volume_ctrl, volume_range).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", VOLUME_CTRL, VOLUME_CTRL_SHORT, volume_ctrl ); println!( @@ -918,7 +939,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|e| { e.unwrap_or_else(|e| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", CACHE_SIZE_LIMIT, CACHE_SIZE_LIMIT_SHORT, e ); exit(1); @@ -945,20 +966,57 @@ fn get_setup(args: &[String]) -> Setup { }; let credentials = { - let cached_credentials = cache.as_ref().and_then(Cache::credentials); + let cached_creds = cache.as_ref().and_then(Cache::credentials); - let password = |username: &String| -> Option { - write!(stderr(), "Password for {}: ", username).ok()?; - stderr().flush().ok()?; - rpassword::read_password().ok() - }; + if let Some(username) = opt_str(USERNAME) { + if username.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + USERNAME, USERNAME_SHORT + ); + exit(1); + } + if let Some(password) = opt_str(PASSWORD) { + if password.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + PASSWORD, PASSWORD_SHORT + ); + exit(1); + } + Some(Credentials::with_password(username, password)) + } else { + match cached_creds { + Some(creds) if username == creds.username => Some(creds), + _ => { + let prompt = &format!("Password for {}: ", username); - get_credentials( - opt_str(USERNAME), - opt_str(PASSWORD), - cached_credentials, - password, - ) + match rpassword::prompt_password_stderr(prompt) { + Ok(password) => { + if !password.is_empty() { + Some(Credentials::with_password(username, password)) + } else { + trace!("Password was empty."); + if cached_creds.is_some() { + trace!("Using cached credentials."); + } + cached_creds + } + } + Err(e) => { + warn!("Cannot parse password: {}", e); + if cached_creds.is_some() { + trace!("Using cached credentials."); + } + cached_creds + } + } + } + } + } + } else { + cached_creds + } }; let enable_discovery = !opt_present(DISABLE_DISCOVERY); @@ -980,12 +1038,14 @@ fn get_setup(args: &[String]) -> Setup { .map(|port| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", ZEROCONF_PORT, ZEROCONF_PORT_SHORT, port ); println!( - "Valid `--{}` / `-{}` values: 1 - 65535", - ZEROCONF_PORT, ZEROCONF_PORT_SHORT + "Valid `--{}` / `-{}` values: 1 - {}", + ZEROCONF_PORT, + ZEROCONF_PORT_SHORT, + u16::MAX ); }; @@ -1011,16 +1071,27 @@ fn get_setup(args: &[String]) -> Setup { let name = opt_str(NAME).unwrap_or_else(|| connect_default_config.name.clone()); + if name.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + NAME, NAME_SHORT + ); + exit(1); + } + let initial_volume = opt_str(INITIAL_VOLUME) .map(|initial_volume| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", INITIAL_VOLUME, INITIAL_VOLUME_SHORT, initial_volume ); println!( - "Valid `--{}` / `-{}` values: 0 - 100", - INITIAL_VOLUME, INITIAL_VOLUME_SHORT + "Valid `--{}` / `-{}` values: {} - {}", + INITIAL_VOLUME, + INITIAL_VOLUME_SHORT, + VALID_INITIAL_VOLUME_RANGE.start(), + VALID_INITIAL_VOLUME_RANGE.end() ); #[cfg(feature = "alsa-backend")] println!( @@ -1039,7 +1110,7 @@ fn get_setup(args: &[String]) -> Setup { exit(1); }); - if volume > 100 { + if !(VALID_INITIAL_VOLUME_RANGE).contains(&volume) { on_error(); exit(1); } @@ -1056,7 +1127,7 @@ fn get_setup(args: &[String]) -> Setup { .as_deref() .map(|device_type| { DeviceType::from_str(device_type).unwrap_or_else(|_| { - error!("Invalid `--{}` / `-{}`: {}", DEVICE_TYPE, DEVICE_TYPE_SHORT, device_type); + error!("Invalid `--{}` / `-{}`: \"{}\"", DEVICE_TYPE, DEVICE_TYPE_SHORT, device_type); println!("Valid `--{}` / `-{}` values: computer, tablet, smartphone, speaker, tv, avr, stb, audiodongle, \ gameconsole, castaudio, castvideo, automobile, smartwatch, chromebook, carthing, homething", DEVICE_TYPE, DEVICE_TYPE_SHORT @@ -1079,55 +1150,50 @@ fn get_setup(args: &[String]) -> Setup { } }; - let session_config = { - let device_id = device_id(&connect_config.name); - - SessionConfig { - user_agent: version::VERSION_STRING.to_string(), - device_id, - proxy: opt_str(PROXY).or_else(|| std::env::var("http_proxy").ok()).map( - |s| { - match Url::parse(&s) { - Ok(url) => { - if url.host().is_none() || url.port_or_known_default().is_none() { - error!("Invalid proxy url, only URLs on the format \"http://host:port\" are allowed"); - exit(1); - } - - if url.scheme() != "http" { - error!("Only unsecure http:// proxies are supported"); - exit(1); - } - - url - }, - Err(e) => { - error!("Invalid proxy URL: {}, only URLs in the format \"http://host:port\" are allowed", e); + let session_config = SessionConfig { + user_agent: version::VERSION_STRING.to_string(), + device_id: device_id(&connect_config.name), + proxy: opt_str(PROXY).or_else(|| std::env::var("http_proxy").ok()).map( + |s| { + match Url::parse(&s) { + Ok(url) => { + if url.host().is_none() || url.port_or_known_default().is_none() { + error!("Invalid proxy url, only URLs on the format \"http://host:port\" are allowed"); exit(1); } - } - }, - ), - ap_port: opt_str(AP_PORT) - .map(|port| { - let on_error = || { - error!("Invalid `--{}` / `-{}`: {}", AP_PORT, AP_PORT_SHORT, port); - println!("Valid `--{}` / `-{}` values: 1 - 65535", AP_PORT, AP_PORT_SHORT); - }; - let port = port.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); + if url.scheme() != "http" { + error!("Only unsecure http:// proxies are supported"); + exit(1); + } - if port == 0 { - on_error(); + url + }, + Err(e) => { + error!("Invalid proxy URL: \"{}\", only URLs in the format \"http://host:port\" are allowed", e); exit(1); } + } + }, + ), + ap_port: opt_str(AP_PORT).map(|port| { + let on_error = || { + error!("Invalid `--{}` / `-{}`: \"{}\"", AP_PORT, AP_PORT_SHORT, port); + println!("Valid `--{}` / `-{}` values: 1 - {}", AP_PORT, AP_PORT_SHORT, u16::MAX); + }; - port - }), - } + let port = port.parse::().unwrap_or_else(|_| { + on_error(); + exit(1); + }); + + if port == 0 { + on_error(); + exit(1); + } + + port + }), }; let player_config = { @@ -1138,7 +1204,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|bitrate| { Bitrate::from_str(bitrate).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", BITRATE, BITRATE_SHORT, bitrate ); println!( @@ -1200,7 +1266,7 @@ fn get_setup(args: &[String]) -> Setup { let method = NormalisationMethod::from_str(method).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT, method ); println!( @@ -1227,7 +1293,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|gain_type| { NormalisationType::from_str(gain_type).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_GAIN_TYPE, NORMALISATION_GAIN_TYPE_SHORT, gain_type ); println!( @@ -1244,7 +1310,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|pregain| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_PREGAIN, NORMALISATION_PREGAIN_SHORT, pregain ); println!( @@ -1275,7 +1341,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|threshold| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_THRESHOLD, NORMALISATION_THRESHOLD_SHORT, threshold ); println!( @@ -1309,7 +1375,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|attack| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_ATTACK, NORMALISATION_ATTACK_SHORT, attack ); println!( @@ -1343,7 +1409,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|release| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_RELEASE, NORMALISATION_RELEASE_SHORT, release ); println!( @@ -1377,7 +1443,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|knee| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_KNEE, NORMALISATION_KNEE_SHORT, knee ); println!( @@ -1418,7 +1484,7 @@ fn get_setup(args: &[String]) -> Setup { Some(dither::find_ditherer(ditherer_name).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", DITHER, DITHER_SHORT, opt_str(DITHER).unwrap_or_default() @@ -1488,8 +1554,7 @@ async fn main() { env::set_var(RUST_BACKTRACE, "full") } - let args: Vec = std::env::args().collect(); - let setup = get_setup(&args); + let setup = get_setup(); let mut last_credentials = None; let mut spirc: Option = None; From 79c4040a53f50f14f27c6d9e0fec9ad00101f638 Mon Sep 17 00:00:00 2001 From: Roderick van Domburg Date: Sat, 11 Dec 2021 21:32:34 +0100 Subject: [PATCH 2/7] Skip track on decoding error --- playback/src/player.rs | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/playback/src/player.rs b/playback/src/player.rs index a7ff916d..a56130f3 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -737,7 +737,14 @@ impl PlayerTrackLoader { } }; - assert!(audio.duration >= 0); + if audio.duration < 0 { + error!( + "Track duration for <{}> cannot be {}", + spotify_id.to_uri(), + audio.duration + ); + return None; + } let duration_ms = audio.duration as u32; // (Most) podcasts seem to support only 96 bit Vorbis, so fall back to it @@ -945,7 +952,7 @@ impl Future for PlayerInternal { } Poll::Ready(Err(_)) => { warn!("Unable to load <{:?}>\nSkipping to next track", track_id); - assert!(self.state.is_loading()); + debug_assert!(self.state.is_loading()); self.send_event(PlayerEvent::EndOfTrack { track_id, play_request_id, @@ -1045,8 +1052,11 @@ impl Future for PlayerInternal { } } Err(e) => { - error!("PlayerInternal poll: {}", e); - exit(1); + warn!("Unable to decode samples for track <{:?}: {:?}>\nSkipping to next track", track_id, e); + self.send_event(PlayerEvent::EndOfTrack { + track_id, + play_request_id, + }) } } } @@ -1058,8 +1068,15 @@ impl Future for PlayerInternal { self.handle_packet(packet, normalisation_factor); } Err(e) => { - error!("PlayerInternal poll: {}", e); - exit(1); + warn!( + "Unable to get packet for track <{:?}: {:?}>\nSkipping to next track", + e, + track_id + ); + self.send_event(PlayerEvent::EndOfTrack { + track_id, + play_request_id, + }) } } } else { From 8f23c3498fd9eeaf7d74ab2cb57548f0e811de57 Mon Sep 17 00:00:00 2001 From: Roderick van Domburg Date: Sun, 12 Dec 2021 20:01:05 +0100 Subject: [PATCH 3/7] Clean up warnings --- playback/src/player.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/playback/src/player.rs b/playback/src/player.rs index a56130f3..d8dbb190 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -950,8 +950,11 @@ impl Future for PlayerInternal { exit(1); } } - Poll::Ready(Err(_)) => { - warn!("Unable to load <{:?}>\nSkipping to next track", track_id); + Poll::Ready(Err(e)) => { + warn!( + "Skipping to next track, unable to load track <{:?}>: {:?}", + track_id, e + ); debug_assert!(self.state.is_loading()); self.send_event(PlayerEvent::EndOfTrack { track_id, @@ -1052,7 +1055,7 @@ impl Future for PlayerInternal { } } Err(e) => { - warn!("Unable to decode samples for track <{:?}: {:?}>\nSkipping to next track", track_id, e); + warn!("Skipping to next track, unable to decode samples for track <{:?}>: {:?}", track_id, e); self.send_event(PlayerEvent::EndOfTrack { track_id, play_request_id, @@ -1068,11 +1071,7 @@ impl Future for PlayerInternal { self.handle_packet(packet, normalisation_factor); } Err(e) => { - warn!( - "Unable to get packet for track <{:?}: {:?}>\nSkipping to next track", - e, - track_id - ); + warn!("Skipping to next track, unable to get next packet for track <{:?}>: {:?}", track_id, e); self.send_event(PlayerEvent::EndOfTrack { track_id, play_request_id, From d29337c62df4e12e8d2120ab973b50ec6ab75a38 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Sun, 12 Dec 2021 17:28:41 -0600 Subject: [PATCH 4/7] Dry up error messages. --- src/main.rs | 406 +++++++++++++++++++++++++--------------------------- 1 file changed, 199 insertions(+), 207 deletions(-) diff --git a/src/main.rs b/src/main.rs index 789654ff..7e13a323 100644 --- a/src/main.rs +++ b/src/main.rs @@ -590,30 +590,23 @@ fn get_setup() -> Setup { .to_lowercase() }; - let env_vars: Vec<_> = env::vars_os().filter_map(|(k, v)| { - let mut env_var = None; - if let Ok(key) = k.into_string() { - if key.starts_with("LIBRESPOT_") { - let stripped_key = stripped_env_key(&key); - // Only match against long option/flag names. - // Something like LIBRESPOT_V for example is - // not valid because there are both -v and -V flags - // but env vars are assumed to be all uppercase. - let len = stripped_key.chars().count(); - if len > 1 && matches.opt_defined(&stripped_key) { - match v.into_string() { - Ok(value) => { - env_var = Some((key, value)); - }, - Err(s) => { - eprintln!("Environment variable was not valid Unicode and will not be evaluated: {}={:?}", key, s); - } + let env_vars: Vec<_> = env::vars_os().filter_map(|(k, v)| match k.into_string() { + Ok(key) if key.starts_with("LIBRESPOT_") => { + let stripped_key = stripped_env_key(&key); + // We only care about long option/flag names. + if stripped_key.chars().count() > 1 && matches.opt_defined(&stripped_key) { + match v.into_string() { + Ok(value) => Some((key, value)), + Err(s) => { + eprintln!("Environment variable was not valid Unicode and will not be evaluated: {}={:?}", key, s); + None } } + } else { + None } - } - - env_var + }, + _ => None }) .collect(); @@ -706,13 +699,33 @@ fn get_setup() -> Setup { exit(0); } + let invalid_error_msg = + |long: &str, short: &str, invalid: &str, valid_values: &str, default_value: &str| { + error!("Invalid `--{}` / `-{}`: \"{}\"", long, short, invalid); + + if !valid_values.is_empty() { + println!("Valid `--{}` / `-{}` values: {}", long, short, valid_values); + } + + if !default_value.is_empty() { + println!("Default: {}", default_value); + } + }; + + let empty_string_error_msg = |long: &str, short: &str| { + error!("`--{}` / `-{}` can not be an empty string", long, short); + exit(1); + }; + let backend = audio_backend::find(backend_name).unwrap_or_else(|| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", + invalid_error_msg( BACKEND, BACKEND_SHORT, - opt_str(BACKEND).unwrap_or_default() + &opt_str(BACKEND).unwrap_or_default(), + "", + "", ); + list_backends(); exit(1); }); @@ -721,15 +734,15 @@ fn get_setup() -> Setup { .as_deref() .map(|format| { AudioFormat::from_str(format).unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - FORMAT, FORMAT_SHORT, format + let default_value = &format!("{:?}", AudioFormat::default()); + invalid_error_msg( + FORMAT, + FORMAT_SHORT, + format, + "F64, F32, S32, S24, S24_3, S16", + default_value, ); - println!( - "Valid `--{}` / `-{}` values: F64, F32, S32, S24, S24_3, S16", - FORMAT, FORMAT_SHORT - ); - println!("Default: {:?}", AudioFormat::default()); + exit(1); }) }) @@ -752,11 +765,7 @@ fn get_setup() -> Setup { backend(device, format); exit(0); } else if value.is_empty() { - error!( - "`--{}` / `-{}` can not be an empty string", - DEVICE, DEVICE_SHORT - ); - exit(1); + empty_string_error_msg(DEVICE, DEVICE_SHORT); } } @@ -785,17 +794,14 @@ fn get_setup() -> Setup { let mixer_type: Option = None; let mixer = mixer::find(mixer_type.as_deref()).unwrap_or_else(|| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", + invalid_error_msg( MIXER_TYPE, MIXER_TYPE_SHORT, - opt_str(MIXER_TYPE).unwrap_or_default() + &opt_str(MIXER_TYPE).unwrap_or_default(), + "alsa, softvol", + "softvol", ); - println!( - "Valid `--{}` / `-{}` values: alsa, softvol", - MIXER_TYPE, MIXER_TYPE_SHORT - ); - println!("Default: softvol"); + exit(1); }); @@ -818,11 +824,14 @@ fn get_setup() -> Setup { let index = opt_str(ALSA_MIXER_INDEX) .map(|index| { index.parse::().unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - ALSA_MIXER_INDEX, ALSA_MIXER_INDEX_SHORT, index + invalid_error_msg( + ALSA_MIXER_INDEX, + ALSA_MIXER_INDEX_SHORT, + &index, + "", + &mixer_default_config.index.to_string(), ); - println!("Default: {}", mixer_default_config.index); + exit(1); }) }) @@ -836,11 +845,7 @@ fn get_setup() -> Setup { #[cfg(feature = "alsa-backend")] if control.is_empty() { - error!( - "`--{}` / `-{}` can not be an empty string", - ALSA_MIXER_CONTROL, ALSA_MIXER_CONTROL_SHORT - ); - exit(1); + empty_string_error_msg(ALSA_MIXER_CONTROL, ALSA_MIXER_CONTROL_SHORT); } #[cfg(not(feature = "alsa-backend"))] @@ -849,24 +854,28 @@ fn get_setup() -> Setup { let volume_range = opt_str(VOLUME_RANGE) .map(|range| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - VOLUME_RANGE, VOLUME_RANGE_SHORT, range - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - VOLUME_RANGE, - VOLUME_RANGE_SHORT, + let valid_values = &format!( + "{} - {}", VALID_VOLUME_RANGE.start(), VALID_VOLUME_RANGE.end() ); + #[cfg(feature = "alsa-backend")] - println!( - "Default: softvol - {}, alsa - what the control supports", + let default_value = &format!( + "softvol - {}, alsa - what the control supports", VolumeCtrl::DEFAULT_DB_RANGE ); + #[cfg(not(feature = "alsa-backend"))] - println!("Default: {}", VolumeCtrl::DEFAULT_DB_RANGE); + let default_value = &VolumeCtrl::DEFAULT_DB_RANGE.to_string(); + + invalid_error_msg( + VOLUME_RANGE, + VOLUME_RANGE_SHORT, + &range, + valid_values, + default_value, + ); }; let range = range.parse::().unwrap_or_else(|_| { @@ -891,15 +900,14 @@ fn get_setup() -> Setup { .as_deref() .map(|volume_ctrl| { VolumeCtrl::from_str_with_range(volume_ctrl, volume_range).unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - VOLUME_CTRL, VOLUME_CTRL_SHORT, volume_ctrl + invalid_error_msg( + VOLUME_CTRL, + VOLUME_CTRL_SHORT, + volume_ctrl, + "cubic, fixed, linear, log", + "log", ); - println!( - "Valid `--{}` / `-{}` values: cubic, fixed, linear, log", - VOLUME_CTRL, VOLUME_CTRL - ); - println!("Default: log"); + exit(1); }) }) @@ -938,10 +946,14 @@ fn get_setup() -> Setup { .map(parse_file_size) .map(|e| { e.unwrap_or_else(|e| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - CACHE_SIZE_LIMIT, CACHE_SIZE_LIMIT_SHORT, e + invalid_error_msg( + CACHE_SIZE_LIMIT, + CACHE_SIZE_LIMIT_SHORT, + &e.to_string(), + "", + "", ); + exit(1); }) }) @@ -970,19 +982,11 @@ fn get_setup() -> Setup { if let Some(username) = opt_str(USERNAME) { if username.is_empty() { - error!( - "`--{}` / `-{}` can not be an empty string", - USERNAME, USERNAME_SHORT - ); - exit(1); + empty_string_error_msg(USERNAME, USERNAME_SHORT); } if let Some(password) = opt_str(PASSWORD) { if password.is_empty() { - error!( - "`--{}` / `-{}` can not be an empty string", - PASSWORD, PASSWORD_SHORT - ); - exit(1); + empty_string_error_msg(PASSWORD, PASSWORD_SHORT); } Some(Credentials::with_password(username, password)) } else { @@ -990,7 +994,6 @@ fn get_setup() -> Setup { Some(creds) if username == creds.username => Some(creds), _ => { let prompt = &format!("Password for {}: ", username); - match rpassword::prompt_password_stderr(prompt) { Ok(password) => { if !password.is_empty() { @@ -1015,6 +1018,9 @@ fn get_setup() -> Setup { } } } else { + if cached_creds.is_some() { + trace!("Using cached credentials."); + } cached_creds } }; @@ -1037,16 +1043,8 @@ fn get_setup() -> Setup { opt_str(ZEROCONF_PORT) .map(|port| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - ZEROCONF_PORT, ZEROCONF_PORT_SHORT, port - ); - println!( - "Valid `--{}` / `-{}` values: 1 - {}", - ZEROCONF_PORT, - ZEROCONF_PORT_SHORT, - u16::MAX - ); + let valid_values = &format!("1 - {}", u16::MAX); + invalid_error_msg(ZEROCONF_PORT, ZEROCONF_PORT_SHORT, &port, valid_values, ""); }; let port = port.parse::().unwrap_or_else(|_| { @@ -1072,36 +1070,37 @@ fn get_setup() -> Setup { let name = opt_str(NAME).unwrap_or_else(|| connect_default_config.name.clone()); if name.is_empty() { - error!( - "`--{}` / `-{}` can not be an empty string", - NAME, NAME_SHORT - ); + empty_string_error_msg(NAME, NAME_SHORT); exit(1); } let initial_volume = opt_str(INITIAL_VOLUME) .map(|initial_volume| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - INITIAL_VOLUME, INITIAL_VOLUME_SHORT, initial_volume - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - INITIAL_VOLUME, - INITIAL_VOLUME_SHORT, + let valid_values = &format!( + "{} - {}", VALID_INITIAL_VOLUME_RANGE.start(), VALID_INITIAL_VOLUME_RANGE.end() ); + #[cfg(feature = "alsa-backend")] - println!( - "Default: {}, or the current value when the alsa mixer is used.", + let default_value = &format!( + "{}, or the current value when the alsa mixer is used.", connect_default_config.initial_volume.unwrap_or_default() ); + #[cfg(not(feature = "alsa-backend"))] - println!( - "Default: {}", - connect_default_config.initial_volume.unwrap_or_default() + let default_value = &connect_default_config + .initial_volume + .unwrap_or_default() + .to_string(); + + invalid_error_msg( + INITIAL_VOLUME, + INITIAL_VOLUME_SHORT, + &initial_volume, + valid_values, + default_value, ); }; @@ -1127,12 +1126,18 @@ fn get_setup() -> Setup { .as_deref() .map(|device_type| { DeviceType::from_str(device_type).unwrap_or_else(|_| { - error!("Invalid `--{}` / `-{}`: \"{}\"", DEVICE_TYPE, DEVICE_TYPE_SHORT, device_type); - println!("Valid `--{}` / `-{}` values: computer, tablet, smartphone, speaker, tv, avr, stb, audiodongle, \ - gameconsole, castaudio, castvideo, automobile, smartwatch, chromebook, carthing, homething", - DEVICE_TYPE, DEVICE_TYPE_SHORT + invalid_error_msg( + DEVICE_TYPE, + DEVICE_TYPE_SHORT, + device_type, + "computer, tablet, smartphone, \ + speaker, tv, avr, stb, audiodongle, \ + gameconsole, castaudio, castvideo, \ + automobile, smartwatch, chromebook, \ + carthing, homething", + "speaker", ); - println!("Default: speaker"); + exit(1); }) }) @@ -1178,8 +1183,8 @@ fn get_setup() -> Setup { ), ap_port: opt_str(AP_PORT).map(|port| { let on_error = || { - error!("Invalid `--{}` / `-{}`: \"{}\"", AP_PORT, AP_PORT_SHORT, port); - println!("Valid `--{}` / `-{}` values: 1 - {}", AP_PORT, AP_PORT_SHORT, u16::MAX); + let valid_values = &format!("1 - {}", u16::MAX); + invalid_error_msg(AP_PORT, AP_PORT_SHORT, &port, valid_values, ""); }; let port = port.parse::().unwrap_or_else(|_| { @@ -1203,15 +1208,7 @@ fn get_setup() -> Setup { .as_deref() .map(|bitrate| { Bitrate::from_str(bitrate).unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - BITRATE, BITRATE_SHORT, bitrate - ); - println!( - "Valid `--{}` / `-{}` values: 96, 160, 320", - BITRATE, BITRATE_SHORT - ); - println!("Default: 160"); + invalid_error_msg(BITRATE, BITRATE_SHORT, bitrate, "96, 160, 320", "160"); exit(1); }) }) @@ -1265,15 +1262,14 @@ fn get_setup() -> Setup { ); let method = NormalisationMethod::from_str(method).unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT, method + invalid_error_msg( + NORMALISATION_METHOD, + NORMALISATION_METHOD_SHORT, + method, + "basic, dynamic", + &format!("{:?}", player_default_config.normalisation_method), ); - println!( - "Valid `--{}` / `-{}` values: basic, dynamic", - NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT - ); - println!("Default: {:?}", player_default_config.normalisation_method); + exit(1); }); @@ -1292,15 +1288,14 @@ fn get_setup() -> Setup { .as_deref() .map(|gain_type| { NormalisationType::from_str(gain_type).unwrap_or_else(|_| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_GAIN_TYPE, NORMALISATION_GAIN_TYPE_SHORT, gain_type + invalid_error_msg( + NORMALISATION_GAIN_TYPE, + NORMALISATION_GAIN_TYPE_SHORT, + gain_type, + "track, album, auto", + &format!("{:?}", player_default_config.normalisation_type), ); - println!( - "Valid `--{}` / `-{}` values: track, album, auto", - NORMALISATION_GAIN_TYPE, NORMALISATION_GAIN_TYPE_SHORT, - ); - println!("Default: {:?}", player_default_config.normalisation_type); + exit(1); }) }) @@ -1309,18 +1304,19 @@ fn get_setup() -> Setup { normalisation_pregain = opt_str(NORMALISATION_PREGAIN) .map(|pregain| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_PREGAIN, NORMALISATION_PREGAIN_SHORT, pregain - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - NORMALISATION_PREGAIN, - NORMALISATION_PREGAIN_SHORT, + let valid_values = &format!( + "{} - {}", VALID_NORMALISATION_PREGAIN_RANGE.start(), VALID_NORMALISATION_PREGAIN_RANGE.end() ); - println!("Default: {}", player_default_config.normalisation_pregain); + + invalid_error_msg( + NORMALISATION_PREGAIN, + NORMALISATION_PREGAIN_SHORT, + &pregain, + valid_values, + &player_default_config.normalisation_pregain.to_string(), + ); }; let pregain = pregain.parse::().unwrap_or_else(|_| { @@ -1340,20 +1336,18 @@ fn get_setup() -> Setup { normalisation_threshold = opt_str(NORMALISATION_THRESHOLD) .map(|threshold| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_THRESHOLD, NORMALISATION_THRESHOLD_SHORT, threshold - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - NORMALISATION_THRESHOLD, - NORMALISATION_THRESHOLD_SHORT, + let valid_values = &format!( + "{} - {}", VALID_NORMALISATION_THRESHOLD_RANGE.start(), VALID_NORMALISATION_THRESHOLD_RANGE.end() ); - println!( - "Default: {}", - ratio_to_db(player_default_config.normalisation_threshold) + + invalid_error_msg( + NORMALISATION_THRESHOLD, + NORMALISATION_THRESHOLD_SHORT, + &threshold, + valid_values, + &ratio_to_db(player_default_config.normalisation_threshold).to_string(), ); }; @@ -1374,20 +1368,21 @@ fn get_setup() -> Setup { normalisation_attack = opt_str(NORMALISATION_ATTACK) .map(|attack| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_ATTACK, NORMALISATION_ATTACK_SHORT, attack - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - NORMALISATION_ATTACK, - NORMALISATION_ATTACK_SHORT, + let valid_values = &format!( + "{} - {}", VALID_NORMALISATION_ATTACK_RANGE.start(), VALID_NORMALISATION_ATTACK_RANGE.end() ); - println!( - "Default: {}", - player_default_config.normalisation_attack.as_millis() + + invalid_error_msg( + NORMALISATION_ATTACK, + NORMALISATION_ATTACK_SHORT, + &attack, + valid_values, + &player_default_config + .normalisation_attack + .as_millis() + .to_string(), ); }; @@ -1408,20 +1403,21 @@ fn get_setup() -> Setup { normalisation_release = opt_str(NORMALISATION_RELEASE) .map(|release| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_RELEASE, NORMALISATION_RELEASE_SHORT, release - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - NORMALISATION_RELEASE, - NORMALISATION_RELEASE_SHORT, + let valid_values = &format!( + "{} - {}", VALID_NORMALISATION_RELEASE_RANGE.start(), VALID_NORMALISATION_RELEASE_RANGE.end() ); - println!( - "Default: {}", - player_default_config.normalisation_release.as_millis() + + invalid_error_msg( + NORMALISATION_RELEASE, + NORMALISATION_RELEASE_SHORT, + &release, + valid_values, + &player_default_config + .normalisation_release + .as_millis() + .to_string(), ); }; @@ -1442,18 +1438,19 @@ fn get_setup() -> Setup { normalisation_knee = opt_str(NORMALISATION_KNEE) .map(|knee| { let on_error = || { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", - NORMALISATION_KNEE, NORMALISATION_KNEE_SHORT, knee - ); - println!( - "Valid `--{}` / `-{}` values: {} - {}", - NORMALISATION_KNEE, - NORMALISATION_KNEE_SHORT, + let valid_values = &format!( + "{} - {}", VALID_NORMALISATION_KNEE_RANGE.start(), VALID_NORMALISATION_KNEE_RANGE.end() ); - println!("Default: {}", player_default_config.normalisation_knee); + + invalid_error_msg( + NORMALISATION_KNEE, + NORMALISATION_KNEE_SHORT, + &knee, + valid_values, + &player_default_config.normalisation_knee.to_string(), + ); }; let knee = knee.parse::().unwrap_or_else(|_| { @@ -1483,19 +1480,14 @@ fn get_setup() -> Setup { } Some(dither::find_ditherer(ditherer_name).unwrap_or_else(|| { - error!( - "Invalid `--{}` / `-{}`: \"{}\"", + invalid_error_msg( DITHER, DITHER_SHORT, - opt_str(DITHER).unwrap_or_default() - ); - println!( - "Valid `--{}` / `-{}` values: none, gpdf, tpdf, tpdf_hp", - DITHER, DITHER_SHORT - ); - println!( - "Default: tpdf for formats S16, S24, S24_3 and none for other formats" + &opt_str(DITHER).unwrap_or_default(), + "none, gpdf, tpdf, tpdf_hp", + "tpdf for formats S16, S24, S24_3 and none for other formats", ); + exit(1); })) } From 368bee10885d5d0e528e45328676e59857cd6896 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Mon, 13 Dec 2021 16:40:26 -0600 Subject: [PATCH 5/7] condense some option parsings --- src/main.rs | 229 ++++++++++++++++++---------------------------------- 1 file changed, 78 insertions(+), 151 deletions(-) diff --git a/src/main.rs b/src/main.rs index 7e13a323..2ce526e1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -852,8 +852,9 @@ fn get_setup() -> Setup { let control = mixer_default_config.control; let volume_range = opt_str(VOLUME_RANGE) - .map(|range| { - let on_error = || { + .map(|range| match range.parse::() { + Ok(value) if (VALID_VOLUME_RANGE).contains(&value) => value, + _ => { let valid_values = &format!( "{} - {}", VALID_VOLUME_RANGE.start(), @@ -876,19 +877,9 @@ fn get_setup() -> Setup { valid_values, default_value, ); - }; - let range = range.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_VOLUME_RANGE).contains(&range) { - on_error(); exit(1); } - - range }) .unwrap_or_else(|| match mixer_type.as_deref() { #[cfg(feature = "alsa-backend")] @@ -1041,23 +1032,14 @@ fn get_setup() -> Setup { let zeroconf_port = if enable_discovery { opt_str(ZEROCONF_PORT) - .map(|port| { - let on_error = || { + .map(|port| match port.parse::() { + Ok(value) if value != 0 => value, + _ => { let valid_values = &format!("1 - {}", u16::MAX); invalid_error_msg(ZEROCONF_PORT, ZEROCONF_PORT_SHORT, &port, valid_values, ""); - }; - let port = port.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if port == 0 { - on_error(); exit(1); } - - port }) .unwrap_or(0) } else { @@ -1076,44 +1058,39 @@ fn get_setup() -> Setup { let initial_volume = opt_str(INITIAL_VOLUME) .map(|initial_volume| { - let on_error = || { - let valid_values = &format!( - "{} - {}", - VALID_INITIAL_VOLUME_RANGE.start(), - VALID_INITIAL_VOLUME_RANGE.end() - ); + let volume = match initial_volume.parse::() { + Ok(value) if (VALID_INITIAL_VOLUME_RANGE).contains(&value) => value, + _ => { + let valid_values = &format!( + "{} - {}", + VALID_INITIAL_VOLUME_RANGE.start(), + VALID_INITIAL_VOLUME_RANGE.end() + ); - #[cfg(feature = "alsa-backend")] - let default_value = &format!( - "{}, or the current value when the alsa mixer is used.", - connect_default_config.initial_volume.unwrap_or_default() - ); + #[cfg(feature = "alsa-backend")] + let default_value = &format!( + "{}, or the current value when the alsa mixer is used.", + connect_default_config.initial_volume.unwrap_or_default() + ); - #[cfg(not(feature = "alsa-backend"))] - let default_value = &connect_default_config - .initial_volume - .unwrap_or_default() - .to_string(); + #[cfg(not(feature = "alsa-backend"))] + let default_value = &connect_default_config + .initial_volume + .unwrap_or_default() + .to_string(); - invalid_error_msg( - INITIAL_VOLUME, - INITIAL_VOLUME_SHORT, - &initial_volume, - valid_values, - default_value, - ); + invalid_error_msg( + INITIAL_VOLUME, + INITIAL_VOLUME_SHORT, + &initial_volume, + valid_values, + default_value, + ); + + exit(1); + } }; - let volume = initial_volume.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_INITIAL_VOLUME_RANGE).contains(&volume) { - on_error(); - exit(1); - } - (volume as f32 / 100.0 * VolumeCtrl::MAX_VOLUME as f32) as u16 }) .or_else(|| match mixer_type.as_deref() { @@ -1135,7 +1112,7 @@ fn get_setup() -> Setup { gameconsole, castaudio, castvideo, \ automobile, smartwatch, chromebook, \ carthing, homething", - "speaker", + DeviceType::default().into(), ); exit(1); @@ -1181,23 +1158,14 @@ fn get_setup() -> Setup { } }, ), - ap_port: opt_str(AP_PORT).map(|port| { - let on_error = || { + ap_port: opt_str(AP_PORT).map(|port| match port.parse::() { + Ok(value) if value != 0 => value, + _ => { let valid_values = &format!("1 - {}", u16::MAX); invalid_error_msg(AP_PORT, AP_PORT_SHORT, &port, valid_values, ""); - }; - let port = port.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if port == 0 { - on_error(); exit(1); } - - port }), }; @@ -1302,8 +1270,9 @@ fn get_setup() -> Setup { .unwrap_or(player_default_config.normalisation_type); normalisation_pregain = opt_str(NORMALISATION_PREGAIN) - .map(|pregain| { - let on_error = || { + .map(|pregain| match pregain.parse::() { + Ok(value) if (VALID_NORMALISATION_PREGAIN_RANGE).contains(&value) => value, + _ => { let valid_values = &format!( "{} - {}", VALID_NORMALISATION_PREGAIN_RANGE.start(), @@ -1317,25 +1286,18 @@ fn get_setup() -> Setup { valid_values, &player_default_config.normalisation_pregain.to_string(), ); - }; - let pregain = pregain.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_NORMALISATION_PREGAIN_RANGE).contains(&pregain) { - on_error(); exit(1); } - - pregain }) .unwrap_or(player_default_config.normalisation_pregain); normalisation_threshold = opt_str(NORMALISATION_THRESHOLD) - .map(|threshold| { - let on_error = || { + .map(|threshold| match threshold.parse::() { + Ok(value) if (VALID_NORMALISATION_THRESHOLD_RANGE).contains(&value) => { + db_to_ratio(value) + } + _ => { let valid_values = &format!( "{} - {}", VALID_NORMALISATION_THRESHOLD_RANGE.start(), @@ -1349,25 +1311,18 @@ fn get_setup() -> Setup { valid_values, &ratio_to_db(player_default_config.normalisation_threshold).to_string(), ); - }; - let threshold = threshold.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_NORMALISATION_THRESHOLD_RANGE).contains(&threshold) { - on_error(); exit(1); } - - db_to_ratio(threshold) }) .unwrap_or(player_default_config.normalisation_threshold); normalisation_attack = opt_str(NORMALISATION_ATTACK) - .map(|attack| { - let on_error = || { + .map(|attack| match attack.parse::() { + Ok(value) if (VALID_NORMALISATION_ATTACK_RANGE).contains(&value) => { + Duration::from_millis(value) + } + _ => { let valid_values = &format!( "{} - {}", VALID_NORMALISATION_ATTACK_RANGE.start(), @@ -1384,25 +1339,18 @@ fn get_setup() -> Setup { .as_millis() .to_string(), ); - }; - let attack = attack.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_NORMALISATION_ATTACK_RANGE).contains(&attack) { - on_error(); exit(1); } - - Duration::from_millis(attack) }) .unwrap_or(player_default_config.normalisation_attack); normalisation_release = opt_str(NORMALISATION_RELEASE) - .map(|release| { - let on_error = || { + .map(|release| match release.parse::() { + Ok(value) if (VALID_NORMALISATION_RELEASE_RANGE).contains(&value) => { + Duration::from_millis(value) + } + _ => { let valid_values = &format!( "{} - {}", VALID_NORMALISATION_RELEASE_RANGE.start(), @@ -1419,25 +1367,16 @@ fn get_setup() -> Setup { .as_millis() .to_string(), ); - }; - let release = release.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_NORMALISATION_RELEASE_RANGE).contains(&release) { - on_error(); exit(1); } - - Duration::from_millis(release) }) .unwrap_or(player_default_config.normalisation_release); normalisation_knee = opt_str(NORMALISATION_KNEE) - .map(|knee| { - let on_error = || { + .map(|knee| match knee.parse::() { + Ok(value) if (VALID_NORMALISATION_KNEE_RANGE).contains(&value) => value, + _ => { let valid_values = &format!( "{} - {}", VALID_NORMALISATION_KNEE_RANGE.start(), @@ -1451,47 +1390,35 @@ fn get_setup() -> Setup { valid_values, &player_default_config.normalisation_knee.to_string(), ); - }; - let knee = knee.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); - - if !(VALID_NORMALISATION_KNEE_RANGE).contains(&knee) { - on_error(); exit(1); } - - knee }) .unwrap_or(player_default_config.normalisation_knee); } let ditherer_name = opt_str(DITHER); let ditherer = match ditherer_name.as_deref() { - // explicitly disabled on command line - Some("none") => None, - // explicitly set on command line - Some(_) => { - if matches!(format, AudioFormat::F64 | AudioFormat::F32) { - error!("Dithering is not available with format: {:?}.", format); - exit(1); - } + Some(value) => match value { + "none" => None, + _ => match format { + AudioFormat::F64 | AudioFormat::F32 => { + error!("Dithering is not available with format: {:?}.", format); + exit(1); + } + _ => Some(dither::find_ditherer(ditherer_name).unwrap_or_else(|| { + invalid_error_msg( + DITHER, + DITHER_SHORT, + &opt_str(DITHER).unwrap_or_default(), + "none, gpdf, tpdf, tpdf_hp for formats S16, S24, S24_3, S32, none for formats F32, F64", + "tpdf for formats S16, S24, S24_3 and none for formats S32, F32, F64", + ); - Some(dither::find_ditherer(ditherer_name).unwrap_or_else(|| { - invalid_error_msg( - DITHER, - DITHER_SHORT, - &opt_str(DITHER).unwrap_or_default(), - "none, gpdf, tpdf, tpdf_hp", - "tpdf for formats S16, S24, S24_3 and none for other formats", - ); - - exit(1); - })) - } - // nothing set on command line => use default + exit(1); + })), + }, + }, None => match format { AudioFormat::S16 | AudioFormat::S24 | AudioFormat::S24_3 => { player_default_config.ditherer From 67836b5b02f2990376849fc04d2f9cd0316f1e0b Mon Sep 17 00:00:00 2001 From: Mateusz Mojsiejuk Date: Tue, 14 Dec 2021 21:23:13 +0100 Subject: [PATCH 6/7] Added arm64 target to docker run examples. Also removed feature quotes as they're not nessesary and don't match the non-quoted examples of targets in the WIKI --- contrib/Dockerfile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contrib/Dockerfile b/contrib/Dockerfile index 74b83d31..aa29183c 100644 --- a/contrib/Dockerfile +++ b/contrib/Dockerfile @@ -9,8 +9,10 @@ # # If only one architecture is desired, cargo can be invoked directly with the appropriate options : # $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --no-default-features --features "alsa-backend" -# $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --target arm-unknown-linux-gnueabihf --no-default-features --features "alsa-backend" -# $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --target arm-unknown-linux-gnueabi --no-default-features --features "alsa-backend" +# $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --target arm-unknown-linux-gnueabihf --no-default-features --features alsa-backend +# $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --target arm-unknown-linux-gnueabi --no-default-features --features alsa-backend +# $ docker run -v /tmp/librespot-build:/build librespot-cross cargo build --release --target aarch64-unknown-linux-gnu --no-default-features --features alsa-backend + # $ docker run -v /tmp/librespot-build:/build librespot-cross contrib/docker-build-pi-armv6hf.sh FROM debian:stretch From d5efb8a620554ced4fda731c72046c4b1af53111 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Tue, 14 Dec 2021 16:49:09 -0600 Subject: [PATCH 7/7] Dynamic failable buffer sizing alsa-backend Dynamically set the alsa buffer and period based on the device's reported min/max buffer and period sizes. In the event of failure use the device's defaults. This should have no effect on devices that allow for reasonable buffer and period sizes but would allow us to be more forgiving with less reasonable devices or configurations. Closes: https://github.com/librespot-org/librespot/issues/895 --- playback/src/audio_backend/alsa.rs | 187 +++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 13 deletions(-) diff --git a/playback/src/audio_backend/alsa.rs b/playback/src/audio_backend/alsa.rs index e572f953..4f82a097 100644 --- a/playback/src/audio_backend/alsa.rs +++ b/playback/src/audio_backend/alsa.rs @@ -4,16 +4,18 @@ use crate::convert::Converter; use crate::decoder::AudioPacket; use crate::{NUM_CHANNELS, SAMPLE_RATE}; use alsa::device_name::HintIter; -use alsa::pcm::{Access, Format, HwParams, PCM}; +use alsa::pcm::{Access, Format, Frames, HwParams, PCM}; use alsa::{Direction, ValueOr}; use std::cmp::min; use std::process::exit; -use std::time::Duration; use thiserror::Error; -// 0.5 sec buffer. -const PERIOD_TIME: Duration = Duration::from_millis(100); -const BUFFER_TIME: Duration = Duration::from_millis(500); +const MAX_BUFFER: Frames = (SAMPLE_RATE / 2) as Frames; +const MIN_BUFFER: Frames = (SAMPLE_RATE / 10) as Frames; +const ZERO_FRAMES: Frames = 0; + +const MAX_PERIOD_DIVISOR: Frames = 4; +const MIN_PERIOD_DIVISOR: Frames = 10; #[derive(Debug, Error)] enum AlsaError { @@ -195,28 +197,187 @@ fn open_device(dev_name: &str, format: AudioFormat) -> SinkResult<(PCM, usize)> e, })?; - hwp.set_buffer_time_near(BUFFER_TIME.as_micros() as u32, ValueOr::Nearest) - .map_err(AlsaError::HwParams)?; + // Clone the hwp while it's in + // a good working state so that + // in the event of an error setting + // the buffer and period sizes + // we can use the good working clone + // instead of the hwp that's in an + // error state. + let hwp_clone = hwp.clone(); - hwp.set_period_time_near(PERIOD_TIME.as_micros() as u32, ValueOr::Nearest) - .map_err(AlsaError::HwParams)?; + // At a sampling rate of 44100: + // The largest buffer is 22050 Frames (500ms) with 5512 Frame periods (125ms). + // The smallest buffer is 4410 Frames (100ms) with 441 Frame periods (10ms). + // Actual values may vary. + // + // Larger buffer and period sizes are preferred as extremely small values + // will cause high CPU useage. + // + // If no buffer or period size is in those ranges or an error happens + // trying to set the buffer or period size use the device's defaults + // which may not be ideal but are *hopefully* serviceable. - pcm.hw_params(&hwp).map_err(AlsaError::Pcm)?; + let buffer_size = { + let max = match hwp.get_buffer_size_max() { + Err(e) => { + trace!("Error getting the device's max Buffer size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + }; - let swp = pcm.sw_params_current().map_err(AlsaError::Pcm)?; + let min = match hwp.get_buffer_size_min() { + Err(e) => { + trace!("Error getting the device's min Buffer size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + }; + + let buffer_size = if min < max { + match (MIN_BUFFER..=MAX_BUFFER) + .rev() + .find(|f| (min..=max).contains(f)) + { + Some(size) => { + trace!("Desired Frames per Buffer: {:?}", size); + + match hwp.set_buffer_size_near(size) { + Err(e) => { + trace!("Error setting the device's Buffer size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + } + } + None => { + trace!("No Desired Buffer size in range reported by the device."); + ZERO_FRAMES + } + } + } else { + trace!("The device's min reported Buffer size was greater than or equal to it's max reported Buffer size."); + ZERO_FRAMES + }; + + if buffer_size == ZERO_FRAMES { + trace!( + "Desired Buffer Frame range: {:?} - {:?}", + MIN_BUFFER, + MAX_BUFFER + ); + + trace!( + "Actual Buffer Frame range as reported by the device: {:?} - {:?}", + min, + max + ); + } + + buffer_size + }; + + let period_size = { + if buffer_size == ZERO_FRAMES { + ZERO_FRAMES + } else { + let max = match hwp.get_period_size_max() { + Err(e) => { + trace!("Error getting the device's max Period size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + }; + + let min = match hwp.get_period_size_min() { + Err(e) => { + trace!("Error getting the device's min Period size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + }; + + let max_period = buffer_size / MAX_PERIOD_DIVISOR; + let min_period = buffer_size / MIN_PERIOD_DIVISOR; + + let period_size = if min < max && min_period < max_period { + match (min_period..=max_period) + .rev() + .find(|f| (min..=max).contains(f)) + { + Some(size) => { + trace!("Desired Frames per Period: {:?}", size); + + match hwp.set_period_size_near(size, ValueOr::Nearest) { + Err(e) => { + trace!("Error setting the device's Period size: {}", e); + ZERO_FRAMES + } + Ok(s) => s, + } + } + None => { + trace!("No Desired Period size in range reported by the device."); + ZERO_FRAMES + } + } + } else { + trace!("The device's min reported Period size was greater than or equal to it's max reported Period size,"); + trace!("or the desired min Period size was greater than or equal to the desired max Period size."); + ZERO_FRAMES + }; + + if period_size == ZERO_FRAMES { + trace!("Buffer size: {:?}", buffer_size); + + trace!( + "Desired Period Frame range: {:?} (Buffer size / {:?}) - {:?} (Buffer size / {:?})", + min_period, + MIN_PERIOD_DIVISOR, + max_period, + MAX_PERIOD_DIVISOR, + ); + + trace!( + "Actual Period Frame range as reported by the device: {:?} - {:?}", + min, + max + ); + } + + period_size + } + }; + + if buffer_size == ZERO_FRAMES || period_size == ZERO_FRAMES { + trace!( + "Failed to set Buffer and/or Period size, falling back to the device's defaults." + ); + + trace!("You may experience higher than normal CPU usage and/or audio issues."); + + pcm.hw_params(&hwp_clone).map_err(AlsaError::Pcm)?; + } else { + pcm.hw_params(&hwp).map_err(AlsaError::Pcm)?; + } + + let hwp = pcm.hw_params_current().map_err(AlsaError::Pcm)?; // Don't assume we got what we wanted. Ask to make sure. let frames_per_period = hwp.get_period_size().map_err(AlsaError::HwParams)?; let frames_per_buffer = hwp.get_buffer_size().map_err(AlsaError::HwParams)?; + let swp = pcm.sw_params_current().map_err(AlsaError::Pcm)?; + swp.set_start_threshold(frames_per_buffer - frames_per_period) .map_err(AlsaError::SwParams)?; pcm.sw_params(&swp).map_err(AlsaError::Pcm)?; - trace!("Frames per Buffer: {:?}", frames_per_buffer); - trace!("Frames per Period: {:?}", frames_per_period); + trace!("Actual Frames per Buffer: {:?}", frames_per_buffer); + trace!("Actual Frames per Period: {:?}", frames_per_period); // Let ALSA do the math for us. pcm.frames_to_bytes(frames_per_period) as usize