Improve player (#823)

* Improve error handling
* Harmonize `Seek`: Make the decoders and player use the same math for converting between samples and milliseconds
* Reduce duplicate calls: Make decoder seek in PCM, not ms
* Simplify decoder errors with `thiserror`

Author: JasonLG1979

CHANGELOG.md

@@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [playback] Add `--normalisation-type auto` that switches between album and track automatically
 
 ### Changed
-- [audio, playback] Moved `VorbisDecoder`, `VorbisError`, `AudioPacket`, `PassthroughDecoder`, `PassthroughError`, `AudioError`, `AudioDecoder` and the `convert` module from `librespot-audio` to `librespot-playback`. The underlying crates `vorbis`, `librespot-tremor`, `lewton` and `ogg` should be used directly. (breaking)
+- [audio, playback] Moved `VorbisDecoder`, `VorbisError`, `AudioPacket`, `PassthroughDecoder`, `PassthroughError`, `DecoderError`, `AudioDecoder` and the `convert` module from `librespot-audio` to `librespot-playback`. The underlying crates `vorbis`, `librespot-tremor`, `lewton` and `ogg` should be used directly. (breaking)
 - [audio, playback] Use `Duration` for time constants and functions (breaking)
 - [connect, playback] Moved volume controls from `librespot-connect` to `librespot-playback` crate
 - [connect] Synchronize player volume with mixer volume on playback

playback/Cargo.toml

@@ -25,6 +25,7 @@ byteorder = "1.4"
 shell-words = "1.0.0"
 tokio = { version = "1", features = ["sync"] }
 zerocopy = { version = "0.3" }
+thiserror = { version = "1" }
 
 # Backends
 alsa            = { version = "0.5", optional = true }
@@ -40,7 +41,6 @@ glib            = { version = "0.10", optional = true }
 # Rodio dependencies
 rodio           = { version = "0.14", optional = true, default-features = false }
 cpal            = { version = "0.13", optional = true }
-thiserror       = { version = "1", optional = true }
 
 # Decoder
 lewton = "0.10"
@@ -51,11 +51,11 @@ rand = "0.8"
 rand_distr = "0.4"
 
 [features]
-alsa-backend = ["alsa", "thiserror"]
+alsa-backend = ["alsa"]
 portaudio-backend = ["portaudio-rs"]
-pulseaudio-backend = ["libpulse-binding", "libpulse-simple-binding", "thiserror"]
+pulseaudio-backend = ["libpulse-binding", "libpulse-simple-binding"]
 jackaudio-backend = ["jack"]
-rodio-backend = ["rodio", "cpal", "thiserror"]
-rodiojack-backend = ["rodio", "cpal/jack", "thiserror"]
+rodio-backend = ["rodio", "cpal"]
+rodiojack-backend = ["rodio", "cpal/jack"]
 sdl-backend = ["sdl2"]
 gstreamer-backend = ["gstreamer", "gstreamer-app", "glib"]

playback/src/audio_backend/jackaudio.rs

@@ -71,7 +71,10 @@ impl Open for JackSink {
 
 impl Sink for JackSink {
     fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> io::Result<()> {
-        let samples_f32: &[f32] = &converter.f64_to_f32(packet.samples());
+        let samples = packet
+            .samples()
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
+        let samples_f32: &[f32] = &converter.f64_to_f32(samples);
         for sample in samples_f32.iter() {
             let res = self.send.send(*sample);
             if res.is_err() {

playback/src/audio_backend/portaudio.rs

@@ -148,7 +148,10 @@ impl<'a> Sink for PortAudioSink<'a> {
             };
         }
 
-        let samples = packet.samples();
+        let samples = packet
+            .samples()
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
+
         let result = match self {
             Self::F32(stream, _parameters) => {
                 let samples_f32: &[f32] = &converter.f64_to_f32(samples);

playback/src/audio_backend/rodio.rs

@@ -176,7 +176,9 @@ pub fn open(host: cpal::Host, device: Option<String>, format: AudioFormat) -> Ro
 
 impl Sink for RodioSink {
     fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> io::Result<()> {
-        let samples = packet.samples();
+        let samples = packet
+            .samples()
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
         match self.format {
             AudioFormat::F32 => {
                 let samples_f32: &[f32] = &converter.f64_to_f32(samples);

playback/src/audio_backend/sdl.rs

@@ -92,7 +92,9 @@ impl Sink for SdlSink {
             }};
         }
 
-        let samples = packet.samples();
+        let samples = packet
+            .samples()
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
         match self {
             Self::F32(queue) => {
                 let samples_f32: &[f32] = &converter.f64_to_f32(samples);

playback/src/decoder/lewton_decoder.rs

@@ -1,74 +1,46 @@
-use super::{AudioDecoder, AudioError, AudioPacket};
+use super::{AudioDecoder, AudioPacket, DecoderError, DecoderResult};
 
+use lewton::audio::AudioReadError::AudioIsHeader;
 use lewton::inside_ogg::OggStreamReader;
 use lewton::samples::InterleavedSamples;
+use lewton::OggReadError::NoCapturePatternFound;
+use lewton::VorbisError::{BadAudio, OggError};
 
-use std::error;
-use std::fmt;
 use std::io::{Read, Seek};
-use std::time::Duration;
 
 pub struct VorbisDecoder<R: Read + Seek>(OggStreamReader<R>);
-pub struct VorbisError(lewton::VorbisError);
 
 impl<R> VorbisDecoder<R>
 where
     R: Read + Seek,
 {
-    pub fn new(input: R) -> Result<VorbisDecoder<R>, VorbisError> {
-        Ok(VorbisDecoder(OggStreamReader::new(input)?))
+    pub fn new(input: R) -> DecoderResult<VorbisDecoder<R>> {
+        let reader =
+            OggStreamReader::new(input).map_err(|e| DecoderError::LewtonDecoder(e.to_string()))?;
+        Ok(VorbisDecoder(reader))
     }
 }
 
 impl<R> AudioDecoder for VorbisDecoder<R>
 where
     R: Read + Seek,
 {
-    fn seek(&mut self, ms: i64) -> Result<(), AudioError> {
-        let absgp = Duration::from_millis(ms as u64 * crate::SAMPLE_RATE as u64).as_secs();
-        match self.0.seek_absgp_pg(absgp as u64) {
-            Ok(_) => Ok(()),
-            Err(err) => Err(AudioError::VorbisError(err.into())),
-        }
+    fn seek(&mut self, absgp: u64) -> DecoderResult<()> {
+        self.0
+            .seek_absgp_pg(absgp)
+            .map_err(|e| DecoderError::LewtonDecoder(e.to_string()))?;
+        Ok(())
     }
 
-    fn next_packet(&mut self) -> Result<Option<AudioPacket>, AudioError> {
-        use lewton::audio::AudioReadError::AudioIsHeader;
-        use lewton::OggReadError::NoCapturePatternFound;
-        use lewton::VorbisError::{BadAudio, OggError};
+    fn next_packet(&mut self) -> DecoderResult<Option<AudioPacket>> {
         loop {
             match self.0.read_dec_packet_generic::<InterleavedSamples<f32>>() {
                 Ok(Some(packet)) => return Ok(Some(AudioPacket::samples_from_f32(packet.samples))),
                 Ok(None) => return Ok(None),
-
                 Err(BadAudio(AudioIsHeader)) => (),
                 Err(OggError(NoCapturePatternFound)) => (),
-                Err(err) => return Err(AudioError::VorbisError(err.into())),
+                Err(e) => return Err(DecoderError::LewtonDecoder(e.to_string())),
             }
         }
     }
 }
-
-impl From<lewton::VorbisError> for VorbisError {
-    fn from(err: lewton::VorbisError) -> VorbisError {
-        VorbisError(err)
-    }
-}
-
-impl fmt::Debug for VorbisError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Debug::fmt(&self.0, f)
-    }
-}
-
-impl fmt::Display for VorbisError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Display::fmt(&self.0, f)
-    }
-}
-
-impl error::Error for VorbisError {
-    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
-        error::Error::source(&self.0)
-    }
-}

playback/src/decoder/mod.rs

@@ -1,10 +1,30 @@
-use std::fmt;
+use thiserror::Error;
 
 mod lewton_decoder;
-pub use lewton_decoder::{VorbisDecoder, VorbisError};
+pub use lewton_decoder::VorbisDecoder;
 
 mod passthrough_decoder;
-pub use passthrough_decoder::{PassthroughDecoder, PassthroughError};
+pub use passthrough_decoder::PassthroughDecoder;
+
+#[derive(Error, Debug)]
+pub enum DecoderError {
+    #[error("Lewton Decoder Error: {0}")]
+    LewtonDecoder(String),
+    #[error("Passthrough Decoder Error: {0}")]
+    PassthroughDecoder(String),
+}
+
+pub type DecoderResult<T> = Result<T, DecoderError>;
+
+#[derive(Error, Debug)]
+pub enum AudioPacketError {
+    #[error("Decoder OggData Error: Can't return OggData on Samples")]
+    OggData,
+    #[error("Decoder Samples Error: Can't return Samples on OggData")]
+    Samples,
+}
+
+pub type AudioPacketResult<T> = Result<T, AudioPacketError>;
 
 pub enum AudioPacket {
     Samples(Vec<f64>),
@@ -17,17 +37,17 @@ impl AudioPacket {
         AudioPacket::Samples(f64_samples)
     }
 
-    pub fn samples(&self) -> &[f64] {
+    pub fn samples(&self) -> AudioPacketResult<&[f64]> {
         match self {
-            AudioPacket::Samples(s) => s,
-            AudioPacket::OggData(_) => panic!("can't return OggData on samples"),
+            AudioPacket::Samples(s) => Ok(s),
+            AudioPacket::OggData(_) => Err(AudioPacketError::OggData),
         }
     }
 
-    pub fn oggdata(&self) -> &[u8] {
+    pub fn oggdata(&self) -> AudioPacketResult<&[u8]> {
         match self {
-            AudioPacket::Samples(_) => panic!("can't return samples on OggData"),
-            AudioPacket::OggData(d) => d,
+            AudioPacket::OggData(d) => Ok(d),
+            AudioPacket::Samples(_) => Err(AudioPacketError::Samples),
         }
     }
 
@@ -39,34 +59,7 @@ impl AudioPacket {
     }
 }
 
-#[derive(Debug)]
-pub enum AudioError {
-    PassthroughError(PassthroughError),
-    VorbisError(VorbisError),
-}
-
-impl fmt::Display for AudioError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            AudioError::PassthroughError(err) => write!(f, "PassthroughError({})", err),
-            AudioError::VorbisError(err) => write!(f, "VorbisError({})", err),
-        }
-    }
-}
-
-impl From<VorbisError> for AudioError {
-    fn from(err: VorbisError) -> AudioError {
-        AudioError::VorbisError(err)
-    }
-}
-
-impl From<PassthroughError> for AudioError {
-    fn from(err: PassthroughError) -> AudioError {
-        AudioError::PassthroughError(err)
-    }
-}
-
 pub trait AudioDecoder {
-    fn seek(&mut self, ms: i64) -> Result<(), AudioError>;
-    fn next_packet(&mut self) -> Result<Option<AudioPacket>, AudioError>;
+    fn seek(&mut self, absgp: u64) -> DecoderResult<()>;
+    fn next_packet(&mut self) -> DecoderResult<Option<AudioPacket>>;
 }