Add pipewire audio backend by ggiraudon · Pull Request #1610 · librespot-org/librespot

ggiraudon · 2025-10-04T20:18:41Z

For your consideration : My implementation of a native pipewire audio-backend.

Copilot

Pull Request Overview

Adds a PipeWire audio backend to provide native support for the modern PipeWire audio server on Linux systems, offering low-latency audio playback as an alternative to existing backends.

Implements a complete PipeWire audio sink with proper stream management and error handling
Adds feature flags and dependency configuration for the new backend
Integrates the backend into the existing audio backend selection system

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

File	Description
playback/src/audio_backend/pipewire.rs	Complete PipeWire sink implementation with audio format conversion and stream handling
playback/src/audio_backend/mod.rs	Registers the PipeWire backend in the available backends list
playback/Cargo.toml	Adds pipewire and libspa dependencies for the playback crate
Cargo.toml	Defines the pipewire-backend feature and workspace-level dependencies

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-04T20:20:02Z

playback/src/audio_backend/pipewire.rs

+            for &byte in data {
+                sender.send(byte).map_err(|_| PipeWireError::NotConnected)?;
+            }


Sending audio data byte-by-byte through a channel is highly inefficient and will likely cause audio dropouts. Consider sending data in chunks or using a different approach like a ring buffer for better performance.

Copilot · 2025-10-04T20:20:02Z

playback/src/audio_backend/pipewire.rs

+        F32 => spa::param::audio::AudioFormat::F32LE,
+        S32 => spa::param::audio::AudioFormat::S32LE,
+        S24 => spa::param::audio::AudioFormat::S24LE,
+        S24_3 => spa::param::audio::AudioFormat::S24_32LE,


The audio format mapping for S24_3 appears incorrect. S24_3 typically represents 24-bit samples packed in 3 bytes, but S24_32LE represents 24-bit samples in 32-bit containers. This should likely be S24_3LE if available.

Suggested change

S24_3 => spa::param::audio::AudioFormat::S24_32LE,

S24_3 => spa::param::audio::AudioFormat::S24_3LE,

Copilot · 2025-10-04T20:20:02Z

playback/src/audio_backend/pipewire.rs

+                        for i in 0..total_bytes {
+                            match receiver.try_recv() {
+                                Ok(byte) => {
+                                    slice[i] = byte;
+                                    bytes_read += 1;
+                                }
+                                Err(_) => break,
+                            }
+                        }


Reading audio data byte-by-byte in the audio callback will cause significant performance issues and audio dropouts. The audio callback should be lock-free and operate on larger chunks of data.

Copilot · 2025-10-04T20:20:03Z

playback/src/audio_backend/pipewire.rs

+        self.initialized = true;
+
+        // Give the thread a moment to initialize
+        thread::sleep(std::time::Duration::from_millis(100));


Using a fixed sleep duration for initialization is unreliable. Consider using proper synchronization mechanisms like condition variables or status callbacks to ensure the PipeWire stream is actually ready before returning.

Copilot · 2025-10-04T20:20:03Z

playback/src/audio_backend/pipewire.rs

+        // Wait for the thread to finish with a timeout
+        if let Some(handle) = self._main_loop_handle.take() {
+            // Give it a moment to exit gracefully
+            thread::sleep(std::time::Duration::from_millis(100));


Using a fixed sleep duration before joining the thread is unreliable. The thread should signal when it's ready to exit, or use a timeout with the join operation.

roderickvd

Yes we will consider 😆 very nice addition. It needs work; also take a look at some of the Copilot suggestions which seem to make sense to me.

Don't forget to add a changelog entry too.

roderickvd · 2025-10-06T19:15:03Z

Cargo.toml

 ] }
 url = "2.2"

+# Dependencies for pipewire 


These should go to playback/Cargo.toml.

playback/src/audio_backend/pipewire.rs

ggiraudon · 2025-10-07T19:57:00Z

Hi Roderick,
I might need your assistance on some of it as I'm not an rust expert.
I tried implementing various buffering methods but always end up with choppy/unreliable playback when i do so and I'm not sure what I'm doing wrong.
On paper things should be more efficient when passing stuff in chunks instead of spoon-feeding the channels byte by byte but in practice I can't seem to get it to work.
I've created a branch called pipewire-buffer-test in my fork that is my attempt at such improvement but the playback performance is horrible (choppy at random) even when compiled in release mode.
The current implementation in the dev branch seems to be the one behaving the best so far but I know its not optimal.

roderickvd · 2025-10-07T20:08:40Z

First a bit of a low-effort but hopefully high-impact reply: does looking at RustAudio/cpal#938 help? That implementation is not perfect, but the use of pipewire::buffer::Buffer seems interesting.

In your fork I see that you're pushing a Vec<u8> through a sync channel. I suspect that will have poor performance too. You may want to look like at using a ring buffer (ringbuf crate or rtrb if you don't need to work across threads).

Hope this helps.

roderickvd

Sorry for the late feedback. Here is a further review.
Also if you would add a changelog entry?

roderickvd · 2025-11-07T17:34:07Z

playback/src/audio_backend/pipewire.rs

