Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pipewire: Add new output plugin. Closes: #1048 #104

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

radioactiveman
Copy link
Member

This is my attempt to port the PipeWire output plugin from Qmmp to Audacious.
Ilya Kotov, author of the related Qmmp code which is under GPL, allowed me to use the BSD license for our code base.

I'm using the plugin successfully for some time now, but further tests are of course appreciated. :)
Could you please support with the remaining things listed below? I don't know how to do them.

  • Implement period_wait().
  • Use the ring buffer from libaudcore instead of the raw char array.
  • A few variables are still hard-coded, this should be cleaned-up.

Other questions:

  • I assume the channel mappings in set_channel_map() can be reduced, since Audacious only supports up to 10 channels. Is this correct?
  • Is it necessary to include all format mappings in to_pipewire_format()? The PulseAudio plugin for example only maps a few formats in to_pulse_format(). Why is that?
  • Is the priority ok? Should it be lower than the PulseAudio plugin for now?

@mcarans
Copy link

mcarans commented Aug 1, 2022

Would be great to see this merged.

@PedroHLC
Copy link

PedroHLC commented Aug 5, 2022

Demo

LGTM, or better, sounding good to me (tested on NixOS).

@jlindgren90
Copy link
Member

The biggest issue I see here is period_wait() isn't implemented. Calling pw_thread_loop_timed_wait() from write_audio() will potentially block the UI thread and make Audacious as a whole less responsive. Until we find a way to solve that, I would think it would be better to continue using the PulseAudio output plugin, as I believe that should be usable still with PipeWire?

@jlindgren90
Copy link
Member

@radioactiveman It may enough to just move the pw_thread_loop_timed_wait() bit from write_audio() to period_wait(). Would you be able to make/test that change? (I am not using PipeWire on any of my own systems currently.)

src/pipewire/pipewire.cc Outdated Show resolved Hide resolved
src/pipewire/pipewire.cc Show resolved Hide resolved
src/pipewire/pipewire.cc Outdated Show resolved Hide resolved
@radioactiveman
Copy link
Member Author

Thanks for the review @jlindgren90 and thanks @PedroHLC for testing. 👍
I will try to address the remarks in the next days when I have some time.

Based on the PipeWire Output Plugin for Qmmp,
written by Ilya Kotov.
- Implement period_wait()
- Don't hardcode buffer size
- Check return values more carefully
- Prevent memory leaks in error branches
- Simplify code
@radioactiveman
Copy link
Member Author

radioactiveman commented Aug 23, 2022

@radioactiveman It may enough to just move the pw_thread_loop_timed_wait() bit from write_audio() to period_wait(). Would you be able to make/test that change?

Yes, this works. 👍 And I have noticed that the volume slider is in fact more responsive now, especially with the GTK interface.

@radioactiveman
Copy link
Member Author

radioactiveman commented Aug 23, 2022

@jlindgren90: I have rebased this to master and addressed your review comments. CI is unfortunately not possible since the PipeWire package is too old on Ubuntu 20.04. Edit: Updated CI to use 22.04.

Could you please answer my two questions in the review? Thanks and looking forward to the merge. :)

src/pipewire/pipewire.cc Show resolved Hide resolved
src/pipewire/pipewire.cc Show resolved Hide resolved
@jlindgren90
Copy link
Member

@radioactiveman Looks good to me. Thanks for all your work on this!

@jlindgren90 jlindgren90 merged commit 5a4a578 into audacious-media-player:master Aug 26, 2022
@birdie-github
Copy link

PipeWire uses/offers the exact same API as PulseAudio, in other words PA plugin is 100% compatible and sufficient for PipeWire.

There's no need to have two plugins. You may rename the old one from "PulseAudio plugin" to "PulseAudio/PipeWire plugin" and that's it.

@kaniini
Copy link
Member

kaniini commented Aug 26, 2022

One plugin uses libpulse, and the other uses libpipewire directly. There's no need to support either sound system, but we do anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants