Merge branch 'librespot-org:dev' into fix_pipe_backend

CHANGELOG.md

@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [playback] `alsamixer`: complete rewrite (breaking)
 - [playback] `alsamixer`: query card dB range for the `log` volume control unless specified otherwise
 - [playback] `alsamixer`: use `--device` name for `--mixer-card` unless specified otherwise
+- [playback] `player`: consider errors in `sink.start`, `sink.stop` and `sink.write` fatal and `exit(1)` (breaking)
 
 ### Deprecated
 - [connect] The `discovery` module was deprecated in favor of the `librespot-discovery` crate
@@ -42,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [playback] `alsamixer`: make `--volume-ctrl {linear|log}` work as expected
 - [playback] `alsa`, `gstreamer`, `pulseaudio`: always output in native endianness
 - [playback] `alsa`: revert buffer size to ~500 ms
+- [playback] `alsa`: better error handling
 
 ## [0.2.0] - 2021-05-04
 

Cargo.toml

@@ -90,5 +90,6 @@ section = "sound"
 priority = "optional"
 assets = [
     ["target/release/librespot", "usr/bin/", "755"],
-    ["contrib/librespot.service", "lib/systemd/system/", "644"]
+    ["contrib/librespot.service", "lib/systemd/system/", "644"],
+    ["contrib/librespot.user.service", "lib/systemd/user/", "644"]
 ]

README.md