+}
+
+impl Open for PipeWireSink {
+    fn open(_device: Option<String>, format: AudioFormat) -> Self {


I think we could not ignore device and around line 238 where you create the stream add:

*pw::keys::TARGET_OBJECT => device.as_deref().unwrap_or("")

Probably it would then also need:

logic to use the "default" device (skipping TARGET_OBJECT?)

support for object enumeration

Point 2 may be complex so for this PR we could also device to leave enumeration for a next PR and just accept a device parameter now.

roderickvd · 2025-11-07T17:40:11Z

playback/src/audio_backend/pipewire.rs

+        if let Some(handle) = self._main_loop_handle.take() {
+            // Give it a moment to exit gracefully
+            thread::sleep(std::time::Duration::from_millis(100));
+            let _ = handle.join();


May want to propagate any errors.

roderickvd · 2025-11-07T17:40:37Z

playback/src/audio_backend/pipewire.rs

+        // Wait for the thread to finish with a timeout
+        if let Some(handle) = self._main_loop_handle.take() {
+            // Give it a moment to exit gracefully
+            thread::sleep(std::time::Duration::from_millis(100));


What happens if we don't?

roderickvd · 2025-11-07T17:41:05Z

playback/src/audio_backend/pipewire.rs

+        info!("Stopping PipeWire sink...");
+
+        // Signal the thread to quit
+        self.quit_flag.store(true, Ordering::Relaxed);


For safety we may want to consider stronger ordering here, like Ordering::Release, because below there's some operations and then you're resetting the quit_flag again - if it matters that this truly happens after the operations...

roderickvd · 2025-11-07T17:41:32Z

playback/src/audio_backend/pipewire.rs

+        .add_local_listener_with_user_data((consumer, quit_flag.clone()))
+        .process(move |stream, (consumer, quit_flag)| {
+            // Check if we should quit
+            if quit_flag.load(Ordering::Relaxed) {


Ordering::Acquire?

roderickvd · 2025-11-07T17:43:26Z

playback/src/audio_backend/pipewire.rs

+                        let bytes_read = consumer.pop_slice(slice);
+
+                        // Fill any remaining space with silence if underrun
+                        if bytes_read < total_bytes {


Consider adding logging in such cases:

if bytes_read < total_bytes { warn!("Buffer underrun: requested {} bytes, got {}", total_bytes, bytes_read); slice[bytes_read..total_bytes].fill(0); }

Makes it easier to debug.

roderickvd · 2025-11-07T17:44:31Z

playback/src/audio_backend/pipewire.rs

+
+                        // Fill any remaining space with silence if underrun
+                        if bytes_read < total_bytes {
+                            slice[bytes_read..total_bytes].fill(0);


For F32 and F64 formats, I'm not sure if u8 byte values of 0 are equal to 0.0 in float.

roderickvd · 2025-11-07T17:51:15Z

playback/src/audio_backend/pipewire.rs

+        self.initialized = true;
+
+        // Give the thread a moment to initialize
+        thread::sleep(std::time::Duration::from_millis(100));


Better to use a channel according to this pattern:

// In the struct: pub struct PipeWireSink { // ... existing fields ... init_rx: Option<oneshot::Receiver<Result<(), String>>>, } // In start(): let (init_tx, init_rx) = oneshot::channel(); self.init_rx = Some(init_rx); let handle = thread::spawn(move || { match initialize_pipewire_stream(...) { Ok(stream) => { let _ = init_tx.send(Ok(())); run_main_loop(stream, ...); } Err(e) => { let _ = init_tx.send(Err(e.to_string())); } } }); // Wait for initialization with timeout: match self.init_rx.take().unwrap().recv_timeout(Duration::from_secs(1)) { Ok(Ok(())) => { /* success */ }, Ok(Err(e)) => return Err(...), Err(_) => return Err(timeout_error), }

roderickvd · 2025-11-07T17:51:41Z

playback/src/audio_backend/pipewire.rs

+    producer: Option<RingProducer>,
+    // Flag to signal thread to stop
+    quit_flag: Arc<AtomicBool>,
+    initialized: bool,


This could be removed in favor of checking _main_loop_handle.is_some().

Add pipewire audio backend

c8c794e

Copilot AI review requested due to automatic review settings October 4, 2025 20:18

Copilot AI reviewed Oct 4, 2025

View reviewed changes

roderickvd requested changes Oct 6, 2025

View reviewed changes

Guillaume Giraudon added 2 commits October 7, 2025 19:39

Fix dependencies

e7c63f9

Fix syntax and formatting to pass fmt check

7ff2a57

Replace bytefeeding with ringbuffer to feed channels

3bad516

ggiraudon requested a review from roderickvd October 7, 2025 21:51

roderickvd requested changes Nov 7, 2025

View reviewed changes

	S24_3 => spa::param::audio::AudioFormat::S24_32LE,
	S24_3 => spa::param::audio::AudioFormat::S24_3LE,

Conversation

ggiraudon commented Oct 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

roderickvd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggiraudon commented Oct 7, 2025

Uh oh!

roderickvd commented Oct 7, 2025

Uh oh!

roderickvd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants