From 79c4040a53f50f14f27c6d9e0fec9ad00101f638 Mon Sep 17 00:00:00 2001 From: Roderick van Domburg Date: Sat, 11 Dec 2021 21:32:34 +0100 Subject: [PATCH 1/4] 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 2/4] 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 67836b5b02f2990376849fc04d2f9cd0316f1e0b Mon Sep 17 00:00:00 2001 From: Mateusz Mojsiejuk Date: Tue, 14 Dec 2021 21:23:13 +0100 Subject: [PATCH 3/4] 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 4/4] 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