From f09be4850ed1c79dc77becdb103b97bbe6a26d48 Mon Sep 17 00:00:00 2001 From: Guillaume Desmottes Date: Wed, 29 Dec 2021 16:26:24 +0100 Subject: [PATCH 1/2] Sink: pass ownership of the packet on write() Prevent a copy if the implementation needs to keep the data around. --- CHANGELOG.md | 1 + playback/src/audio_backend/jackaudio.rs | 2 +- playback/src/audio_backend/mod.rs | 16 ++++++++-------- playback/src/audio_backend/portaudio.rs | 8 ++++---- playback/src/audio_backend/rodio.rs | 2 +- playback/src/audio_backend/sdl.rs | 2 +- playback/src/player.rs | 2 +- 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d603a93..a2be0992 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [playback] `alsa`: Improve `--device ?` functionality for the alsa backend. - [contrib] Hardened security of the systemd service units - [main] Verbose logging mode (`-v`, `--verbose`) now logs all parsed environment variables and command line arguments (credentials are redacted). +- [playback] `Sink`: `write()` now receives ownership of the packet (breaking). ### Added - [cache] Add `disable-credential-cache` flag (breaking). diff --git a/playback/src/audio_backend/jackaudio.rs b/playback/src/audio_backend/jackaudio.rs index 15acf99d..b4d24949 100644 --- a/playback/src/audio_backend/jackaudio.rs +++ b/playback/src/audio_backend/jackaudio.rs @@ -66,7 +66,7 @@ impl Open for JackSink { } impl Sink for JackSink { - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()> { + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()> { let samples = packet .samples() .map_err(|e| SinkError::OnWrite(e.to_string()))?; diff --git a/playback/src/audio_backend/mod.rs b/playback/src/audio_backend/mod.rs index dc21fb3d..6c903d22 100644 --- a/playback/src/audio_backend/mod.rs +++ b/playback/src/audio_backend/mod.rs @@ -28,7 +28,7 @@ pub trait Sink { fn stop(&mut self) -> SinkResult<()> { Ok(()) } - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()>; + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()>; } pub type SinkBuilder = fn(Option, AudioFormat) -> Box; @@ -44,34 +44,34 @@ fn mk_sink(device: Option, format: AudioFormat // reuse code for various backends macro_rules! sink_as_bytes { () => { - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()> { + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()> { use crate::convert::i24; use zerocopy::AsBytes; match packet { AudioPacket::Samples(samples) => match self.format { AudioFormat::F64 => self.write_bytes(samples.as_bytes()), AudioFormat::F32 => { - let samples_f32: &[f32] = &converter.f64_to_f32(samples); + let samples_f32: &[f32] = &converter.f64_to_f32(&samples); self.write_bytes(samples_f32.as_bytes()) } AudioFormat::S32 => { - let samples_s32: &[i32] = &converter.f64_to_s32(samples); + let samples_s32: &[i32] = &converter.f64_to_s32(&samples); self.write_bytes(samples_s32.as_bytes()) } AudioFormat::S24 => { - let samples_s24: &[i32] = &converter.f64_to_s24(samples); + let samples_s24: &[i32] = &converter.f64_to_s24(&samples); self.write_bytes(samples_s24.as_bytes()) } AudioFormat::S24_3 => { - let samples_s24_3: &[i24] = &converter.f64_to_s24_3(samples); + let samples_s24_3: &[i24] = &converter.f64_to_s24_3(&samples); self.write_bytes(samples_s24_3.as_bytes()) } AudioFormat::S16 => { - let samples_s16: &[i16] = &converter.f64_to_s16(samples); + let samples_s16: &[i16] = &converter.f64_to_s16(&samples); self.write_bytes(samples_s16.as_bytes()) } }, - AudioPacket::OggData(samples) => self.write_bytes(samples), + AudioPacket::OggData(samples) => self.write_bytes(&samples), } } }; diff --git a/playback/src/audio_backend/portaudio.rs b/playback/src/audio_backend/portaudio.rs index 7a0b179f..12a5404d 100644 --- a/playback/src/audio_backend/portaudio.rs +++ b/playback/src/audio_backend/portaudio.rs @@ -140,7 +140,7 @@ impl<'a> Sink for PortAudioSink<'a> { Ok(()) } - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()> { + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()> { macro_rules! write_sink { (ref mut $stream: expr, $samples: expr) => { $stream.as_mut().unwrap().write($samples) @@ -153,15 +153,15 @@ impl<'a> Sink for PortAudioSink<'a> { let result = match self { Self::F32(stream, _parameters) => { - let samples_f32: &[f32] = &converter.f64_to_f32(samples); + let samples_f32: &[f32] = &converter.f64_to_f32(&samples); write_sink!(ref mut stream, samples_f32) } Self::S32(stream, _parameters) => { - let samples_s32: &[i32] = &converter.f64_to_s32(samples); + let samples_s32: &[i32] = &converter.f64_to_s32(&samples); write_sink!(ref mut stream, samples_s32) } Self::S16(stream, _parameters) => { - let samples_s16: &[i16] = &converter.f64_to_s16(samples); + let samples_s16: &[i16] = &converter.f64_to_s16(&samples); write_sink!(ref mut stream, samples_s16) } }; diff --git a/playback/src/audio_backend/rodio.rs b/playback/src/audio_backend/rodio.rs index ab356d67..9f4ad059 100644 --- a/playback/src/audio_backend/rodio.rs +++ b/playback/src/audio_backend/rodio.rs @@ -189,7 +189,7 @@ pub fn open(host: cpal::Host, device: Option, format: AudioFormat) -> Ro } impl Sink for RodioSink { - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()> { + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()> { let samples = packet .samples() .map_err(|e| RodioError::Samples(e.to_string()))?; diff --git a/playback/src/audio_backend/sdl.rs b/playback/src/audio_backend/sdl.rs index 6272fa32..1c9794a2 100644 --- a/playback/src/audio_backend/sdl.rs +++ b/playback/src/audio_backend/sdl.rs @@ -82,7 +82,7 @@ impl Sink for SdlSink { Ok(()) } - fn write(&mut self, packet: &AudioPacket, converter: &mut Converter) -> SinkResult<()> { + fn write(&mut self, packet: AudioPacket, converter: &mut Converter) -> SinkResult<()> { macro_rules! drain_sink { ($queue: expr, $size: expr) => {{ // sleep and wait for sdl thread to drain the queue a bit diff --git a/playback/src/player.rs b/playback/src/player.rs index d8dbb190..d3bc2b39 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -1384,7 +1384,7 @@ impl PlayerInternal { } } - if let Err(e) = self.sink.write(&packet, &mut self.converter) { + if let Err(e) = self.sink.write(packet, &mut self.converter) { error!("{}", e); exit(1); } From 8dfa00d66f64180e504067f81824397638eed6a8 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Sat, 1 Jan 2022 17:19:12 -0600 Subject: [PATCH 2/2] Clean up list_compatible_devices Fix a typo and be a little more forgiving. --- playback/src/audio_backend/alsa.rs | 79 +++++++++++++++++------------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/playback/src/audio_backend/alsa.rs b/playback/src/audio_backend/alsa.rs index 4f82a097..16aa420d 100644 --- a/playback/src/audio_backend/alsa.rs +++ b/playback/src/audio_backend/alsa.rs @@ -62,8 +62,8 @@ enum AlsaError { #[error(" PCM, {0}")] Pcm(alsa::Error), - #[error(" Could Not Parse Ouput Name(s) and/or Description(s)")] - Parsing, + #[error(" Could Not Parse Output Name(s) and/or Description(s), {0}")] + Parsing(alsa::Error), #[error("")] NotConnected, @@ -107,49 +107,58 @@ pub struct AlsaSink { } fn list_compatible_devices() -> SinkResult<()> { + let i = HintIter::new_str(None, "pcm").map_err(AlsaError::Parsing)?; + println!("\n\n\tCompatible alsa device(s):\n"); println!("\t------------------------------------------------------\n"); - let i = HintIter::new_str(None, "pcm").map_err(|_| AlsaError::Parsing)?; - for a in i { if let Some(Direction::Playback) = a.direction { - let name = a.name.ok_or(AlsaError::Parsing)?; - let desc = a.desc.ok_or(AlsaError::Parsing)?; + if let Some(name) = a.name { + if let Ok(pcm) = PCM::new(&name, Direction::Playback, false) { + if let Ok(hwp) = HwParams::any(&pcm) { + // Only show devices that support + // 2 ch 44.1 Interleaved. - if let Ok(pcm) = PCM::new(&name, Direction::Playback, false) { - if let Ok(hwp) = HwParams::any(&pcm) { - // Only show devices that support - // 2 ch 44.1 Interleaved. - if hwp.set_access(Access::RWInterleaved).is_ok() - && hwp.set_rate(SAMPLE_RATE, ValueOr::Nearest).is_ok() - && hwp.set_channels(NUM_CHANNELS as u32).is_ok() - { - println!("\tDevice:\n\n\t\t{}\n", name); - println!("\tDescription:\n\n\t\t{}\n", desc.replace("\n", "\n\t\t")); + if hwp.set_access(Access::RWInterleaved).is_ok() + && hwp.set_rate(SAMPLE_RATE, ValueOr::Nearest).is_ok() + && hwp.set_channels(NUM_CHANNELS as u32).is_ok() + { + let mut supported_formats = vec![]; - let mut supported_formats = vec![]; + for f in &[ + AudioFormat::S16, + AudioFormat::S24, + AudioFormat::S24_3, + AudioFormat::S32, + AudioFormat::F32, + AudioFormat::F64, + ] { + if hwp.test_format(Format::from(*f)).is_ok() { + supported_formats.push(format!("{:?}", f)); + } + } - for f in &[ - AudioFormat::S16, - AudioFormat::S24, - AudioFormat::S24_3, - AudioFormat::S32, - AudioFormat::F32, - AudioFormat::F64, - ] { - if hwp.test_format(Format::from(*f)).is_ok() { - supported_formats.push(format!("{:?}", f)); + if !supported_formats.is_empty() { + println!("\tDevice:\n\n\t\t{}\n", name); + + println!( + "\tDescription:\n\n\t\t{}\n", + a.desc.unwrap_or_default().replace("\n", "\n\t\t") + ); + + println!( + "\tSupported Format(s):\n\n\t\t{}\n", + supported_formats.join(" ") + ); + + println!( + "\t------------------------------------------------------\n" + ); } } - - println!( - "\tSupported Format(s):\n\n\t\t{}\n", - supported_formats.join(" ") - ); - println!("\t------------------------------------------------------\n"); - } - }; + }; + } } } }