@@ -89,6 +89,7 @@ The above command will create a receiver named ```Librespot```, with bitrate set
 A full list of runtime options are available [here](https://github.com/librespot-org/librespot/wiki/Options)
 
 _Please Note: When using the cache feature, an authentication blob is stored for your account in the cache directory. For security purposes, we recommend that you set directory permissions on the cache directory to `700`._
+
 ## Contact
 Come and hang out on gitter if you need help or want to offer some.
 https://gitter.im/librespot-org/spotify-connect-resources

contrib/librespot.service

@@ -1,5 +1,7 @@
 [Unit]
-Description=Librespot
+Description=Librespot (an open source Spotify client)
+Documentation=https://github.com/librespot-org/librespot
+Documentation=https://github.com/librespot-org/librespot/wiki/Options
 Requires=network-online.target
 After=network-online.target
 
@@ -8,8 +10,7 @@ User=nobody
 Group=audio
 Restart=always
 RestartSec=10
-ExecStart=/usr/bin/librespot -n "%p on %H"
+ExecStart=/usr/bin/librespot --name "%p@%H"
 
 [Install]
 WantedBy=multi-user.target
-

contrib/librespot.user.service

@@ -0,0 +1,12 @@
+[Unit]
+Description=Librespot (an open source Spotify client)
+Documentation=https://github.com/librespot-org/librespot
+Documentation=https://github.com/librespot-org/librespot/wiki/Options
+
+[Service]
+Restart=always
+RestartSec=10
+ExecStart=/usr/bin/librespot --name "%u@%H"
+
+[Install]
+WantedBy=default.target

playback/Cargo.toml

@@ -51,7 +51,7 @@ rand = "0.8"
 rand_distr = "0.4"
 
 [features]
-alsa-backend = ["alsa"]
+alsa-backend = ["alsa", "thiserror"]
 portaudio-backend = ["portaudio-rs"]
 pulseaudio-backend = ["libpulse-binding", "libpulse-simple-binding"]
 jackaudio-backend = ["jack"]

playback/src/audio_backend/alsa.rs

@@ -5,17 +5,49 @@ use crate::decoder::AudioPacket;
 use crate::{NUM_CHANNELS, SAMPLE_RATE};
 use alsa::device_name::HintIter;
 use alsa::pcm::{Access, Format, HwParams, PCM};
-use alsa::{Direction, Error, ValueOr};
+use alsa::{Direction, ValueOr};
 use std::cmp::min;
-use std::ffi::CString;
 use std::io;
 use std::process::exit;
 use std::time::Duration;
+use thiserror::Error;
 
 // 125 ms Period time * 4 periods = 0.5 sec buffer.
 const PERIOD_TIME: Duration = Duration::from_millis(125);
 const NUM_PERIODS: u32 = 4;
 
+#[derive(Debug, Error)]
+enum AlsaError {
+    #[error("AlsaSink, device {device} may be invalid or busy, {err}")]
+    PCMSetUpError { device: String, err: alsa::Error },
+    #[error("AlsaSink, device {device} unsupported access type RWInterleaved, {err}")]
+    UnsupportedAccessTypeError { device: String, err: alsa::Error },
+    #[error("AlsaSink, device {device} unsupported format {format:?}, {err}")]
+    UnsupportedFormatError {
+        device: String,
+        format: AudioFormat,
+        err: alsa::Error,
+    },
+    #[error("AlsaSink, device {device} unsupported sample rate {samplerate}, {err}")]
+    UnsupportedSampleRateError {
+        device: String,
+        samplerate: u32,
+        err: alsa::Error,
+    },
+    #[error("AlsaSink, device {device} unsupported channel count {channel_count}, {err}")]
+    UnsupportedChannelCountError {
+        device: String,
+        channel_count: u8,
+        err: alsa::Error,
+    },
+    #[error("AlsaSink Hardware Parameters Error, {0}")]
+    HwParamsError(alsa::Error),
+    #[error("AlsaSink Software Parameters Error, {0}")]
+    SwParamsError(alsa::Error),
+    #[error("AlsaSink PCM Error, {0}")]
+    PCMError(alsa::Error),
+}
+
 pub struct AlsaSink {
     pcm: Option<PCM>,
     format: AudioFormat,
@@ -23,25 +55,40 @@ pub struct AlsaSink {
     period_buffer: Vec<u8>,
 }
 
-fn list_outputs() {
+fn list_outputs() -> io::Result<()> {
+    println!("Listing available Alsa outputs:");
     for t in &["pcm", "ctl", "hwdep"] {
         println!("{} devices:", t);
-        let i = HintIter::new(None, &*CString::new(*t).unwrap()).unwrap();
+        let i = match HintIter::new_str(None, &t) {
+            Ok(i) => i,
+            Err(e) => {
+                return Err(io::Error::new(io::ErrorKind::Other, e));
+            }
+        };
         for a in i {
             if let Some(Direction::Playback) = a.direction {
                 // mimic aplay -L
-                println!(
+                let name = a
-                    "{}\n\t{}\n",
+                    .name
-                    a.name.unwrap(),
+                    .ok_or(io::Error::new(io::ErrorKind::Other, "Could not parse name"))?;
-                    a.desc.unwrap().replace("\n", "\n\t")
+                let desc = a
-                );
+                    .desc
+                    .ok_or(io::Error::new(io::ErrorKind::Other, "Could not parse desc"))?;
+                println!("{}\n\t{}\n", name, desc.replace("\n", "\n\t"));
             }
         }
     }
+
+    Ok(())
 }
 
-fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), Box<Error>> {
+fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), AlsaError> {
-    let pcm = PCM::new(dev_name, Direction::Playback, false)?;
+    let pcm =
+        PCM::new(dev_name, Direction::Playback, false).map_err(|e| AlsaError::PCMSetUpError {
+            device: dev_name.to_string(),
+            err: e,
+        })?;
+
     let alsa_format = match format {
         AudioFormat::F64 => Format::float64(),
         AudioFormat::F32 => Format::float(),
@@ -56,23 +103,64 @@ fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), Box<
     };
 
     let bytes_per_period = {
-        let hwp = HwParams::any(&pcm)?;
+        let hwp = HwParams::any(&pcm).map_err(|e| AlsaError::HwParamsError(e))?;
-        hwp.set_access(Access::RWInterleaved)?;
+        hwp.set_access(Access::RWInterleaved).map_err(|e| {
-        hwp.set_format(alsa_format)?;
+            AlsaError::UnsupportedAccessTypeError {
-        hwp.set_rate(SAMPLE_RATE, ValueOr::Nearest)?;
+                device: dev_name.to_string(),
-        hwp.set_channels(NUM_CHANNELS as u32)?;
+                err: e,
-        // Deal strictly in time and periods.
+            }
-        hwp.set_periods(NUM_PERIODS, ValueOr::Nearest)?;
+        })?;
-        hwp.set_period_time_near(PERIOD_TIME.as_micros() as u32, ValueOr::Nearest)?;
+
-        pcm.hw_params(&hwp)?;
+        hwp.set_format(alsa_format)
+            .map_err(|e| AlsaError::UnsupportedFormatError {
+                device: dev_name.to_string(),
+                format: format,
+                err: e,
+            })?;
+
+        hwp.set_rate(SAMPLE_RATE, ValueOr::Nearest).map_err(|e| {
+            AlsaError::UnsupportedSampleRateError {
+                device: dev_name.to_string(),
+                samplerate: SAMPLE_RATE,
+                err: e,
+            }
+        })?;
+
+        hwp.set_channels(NUM_CHANNELS as u32).map_err(|e| {
+            AlsaError::UnsupportedChannelCountError {
+                device: dev_name.to_string(),
+                channel_count: NUM_CHANNELS,
+                err: e,
+            }
+        })?;
+
+        // Deal strictly in time and periods.
+        hwp.set_periods(NUM_PERIODS, ValueOr::Nearest)
+            .map_err(|e| AlsaError::HwParamsError(e))?;
+
+        hwp.set_period_time_near(PERIOD_TIME.as_micros() as u32, ValueOr::Nearest)
+            .map_err(|e| AlsaError::HwParamsError(e))?;
+
+        pcm.hw_params(&hwp).map_err(|e| AlsaError::PCMError(e))?;
+
+        let swp = pcm
+            .sw_params_current()
+            .map_err(|e| AlsaError::PCMError(e))?;
 
-        let swp = pcm.sw_params_current()?;
         // Don't assume we got what we wanted.
         // Ask to make sure.
-        let frames_per_period = hwp.get_period_size()?;
+        let frames_per_period = hwp
+            .get_period_size()
+            .map_err(|e| AlsaError::HwParamsError(e))?;
 
-        swp.set_start_threshold(hwp.get_buffer_size()? - frames_per_period)?;
+        let frames_per_buffer = hwp
-        pcm.sw_params(&swp)?;
+            .get_buffer_size()
+            .map_err(|e| AlsaError::HwParamsError(e))?;
+
+        swp.set_start_threshold(frames_per_buffer - frames_per_period)
+            .map_err(|e| AlsaError::SwParamsError(e))?;
+
+        pcm.sw_params(&swp).map_err(|e| AlsaError::PCMError(e))?;
 
         // Let ALSA do the math for us.
         pcm.frames_to_bytes(frames_per_period) as usize
@@ -83,19 +171,23 @@ fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), Box<
 
 impl Open for AlsaSink {
     fn open(device: Option<String>, format: AudioFormat) -> Self {
-        info!("Using Alsa sink with format: {:?}", format);
-
         let name = match device.as_deref() {
-            Some("?") => {
+            Some("?") => match list_outputs() {
-                println!("Listing available Alsa outputs:");
+                Ok(_) => {
-                list_outputs();
+                    exit(0);
-                exit(0)
                 }
+                Err(err) => {
+                    error!("Error listing Alsa outputs, {}", err);
+                    exit(1);
+                }
+            },
             Some(device) => device,
             None => "default",
         }
         .to_string();
 
+        info!("Using AlsaSink with format: {:?}", format);
+
         Self {
             pcm: None,
             format,
@@ -108,18 +200,13 @@ impl Open for AlsaSink {
 impl Sink for AlsaSink {
     fn start(&mut self) -> io::Result<()> {
         if self.pcm.is_none() {
-            let pcm = open_device(&self.device, self.format);
+            match open_device(&self.device, self.format) {
-            match pcm {
+                Ok((pcm, bytes_per_period)) => {
-                Ok((p, bytes_per_period)) => {
+                    self.pcm = Some(pcm);
-                    self.pcm = Some(p);
                     self.period_buffer = Vec::with_capacity(bytes_per_period);
                 }
                 Err(e) => {
-                    error!("Alsa error PCM open {}", e);
+                    return Err(io::Error::new(io::ErrorKind::Other, e));
-                    return Err(io::Error::new(
-                        io::ErrorKind::Other,
-                        "Alsa error: PCM open failed",
-                    ));
                 }
             }
         }
@@ -131,9 +218,17 @@ impl Sink for AlsaSink {
         {
             // Write any leftover data in the period buffer
             // before draining the actual buffer
-            self.write_bytes(&[]).expect("could not flush buffer");
+            self.write_bytes(&[])?;
-            let pcm = self.pcm.as_mut().unwrap();
+            let pcm = self.pcm.as_mut().ok_or(io::Error::new(
-            pcm.drain().unwrap();
+                io::ErrorKind::Other,
+                "Error stopping AlsaSink, PCM is None",
+            ))?;
+            pcm.drain().map_err(|e| {
+                io::Error::new(
+                    io::ErrorKind::Other,
+                    format!("Error stopping AlsaSink {}", e),
+                )
+            })?
         }
         self.pcm = None;
         Ok(())
@@ -154,7 +249,7 @@ impl SinkAsBytes for AlsaSink {
                 .extend_from_slice(&data[processed_data..processed_data + data_to_buffer]);
             processed_data += data_to_buffer;
             if self.period_buffer.len() == self.period_buffer.capacity() {
-                self.write_buf();
+                self.write_buf()?;
                 self.period_buffer.clear();
             }
         }
@@ -166,12 +261,30 @@ impl SinkAsBytes for AlsaSink {
 impl AlsaSink {
     pub const NAME: &'static str = "alsa";
 
-    fn write_buf(&mut self) {
+    fn write_buf(&mut self) -> io::Result<()> {
-        let pcm = self.pcm.as_mut().unwrap();
+        let pcm = self.pcm.as_mut().ok_or(io::Error::new(
+            io::ErrorKind::Other,
+            "Error writing from AlsaSink buffer to PCM, PCM is None",
+        ))?;
         let io = pcm.io_bytes();
-        match io.writei(&self.period_buffer) {
+        if let Err(err) = io.writei(&self.period_buffer) {
-            Ok(_) => (),
+            // Capture and log the original error as a warning, and then try to recover.
-            Err(err) => pcm.try_recover(err, false).unwrap(),
+            // If recovery fails then forward that error back to player.
-        };
+            warn!(
+                "Error writing from AlsaSink buffer to PCM, trying to recover {}",
+                err
+            );
+            pcm.try_recover(err, false).map_err(|e| {
+                io::Error::new(
+                    io::ErrorKind::Other,
+                    format!(
+                        "Error writing from AlsaSink buffer to PCM, recovery failed {}",
+                        e
+                    ),
+                )
+            })?
+        }
+
+        Ok(())
     }
 }

playback/src/player.rs

@@ -2,6 +2,7 @@ use std::cmp::max;
 use std::future::Future;
 use std::io::{self, Read, Seek, SeekFrom};
 use std::pin::Pin;
+use std::process::exit;
 use std::task::{Context, Poll};
 use std::time::{Duration, Instant};
 use std::{mem, thread};
@@ -1057,7 +1058,10 @@ impl PlayerInternal {
             }
             match self.sink.start() {
                 Ok(()) => self.sink_status = SinkStatus::Running,
-                Err(err) => error!("Could not start audio: {}", err),
+                Err(err) => {
+                    error!("Fatal error, could not start audio sink: {}", err);
+                    exit(1);
+                }
             }
         }
     }
@@ -1066,7 +1070,8 @@ impl PlayerInternal {
         match self.sink_status {
             SinkStatus::Running => {
                 trace!("== Stopping sink ==");
-                self.sink.stop().unwrap();
+                match self.sink.stop() {
+                    Ok(()) => {
                         self.sink_status = if temporarily {
                             SinkStatus::TemporarilyClosed
                         } else {
@@ -1076,6 +1081,12 @@ impl PlayerInternal {
                             callback(self.sink_status);
                         }
                     }
+                    Err(err) => {
+                        error!("Fatal error, could not stop audio sink: {}", err);
+                        exit(1);
+                    }
+                }
+            }
             SinkStatus::TemporarilyClosed => {
                 if !temporarily {
                     self.sink_status = SinkStatus::Closed;
@@ -1294,8 +1305,8 @@ impl PlayerInternal {
                     }
 
                     if let Err(err) = self.sink.write(&packet, &mut self.converter) {
-                        error!("Could not write audio: {}", err);
+                        error!("Fatal error, could not write audio to audio sink: {}", err);
-                        self.ensure_sink_stopped(false);
+                        exit(1);
                     }
                 }
             }