From 2f05ddfbc20fb04131a2ad1ce68dbc181fe6b7cf Mon Sep 17 00:00:00 2001 From: johannesd3 Date: Fri, 12 Feb 2021 18:19:04 +0100 Subject: [PATCH 1/4] Fix bugs in player --- playback/src/player.rs | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/playback/src/player.rs b/playback/src/player.rs index b5683e55..26eea7f2 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -327,15 +327,12 @@ impl Player { } pub async fn get_end_of_track_future(&self) { - self.get_player_event_channel() - .filter(|event| { - future::ready(matches!( - event, - PlayerEvent::EndOfTrack { .. } | PlayerEvent::Stopped { .. } - )) - }) - .for_each(|_| future::ready(())) - .await + let mut channel = self.get_player_event_channel(); + while let Some(event) = channel.next().await { + if matches!(event, PlayerEvent::EndOfTrack { .. } | PlayerEvent::Stopped { .. }) { + return; + } + } } pub fn set_sink_event_callback(&self, callback: Option) { @@ -676,14 +673,6 @@ impl PlayerTrackLoader { let bytes_per_second = self.stream_data_rate(format); let play_from_beginning = position_ms == 0; - let key = match self.session.audio_key().request(spotify_id, file_id).await { - Ok(key) => key, - Err(_) => { - error!("Unable to load decryption key"); - return None; - } - }; - // This is only a loop to be able to reload the file if an error occured // while opening a cached file. loop { @@ -712,6 +701,14 @@ impl PlayerTrackLoader { // we need to seek -> we set stream mode after the initial seek. stream_loader_controller.set_random_access_mode(); } + + let key = match self.session.audio_key().request(spotify_id, file_id).await { + Ok(key) => key, + Err(_) => { + error!("Unable to load decryption key"); + return None; + } + }; let mut decrypted_file = AudioDecrypt::new(key, encrypted_file); From b2f1be4374e091267d0676e92870b5f3eb30fa7c Mon Sep 17 00:00:00 2001 From: johannesd3 Date: Fri, 12 Feb 2021 18:25:13 +0100 Subject: [PATCH 2/4] Make `RodioSink` `Send` and improve error handling --- Cargo.lock | 1 + playback/Cargo.toml | 9 ++- playback/src/audio_backend/rodio.rs | 111 ++++++++++++++++++---------- 3 files changed, 78 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6255e76c..00343168 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1375,6 +1375,7 @@ dependencies = [ "rodio", "sdl2", "shell-words", + "thiserror", "zerocopy", ] diff --git a/playback/Cargo.toml b/playback/Cargo.toml index 95c4a12a..15622198 100644 --- a/playback/Cargo.toml +++ b/playback/Cargo.toml @@ -29,19 +29,22 @@ libpulse-binding = { version = "2.13", optional = true, default-features libpulse-simple-binding = { version = "2.13", optional = true, default-features = false } jack = { version = "0.6", optional = true } libc = { version = "0.2", optional = true } -rodio = { version = "0.13", optional = true, default-features = false } -cpal = { version = "0.13", optional = true } sdl2 = { version = "0.34", optional = true } gstreamer = { version = "0.16", optional = true } gstreamer-app = { version = "0.16", optional = true } glib = { version = "0.10", optional = true } zerocopy = { version = "0.3", optional = true } +# Rodio dependencies +rodio = { version = "0.13", optional = true, default-features = false } +cpal = { version = "0.13", optional = true } +thiserror = { version = "1", optional = true } + [features] alsa-backend = ["alsa"] portaudio-backend = ["portaudio-rs"] pulseaudio-backend = ["libpulse-binding", "libpulse-simple-binding"] jackaudio-backend = ["jack"] -rodio-backend = ["rodio", "cpal"] +rodio-backend = ["rodio", "cpal", "thiserror"] sdl-backend = ["sdl2"] gstreamer-backend = ["gstreamer", "gstreamer-app", "glib", "zerocopy"] diff --git a/playback/src/audio_backend/rodio.rs b/playback/src/audio_backend/rodio.rs index bc101786..0c4b1a42 100644 --- a/playback/src/audio_backend/rodio.rs +++ b/playback/src/audio_backend/rodio.rs @@ -1,20 +1,36 @@ -use super::{Open, Sink}; -extern crate cpal; -extern crate rodio; -use cpal::traits::{DeviceTrait, HostTrait}; use std::process::exit; +use std::{convert::Infallible, sync::mpsc}; use std::{io, thread, time}; +use cpal::traits::{DeviceTrait, HostTrait}; +use thiserror::Error; + +use super::{Open, Sink}; + +#[derive(Debug, Error)] +pub enum RodioError { + #[error("Rodio: no device available")] + NoDeviceAvailable, + #[error("Rodio: device \"{0}\" is not available")] + DeviceNotAvailable(String), + #[error("Rodio play error: {0}")] + PlayError(#[from] rodio::PlayError), + #[error("Rodio stream error: {0}")] + StreamError(#[from] rodio::StreamError), + #[error("Cannot get audio devices: {0}")] + DevicesError(#[from] cpal::DevicesError), +} + pub struct RodioSink { rodio_sink: rodio::Sink, - // We have to keep hold of this object, or the Sink can't play... - #[allow(dead_code)] - stream: rodio::OutputStream, + + // will produce a TryRecvError on the receiver side when it is dropped. + _close_tx: mpsc::SyncSender, } -fn list_formats(ref device: &rodio::Device) { +fn list_formats(device: &rodio::Device) { let default_fmt = match device.default_output_config() { - Ok(fmt) => cpal::SupportedStreamConfig::from(fmt), + Ok(fmt) => fmt, Err(e) => { warn!("Error getting default rodio::Sink config: {}", e); return; @@ -39,8 +55,8 @@ fn list_formats(ref device: &rodio::Device) { } } -fn list_outputs() { - let default_device = get_default_device(); +fn list_outputs_and_exit() -> ! { + let default_device = get_default_device().unwrap(); let default_device_name = default_device.name().expect("cannot get output name"); println!("Default Audio Device:\n {}", default_device_name); list_formats(&default_device); @@ -56,54 +72,69 @@ fn list_outputs() { list_formats(&device); } } + + exit(0) } -fn get_default_device() -> rodio::Device { +fn get_default_device() -> Result { cpal::default_host() .default_output_device() - .expect("no default output device available") + .ok_or(RodioError::NoDeviceAvailable) } -fn match_device(device: Option) -> rodio::Device { - match device { +fn create_sink(device: Option) -> Result<(rodio::Sink, rodio::OutputStream), RodioError> { + let rodio_device = match device { + Some(ask) if &ask == "?" => list_outputs_and_exit(), Some(device_name) => { - if device_name == "?".to_string() { - list_outputs(); - exit(0) - } - for d in cpal::default_host() - .output_devices() - .expect("cannot get list of output devices") - { - if d.name().expect("cannot get output name") == device_name { - return d; - } - } - println!("No output sink matching '{}' found.", device_name); - exit(0) + cpal::default_host() + .output_devices()? + .find(|d| d.name().ok().map_or(false, |name| name == device_name)) // Ignore devices for which getting name fails + .ok_or(RodioError::DeviceNotAvailable(device_name))? } - None => return get_default_device(), - } + None => get_default_device()?, + }; + + let name = rodio_device.name().ok(); + info!( + "Using audio device: {}", + name.as_deref().unwrap_or("(unknown name)") + ); + + let (stream, handle) = rodio::OutputStream::try_from_device(&rodio_device)?; + let sink = rodio::Sink::try_new(&handle)?; + Ok((sink, stream)) } impl Open for RodioSink { fn open(device: Option) -> RodioSink { debug!( "Using rodio sink with cpal host: {:?}", - cpal::default_host().id() + cpal::default_host().id().name() ); - let rodio_device = match_device(device); - debug!("Using cpal device"); - let stream = rodio::OutputStream::try_from_device(&rodio_device) - .expect("Couldn't open output stream."); - debug!("Using rodio stream"); - let sink = rodio::Sink::try_new(&stream.1).expect("Couldn't create output sink."); - debug!("Using rodio sink"); + let (sink_tx, sink_rx) = mpsc::sync_channel(1); + let (close_tx, close_rx) = mpsc::sync_channel(1); + std::thread::spawn(move || match create_sink(device) { + Ok((sink, stream)) => { + sink_tx.send(Ok(sink)).unwrap(); + + close_rx.recv().unwrap_err(); // This will fail as soon as the sender is dropped + debug!("drop rodio::OutputStream"); + drop(stream); + } + Err(e) => { + sink_tx.send(Err(e)).unwrap(); + } + }); + + // Instead of the second `unwrap`, better error handling could be introduced + let sink = sink_rx.recv().unwrap().unwrap(); + + debug!("Rodio sink was created"); RodioSink { rodio_sink: sink, - stream: stream.0, + _close_tx: close_tx, } } } From 689415a6f1580547dae5be592cb21a847114edb1 Mon Sep 17 00:00:00 2001 From: johannesd3 Date: Fri, 12 Feb 2021 19:31:41 +0100 Subject: [PATCH 3/4] Improved error handling in rodio backend --- playback/src/audio_backend/rodio.rs | 103 +++++++++++++++++----------- 1 file changed, 64 insertions(+), 39 deletions(-) diff --git a/playback/src/audio_backend/rodio.rs b/playback/src/audio_backend/rodio.rs index 0c4b1a42..034bd086 100644 --- a/playback/src/audio_backend/rodio.rs +++ b/playback/src/audio_backend/rodio.rs @@ -29,75 +29,100 @@ pub struct RodioSink { } fn list_formats(device: &rodio::Device) { - let default_fmt = match device.default_output_config() { - Ok(fmt) => fmt, - Err(e) => { - warn!("Error getting default rodio::Sink config: {}", e); - return; + match device.default_output_config() { + Ok(cfg) => { + debug!(" Default config:"); + debug!(" {:?}", cfg); } - }; - debug!(" Default config:"); - debug!(" {:?}", default_fmt); - - let mut output_configs = match device.supported_output_configs() { - Ok(f) => f.peekable(), Err(e) => { - warn!("Error getting supported rodio::Sink configs: {}", e); - return; + // Use loglevel debug, since even the output is only debug + debug!("Error getting default rodio::Sink config: {}", e); } }; - if output_configs.peek().is_some() { - debug!(" Available configs:"); - for format in output_configs { - debug!(" {:?}", format); + match device.supported_output_configs() { + Ok(mut cfgs) => { + if let Some(first) = cfgs.next() { + debug!(" Available configs:"); + debug!(" {:?}", first); + } else { + return; + } + + for cfg in cfgs { + debug!(" {:?}", cfg); + } + } + Err(e) => { + debug!("Error getting supported rodio::Sink configs: {}", e); } } } -fn list_outputs_and_exit() -> ! { - let default_device = get_default_device().unwrap(); - let default_device_name = default_device.name().expect("cannot get output name"); - println!("Default Audio Device:\n {}", default_device_name); - list_formats(&default_device); +fn list_outputs() -> Result<(), cpal::DevicesError> { + let mut default_device_name = None; - println!("Other Available Audio Devices:"); - for device in cpal::default_host() - .output_devices() - .expect("cannot get list of output devices") - { - let device_name = device.name().expect("cannot get output name"); - if device_name != default_device_name { - println!(" {}", device_name); - list_formats(&device); + if let Some(default_device) = get_default_device() { + default_device_name = default_device.name().ok(); + println!( + "Default Audio Device:\n {}", + default_device_name.as_deref().unwrap_or("[unknown name]") + ); + + list_formats(&default_device); + + println!("Other Available Audio Devices:"); + } else { + warn!("No default device was found"); + } + + for device in cpal::default_host().output_devices()? { + match device.name() { + Ok(name) if Some(&name) == default_device_name.as_ref() => (), + Ok(name) => { + println!(" {}", name); + list_formats(&device); + } + Err(e) => { + warn!("Cannot get device name: {}", e); + println!(" [unknown name]"); + list_formats(&device); + } } } - exit(0) + Ok(()) } -fn get_default_device() -> Result { - cpal::default_host() - .default_output_device() - .ok_or(RodioError::NoDeviceAvailable) +fn get_default_device() -> Option { + cpal::default_host().default_output_device() } fn create_sink(device: Option) -> Result<(rodio::Sink, rodio::OutputStream), RodioError> { let rodio_device = match device { - Some(ask) if &ask == "?" => list_outputs_and_exit(), + Some(ask) if &ask == "?" => { + let exit_code = match list_outputs() { + Ok(()) => 0, + Err(e) => { + error!("{}", e); + 1 + } + }; + exit(exit_code) + } Some(device_name) => { cpal::default_host() .output_devices()? .find(|d| d.name().ok().map_or(false, |name| name == device_name)) // Ignore devices for which getting name fails .ok_or(RodioError::DeviceNotAvailable(device_name))? } - None => get_default_device()?, + None => get_default_device().ok_or(RodioError::NoDeviceAvailable)?, }; let name = rodio_device.name().ok(); info!( "Using audio device: {}", - name.as_deref().unwrap_or("(unknown name)") + name.as_deref().unwrap_or("[unknown name]") ); let (stream, handle) = rodio::OutputStream::try_from_device(&rodio_device)?; From b77f0a18cef0c893cb48d2b34746287c7c00cdc6 Mon Sep 17 00:00:00 2001 From: johannesd3 Date: Sat, 13 Feb 2021 10:29:00 +0100 Subject: [PATCH 4/4] Fix formatting --- core/src/connection/mod.rs | 2 +- playback/src/player.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/core/src/connection/mod.rs b/core/src/connection/mod.rs index 1ca73165..68e2e7a5 100644 --- a/core/src/connection/mod.rs +++ b/core/src/connection/mod.rs @@ -32,7 +32,7 @@ pub async fn connect(addr: String, proxy: &Option) -> io::Result .map_err(|e| { io::Error::new(io::ErrorKind::InvalidInput, format!("Invalid port: {}", e)) })?; - + let host = split .next() .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "Missing port"))?; diff --git a/playback/src/player.rs b/playback/src/player.rs index 26eea7f2..6f6a85ae 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -329,7 +329,10 @@ impl Player { pub async fn get_end_of_track_future(&self) { let mut channel = self.get_player_event_channel(); while let Some(event) = channel.next().await { - if matches!(event, PlayerEvent::EndOfTrack { .. } | PlayerEvent::Stopped { .. }) { + if matches!( + event, + PlayerEvent::EndOfTrack { .. } | PlayerEvent::Stopped { .. } + ) { return; } } @@ -701,7 +704,7 @@ impl PlayerTrackLoader { // we need to seek -> we set stream mode after the initial seek. stream_loader_controller.set_random_access_mode(); } - + let key = match self.session.audio_key().request(spotify_id, file_id).await { Ok(key) => key, Err(_) => {