Implement common SinkError and SinkResult (#820)

* Make error messages more consistent and concise.

* `impl From<AlsaError> for io::Error` so `AlsaErrors` can be thrown to player as `io::Errors`. This little bit of boilerplate goes a long way to simplifying things further down in the code. And will make any needed future changes easier.

* Bonus: handle ALSA backend buffer sizing a little better.

Author: JasonLG1979

playback/src/audio_backend/alsa.rs

@@ -1,4 +1,4 @@
-use super::{Open, Sink, SinkAsBytes};
+use super::{Open, Sink, SinkAsBytes, SinkError, SinkResult};
 use crate::config::AudioFormat;
 use crate::convert::Converter;
 use crate::decoder::AudioPacket;
@@ -7,7 +7,6 @@ use alsa::device_name::HintIter;
 use alsa::pcm::{Access, Format, HwParams, PCM};
 use alsa::{Direction, ValueOr};
 use std::cmp::min;
-use std::io;
 use std::process::exit;
 use std::time::Duration;
 use thiserror::Error;
@@ -18,34 +17,67 @@ const BUFFER_TIME: Duration = Duration::from_millis(500);
 
 #[derive(Debug, Error)]
 enum AlsaError {
-    #[error("AlsaSink, device {device} may be invalid or busy, {err}")]
-    PcmSetUp { device: String, err: alsa::Error },
-    #[error("AlsaSink, device {device} unsupported access type RWInterleaved, {err}")]
-    UnsupportedAccessType { device: String, err: alsa::Error },
-    #[error("AlsaSink, device {device} unsupported format {format:?}, {err}")]
+    #[error("<AlsaSink> Device {device} Unsupported Format {alsa_format:?} ({format:?}), {e}")]
     UnsupportedFormat {
         device: String,
+        alsa_format: Format,
         format: AudioFormat,
-        err: alsa::Error,
+        e: alsa::Error,
     },
-    #[error("AlsaSink, device {device} unsupported sample rate {samplerate}, {err}")]
-    UnsupportedSampleRate {
-        device: String,
-        samplerate: u32,
-        err: alsa::Error,
-    },
-    #[error("AlsaSink, device {device} unsupported channel count {channel_count}, {err}")]
+
+    #[error("<AlsaSink> Device {device} Unsupported Channel Count {channel_count}, {e}")]
     UnsupportedChannelCount {
         device: String,
         channel_count: u8,
-        err: alsa::Error,
+        e: alsa::Error,
+    },
+
+    #[error("<AlsaSink> Device {device} Unsupported Sample Rate {samplerate}, {e}")]
+    UnsupportedSampleRate {
+        device: String,
+        samplerate: u32,
+        e: alsa::Error,
     },
-    #[error("AlsaSink Hardware Parameters Error, {0}")]
+
+    #[error("<AlsaSink> Device {device} Unsupported Access Type RWInterleaved, {e}")]
+    UnsupportedAccessType { device: String, e: alsa::Error },
+
+    #[error("<AlsaSink> Device {device} May be Invalid, Busy, or Already in Use, {e}")]
+    PcmSetUp { device: String, e: alsa::Error },
+
+    #[error("<AlsaSink> Failed to Drain PCM Buffer, {0}")]
+    DrainFailure(alsa::Error),
+
+    #[error("<AlsaSink> {0}")]
+    OnWrite(alsa::Error),
+
+    #[error("<AlsaSink> Hardware, {0}")]
     HwParams(alsa::Error),
-    #[error("AlsaSink Software Parameters Error, {0}")]
+
+    #[error("<AlsaSink> Software, {0}")]
     SwParams(alsa::Error),
-    #[error("AlsaSink PCM Error, {0}")]
+
+    #[error("<AlsaSink> PCM, {0}")]
     Pcm(alsa::Error),
+
+    #[error("<AlsaSink> Could Not Parse Ouput Name(s) and/or Description(s)")]
+    Parsing,
+
+    #[error("<AlsaSink>")]
+    NotConnected,
+}
+
+impl From<AlsaError> for SinkError {
+    fn from(e: AlsaError) -> SinkError {
+        use AlsaError::*;
+        let es = e.to_string();
+        match e {
+            DrainFailure(_) | OnWrite(_) => SinkError::OnWrite(es),
+            PcmSetUp { .. } => SinkError::ConnectionRefused(es),
+            NotConnected => SinkError::NotConnected(es),
+            _ => SinkError::InvalidParams(es),
+        }
+    }
 }
 
 pub struct AlsaSink {
@@ -55,25 +87,19 @@ pub struct AlsaSink {
     period_buffer: Vec<u8>,
 }
 
-fn list_outputs() -> io::Result<()> {
+fn list_outputs() -> SinkResult<()> {
     println!("Listing available Alsa outputs:");
     for t in &["pcm", "ctl", "hwdep"] {
         println!("{} devices:", t);
-        let i = match HintIter::new_str(None, t) {
-            Ok(i) => i,
-            Err(e) => {
-                return Err(io::Error::new(io::ErrorKind::Other, e));
-            }
-        };
+
+        let i = HintIter::new_str(None, &t).map_err(|_| AlsaError::Parsing)?;
+
         for a in i {
             if let Some(Direction::Playback) = a.direction {
                 // mimic aplay -L
-                let name = a
-                    .name
-                    .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Could not parse name"))?;
-                let desc = a
-                    .desc
-                    .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Could not parse desc"))?;
+                let name = a.name.ok_or(AlsaError::Parsing)?;
+                let desc = a.desc.ok_or(AlsaError::Parsing)?;
+
                 println!("{}\n\t{}\n", name, desc.replace("\n", "\n\t"));
             }
         }
@@ -82,10 +108,10 @@ fn list_outputs() -> io::Result<()> {
     Ok(())
 }
 
-fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), AlsaError> {
+fn open_device(dev_name: &str, format: AudioFormat) -> SinkResult<(PCM, usize)> {
     let pcm = PCM::new(dev_name, Direction::Playback, false).map_err(|e| AlsaError::PcmSetUp {
         device: dev_name.to_string(),
-        err: e,
+        e,
     })?;
 
     let alsa_format = match format {
@@ -103,32 +129,34 @@ fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), Alsa
 
     let bytes_per_period = {
         let hwp = HwParams::any(&pcm).map_err(AlsaError::HwParams)?;
+
         hwp.set_access(Access::RWInterleaved)
             .map_err(|e| AlsaError::UnsupportedAccessType {
                 device: dev_name.to_string(),
-                err: e,
+                e,
             })?;
 
         hwp.set_format(alsa_format)
             .map_err(|e| AlsaError::UnsupportedFormat {
                 device: dev_name.to_string(),
+                alsa_format,
                 format,
-                err: e,
+                e,
             })?;
 
         hwp.set_rate(SAMPLE_RATE, ValueOr::Nearest).map_err(|e| {
             AlsaError::UnsupportedSampleRate {
                 device: dev_name.to_string(),
                 samplerate: SAMPLE_RATE,
-                err: e,
+                e,
             }
         })?;
 
         hwp.set_channels(NUM_CHANNELS as u32)
             .map_err(|e| AlsaError::UnsupportedChannelCount {
                 device: dev_name.to_string(),
                 channel_count: NUM_CHANNELS,
-                err: e,
+                e,
             })?;
 
         hwp.set_buffer_time_near(BUFFER_TIME.as_micros() as u32, ValueOr::Nearest)
@@ -141,8 +169,7 @@ fn open_device(dev_name: &str, format: AudioFormat) -> Result<(PCM, usize), Alsa
 
         let swp = pcm.sw_params_current().map_err(AlsaError::Pcm)?;
 
-        // Don't assume we got what we wanted.
-        // Ask to make sure.
+        // 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)?;
@@ -171,8 +198,8 @@ impl Open for AlsaSink {
                 Ok(_) => {
                     exit(0);
                 }
-                Err(err) => {
-                    error!("Error listing Alsa outputs, {}", err);
+                Err(e) => {
+                    error!("{}", e);
                     exit(1);
                 }
             },
@@ -193,53 +220,40 @@ impl Open for AlsaSink {
 }
 
 impl Sink for AlsaSink {
-    fn start(&mut self) -> io::Result<()> {
+    fn start(&mut self) -> SinkResult<()> {
         if self.pcm.is_none() {
-            match open_device(&self.device, self.format) {
-                Ok((pcm, bytes_per_period)) => {
-                    self.pcm = Some(pcm);
-                    // If the capacity is greater than we want shrink it
-                    // to it's current len (which should be zero) before
-                    // setting the capacity with reserve_exact.
-                    if self.period_buffer.capacity() > bytes_per_period {
-                        self.period_buffer.shrink_to_fit();
-                    }
-                    // This does nothing if the capacity is already sufficient.
-                    // Len should always be zero, but for the sake of being thorough...
-                    self.period_buffer
-                        .reserve_exact(bytes_per_period - self.period_buffer.len());
-
-                    // Should always match the "Period Buffer size in bytes: " trace! message.
-                    trace!(
-                        "Period Buffer capacity: {:?}",
-                        self.period_buffer.capacity()
-                    );
-                }
-                Err(e) => {
-                    return Err(io::Error::new(io::ErrorKind::Other, e));
-                }
+            let (pcm, bytes_per_period) = open_device(&self.device, self.format)?;
+            self.pcm = Some(pcm);
+
+            let current_capacity = self.period_buffer.capacity();
+
+            if current_capacity > bytes_per_period {
+                self.period_buffer.truncate(bytes_per_period);
+                self.period_buffer.shrink_to_fit();
+            } else if current_capacity < bytes_per_period {
+                let extra = bytes_per_period - self.period_buffer.len();
+                self.period_buffer.reserve_exact(extra);
             }
+
+            // Should always match the "Period Buffer size in bytes: " trace! message.
+            trace!(
+                "Period Buffer capacity: {:?}",
+                self.period_buffer.capacity()
+            );
         }
 
         Ok(())
     }
 
-    fn stop(&mut self) -> io::Result<()> {
+    fn stop(&mut self) -> SinkResult<()> {
         // Zero fill the remainder of the period buffer and
         // write any leftover data before draining the actual PCM buffer.
         self.period_buffer.resize(self.period_buffer.capacity(), 0);
         self.write_buf()?;
 
-        let pcm = self.pcm.as_mut().ok_or_else(|| {
-            io::Error::new(io::ErrorKind::Other, "Error stopping AlsaSink, PCM is None")
-        })?;
+        let pcm = self.pcm.as_mut().ok_or(AlsaError::NotConnected)?;
 
-        pcm.drain().map_err(|e| {
-            io::Error::new(
-                io::ErrorKind::Other,
-                format!("Error stopping AlsaSink {}", e),
-            )
-        })?;
+        pcm.drain().map_err(AlsaError::DrainFailure)?;
 
         self.pcm = None;
         Ok(())
@@ -249,23 +263,28 @@ impl Sink for AlsaSink {
 }
 
 impl SinkAsBytes for AlsaSink {
-    fn write_bytes(&mut self, data: &[u8]) -> io::Result<()> {
+    fn write_bytes(&mut self, data: &[u8]) -> SinkResult<()> {
         let mut start_index = 0;
         let data_len = data.len();
         let capacity = self.period_buffer.capacity();
+
         loop {
             let data_left = data_len - start_index;
             let space_left = capacity - self.period_buffer.len();
             let data_to_buffer = min(data_left, space_left);
             let end_index = start_index + data_to_buffer;
+
             self.period_buffer
                 .extend_from_slice(&data[start_index..end_index]);
+
             if self.period_buffer.len() == capacity {
                 self.write_buf()?;
             }
+
             if end_index == data_len {
                 break Ok(());
             }
+
             start_index = end_index;
         }
     }
@@ -274,30 +293,18 @@ impl SinkAsBytes for AlsaSink {
 impl AlsaSink {
     pub const NAME: &'static str = "alsa";
 
-    fn write_buf(&mut self) -> io::Result<()> {
-        let pcm = self.pcm.as_mut().ok_or_else(|| {
-            io::Error::new(
-                io::ErrorKind::Other,
-                "Error writing from AlsaSink buffer to PCM, PCM is None",
-            )
-        })?;
-        let io = pcm.io_bytes();
-        if let Err(err) = io.writei(&self.period_buffer) {
+    fn write_buf(&mut self) -> SinkResult<()> {
+        let pcm = self.pcm.as_mut().ok_or(AlsaError::NotConnected)?;
+
+        if let Err(e) = pcm.io_bytes().writei(&self.period_buffer) {
             // Capture and log the original error as a warning, and then try to recover.
             // If recovery fails then forward that error back to player.
             warn!(
-                "Error writing from AlsaSink buffer to PCM, trying to recover {}",
-                err
+                "Error writing from AlsaSink buffer to PCM, trying to recover, {}",
+                e
             );
-            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
-                    ),
-                )
-            })?
+
+            pcm.try_recover(e, false).map_err(AlsaError::OnWrite)?
         }
 
         self.period_buffer.clear();

playback/src/audio_backend/gstreamer.rs

@@ -1,4 +1,4 @@
-use super::{Open, Sink, SinkAsBytes};
+use super::{Open, Sink, SinkAsBytes, SinkResult};
 use crate::config::AudioFormat;
 use crate::convert::Converter;
 use crate::decoder::AudioPacket;
@@ -11,7 +11,7 @@ use gst::prelude::*;
 use zerocopy::AsBytes;
 
 use std::sync::mpsc::{sync_channel, SyncSender};
-use std::{io, thread};
+use std::thread;
 
 #[allow(dead_code)]
 pub struct GstreamerSink {
@@ -131,7 +131,7 @@ impl Sink for GstreamerSink {
 }
 
 impl SinkAsBytes for GstreamerSink {
-    fn write_bytes(&mut self, data: &[u8]) -> io::Result<()> {
+    fn write_bytes(&mut self, data: &[u8]) -> SinkResult<()> {
         // Copy expensively (in to_vec()) to avoid thread synchronization
         self.tx
             .send(data.to_vec())