Native pipewire AO #9587

t-8ch · 2021-12-10T19:38:48Z

This PR adds a native AO for pipewire and closes #8569.

The original code has been written by @Oschowa which was extended by me. (See the different commits)
@Oschowa did not propose the PR themselves as they were experiencing vsync jitter with this AO.
For other users however this AO works as good as or better than the existing ones, and they have been using it for some time with custom builds.

When running on a native pipewire system this AO saves intermediate steps through libpulse and pipewires pulseaudio server.

set_pause
uau wonders if there is a problem in accurately reporting the remaining buffered audio.

wscript_build.py

etircopyh · 2021-12-11T18:17:00Z

You might also want to make docs changes.

t-8ch · 2021-12-11T20:22:48Z

You might also want to make docs changes.

@etircopyh I added a mention to the manpage. It's really tiny, but there is not much to document.

audio/out/ao_pipewire.c

etircopyh · 2021-12-12T23:43:38Z

audio/out/ao.c

@@ -87,6 +88,9 @@ static const struct ao_driver * const audio_out_drivers[] = {
 #endif
 #if HAVE_SDL2_AUDIO
    &audio_out_sdl,
+#endif
+#if HAVE_PIPEWIRE
+    &audio_out_pipewire,


Just a nitpick. Why is pipewire placed under wrappers?

The goal was to make it explicitly opt-in, to give it some time to stabilize

Ah, I see. Makes sense, especially with this issue not being resolved.

etircopyh · 2021-12-13T17:46:27Z

Trying to figure out why I'm having much lower volume with ao_pipewire. The funny thing it only occurs on my KDE setup, the Sway one doesn't have the problem. And I don't know what's the culprit. I have two separate installations of Arch on my laptop. KDE has the volume issue and Sway doesn't. I even tried to run mpv with --no-config, no luck.

UPD: The cause seems to be channelVolumes. No matter what volume I set in mpv I get "channelVolumes": [ 0.068210, 0.068210 ] from pw-dump.

t-8ch · 2021-12-14T09:49:41Z

@etircopyh Please make sure you are modifying ao-volume on both machines (look in input.conf).

etircopyh · 2021-12-16T16:38:36Z

https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/1915#note_1193527

t-8ch · 2021-12-16T22:21:15Z

@etircopyh Please test with the latest commit on this PR.

etircopyh · 2021-12-17T15:47:43Z

No, it doesn't work with this property and it won't. PW_KEY_NODE_RATE is used to suggest a sample rate.

Here's some changes: ao_pipewire-mpv.patch.txt

t-8ch · 2021-12-17T16:10:42Z

@etircopyh Is this patch supposed to fix the issue or just do some cleanups?
If it is supposed to fix it, could you point out the specific part doing that.
If not, could you provide a way to reproduce the issue?

etircopyh · 2021-12-17T16:23:25Z

@etircopyh Is this patch supposed to fix the issue or just do some cleanups? If it is supposed to fix it, could you point out the specific part doing that. If not, could you provide a way to reproduce the issue?

It does fix the issue and doing a bit of cleanup along the way. It does not interfere with anything. You can reproduce the issue with current state of PR with playing some 44.1 kHz file and running pw-top where you'll see something like this:

Device doesn't follow file's sample rate.

With fix you'll have this:

And yeah, you need to have 44100 in your allowed-rates in your pipewire.conf.

etircopyh · 2021-12-17T16:30:13Z

If it is supposed to fix it, could you point out the specific part doing that.

It's the part that sets PW_KEY_NODE_RATE as I said before.

t-8ch · 2021-12-17T16:43:43Z

Thanks for the information. I messed up PW_KEY_NODE_RATE vs PW_KEY_AUDIO_RATE...
I'll incorporate your changes.

A few more questions:

Why is "PW_KEY_NODE_ALWAYS_PROCESS" needed?
Why are some of the properties that are set to ao->client_name set in pw_properties_new() and some with pw_properties_set()?

etircopyh · 2021-12-17T17:44:41Z

Why is "PW_KEY_NODE_ALWAYS_PROCESS" needed?

As far as I understand it's supposed to make life easier for people that have no device node or are playing around with virtual sinks/sources, like connecting them together, etc. Some explanation. It seems like there's more to come on this one.

Why are some of the properties that are set to ao->client_name set in pw_properties_new() and some with pw_properties_set()?

I kind of looked it up but I think it's to make sure these properties don't collide with ones that pipewire sets. Because pw_properties_set() does set a property and overrides it.

t-8ch · 2021-12-17T18:00:03Z

Why is "PW_KEY_NODE_ALWAYS_PROCESS" needed?

As far as I understand it's supposed to make life easier for people that have no device node or are playing around with virtual sinks/sources, like connecting them together, etc. Some explanation. It seems like there's more to come on this one.

Good, thanks for the explanation!

Why are some of the properties that are set to ao->client_name set in pw_properties_new() and some with pw_properties_set()?

I kind of looked it up but I think it's to make sure these properties don't collide with ones that pipewire sets. Because pw_properties_set() does set a property and overrides it.

Yes it would replace existing values. But as we are constructing the properties from scratch there should be no conflict.
After the properties have been populated and send to Pipewire there is no difference.

LaserEyess · 2021-12-31T03:16:53Z

Just wanted to say I've been testing this patch since the latest changes, and while I can't comment on the code, I can say it works as advertised. No audio glitches as far as I can tell. Hopefully someone will have time for a more thorough review soon. Thank you for making this PR.

etircopyh · 2022-01-01T14:45:53Z

Well, as soon as the issue resolved I hope this PR will make it to upstream, more so as prioritized AO if pipewire is driving audio.

philipl · 2022-01-04T23:27:31Z

I've been using this for a couple of weeks now (after experiencing the mentioned issue about detecting if pipewire will be useful or not...) and it's been working as well as the pulse AO for me.

audio/out/ao_pipewire.c

philipl

I'm no pipewire expert, but the change looks reasonable from an mpv perspective, and it's worked fine in my local testing.

audio/out/ao_pipewire.c

philipl · 2022-01-06T19:32:20Z

Can you squash all the changes please. This should only need to be one commit.

t-8ch · 2022-01-06T20:54:57Z

@philipl Done.
I added some tags to attribute @andreaskem , @Oschowa and @etircopyh for their original development , I hope that is fine.

audio/out/ao_pipewire.c

philipl · 2022-01-11T17:55:22Z

Perhaps we are looking at similar symptoms. Maybe the issue here is that the quantum calcuation is off? @t-8ch Can you push another branch with the buffer size still being managed by size rather than time? Maybe I didn't notice the juddering before because that was testing before that change (which I admit I asked you to do :-P)

t-8ch · 2022-01-11T18:01:11Z

@etircopyh Did you test the latest state including 158d0f5 for pausing and seeking?
@philipl be678a0 should be the commit with the old buffer parameter, although it should not make much difference. Now we have the same default as pw-cat.

etircopyh · 2022-01-11T18:19:31Z

@t-8ch yes, I'm currently testing with latest PR changes.

Now we have the same default as pw-cat.

It seems to be kind of "just in case" value. Here's a comment from OpenAL code.

t-8ch · 2022-01-11T18:28:33Z

The default is 100ms.
For me it works flawlessly with buffer sizes from 2ms to 2s. Is there anything special about your setup?

etircopyh · 2022-01-11T18:31:45Z

Is there anything special about your setup?

No, I don't think so.

philipl · 2022-01-11T18:51:47Z

@t-8ch If I revert the change to express buffers in miliseconds, the juddering goes away. So it's clearly related to that. Did you set the default to keep the total buffer size the same? Or does it result in a different (larger) buffer. As I noted previously, a small buffer (I tested with 10ms) does not judder, while the default value does, and larger buffers judder more badly. I notice @etircopyh said OpenAL had the smallest default buffer size and the best performance in his tests. Maybe that's related.

So, it's not clear if the problem is with the calculation or that big buffers are bad.

t-8ch · 2022-01-11T19:02:51Z

@philipl The previous default was 2048 samples. The current default is 100ms, so probably 4800 samples.
The calculation should be fine, as the quantum is shown correctly in pw-top.
How about reducing the default to 20ms? When going through libpulse the default is 60ms.
(Please note that the OpenAL comment is for their input stream)

v-fox · 2022-01-11T19:04:10Z

The default is 100ms.
For me it works flawlessly with buffer sizes from 2ms to 2s. Is there anything special about your setup?

Isn't it considered that buffer for communicating with PW needs to be as small as possible while internal buffer may be whatever app wants ? Meaning that sane values would be somewhere around 4-24ms. OpenAL defaults to 20ms so it's on a safe side from both missing its time-frame and being too unwieldy.

philipl · 2022-01-11T19:23:53Z

Yea, 20ms is smooth for me. I remain confused that @t-8ch can't see anything wrong at higher buffer sizes but if it's more correct to have the small buffer, then let's just make 20ms the default and be happy with that.

t-8ch · 2022-01-11T20:52:39Z

@philipl Sounds good, it's commited. I'll rebase before the final merge

philipl · 2022-01-11T23:15:43Z

Thanks. Let's give it a few more days to see if we get any more feedback.

notgood · 2022-01-12T15:22:31Z

Tested this PR on a few videos while debugging pipewire issue https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/1200

No visible/audible problems so far, at least to my untrained eyes and ears.

etircopyh · 2022-01-12T19:17:30Z

https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/1931#note_1217075

haasn · 2022-01-13T06:55:10Z

Thanks. Let's give it a few more days to see if we get any more feedback.

~~My issue is still not fixed~~

Edit: nvm, I see, this is a pipewire issue, not an issue with the AO

haasn · 2022-01-13T06:56:56Z

Is there any way you could add --audio-device support to this AO? So you can choose which sink to play on. Right now it always plays on the default sink, but I want to be able to control that.

philipl · 2022-01-17T05:05:25Z

@t-8ch Is @haasn's request possible? In the mean time, I think you can squash the current set of changes.

The AO provides a way for mpv to directly submit audio to the PipeWire audio server. Doing this directly instead of going through the various compatibility layers provided by PipeWire has the following advantages: * It reduces complexity of going through the compatibility layers * It allows a richer integration between mpv and PipeWire (for example for metadata) * Some users report issues with the compatibility layers that to not occur with the native AO For now the AO is ordered after all the other relevant AOs, so it will most probably not be picked up by default. This is for the following reasons: * Currently it is not possible to detect if the PipeWire daemon that mpv connects to is actually driving the system audio. (https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/1835) * It gives the AO time to stabilize before it is used by everyone. Based-on-patch-by: Oschowa <oschowa@web.de> Based-on-patch-by: Andreas Kempf <aakempf@gmail.com> Helped-by: Ivan <etircopyhdot@gmail.com>

t-8ch · 2022-01-17T09:33:10Z

@t-8ch Is @haasn's request possible?

Sorry, I missed this comment. Yes that's possible, I have a halfway done branch for it.
Should it be part of this PR? It's getting already a bit long.
This should not be a critical as the output can always be changed afterwards.

In the mean time, I think you can squash the current set of changes.

Done

philipl · 2022-01-17T19:43:32Z

@t-8ch Thanks so much for working on this and addressing all the feedback! Please open a new PR for the audio-device change.

Sometimes the most obvious things can be missed. Reflects authorship described in the original commit. * #7902 * Oschowa@fddb143 * #9587

laichiaheng · 2022-01-18T14:04:37Z

Does it output audio to pipewire by default?

Dudemanguy · 2022-01-18T14:08:40Z

It does not. You need to specifiy --ao=pipewire. The reason is because there's currently no way to detect if pipewire is running.

t-8ch · 2022-01-20T23:06:50Z

There is now a mostly-functionial PR for the device selection at #9734

t-8ch mentioned this pull request Dec 10, 2021

ao_pipewire fixes and media control Oschowa/mpv#1

Closed

LaserEyess reviewed Dec 10, 2021

View reviewed changes

wscript_build.py Show resolved Hide resolved

etircopyh reviewed Dec 12, 2021

View reviewed changes

audio/out/ao_pipewire.c Outdated Show resolved Hide resolved

t-8ch force-pushed the ao-pipewire-2 branch from bde57ac to 6cdbf74 Compare December 12, 2021 15:04

etircopyh reviewed Dec 12, 2021

View reviewed changes

t-8ch force-pushed the ao-pipewire-2 branch from 8cabe65 to d0c08f9 Compare December 17, 2021 18:28

philipl reviewed Jan 4, 2022

View reviewed changes

audio/out/ao_pipewire.c Show resolved Hide resolved

philipl approved these changes Jan 6, 2022

View reviewed changes

audio/out/ao_pipewire.c Show resolved Hide resolved

audio/out/ao_pipewire.c Outdated Show resolved Hide resolved

t-8ch force-pushed the ao-pipewire-2 branch from 6a0a491 to 77cbd9c Compare January 6, 2022 18:09

t-8ch force-pushed the ao-pipewire-2 branch 2 times, most recently from ae7efa4 to e9a44b1 Compare January 6, 2022 20:54

philipl reviewed Jan 6, 2022

View reviewed changes

audio/out/ao_pipewire.c Outdated Show resolved Hide resolved

t-8ch force-pushed the ao-pipewire-2 branch from a1b2fa0 to ebb0739 Compare January 17, 2022 09:29

philipl merged commit 87aba14 into mpv-player:master Jan 17, 2022

philipl added a commit that referenced this pull request Jan 17, 2022

ao/pipewire: Add copyright header

22b0bac

Sometimes the most obvious things can be missed. Reflects authorship described in the original commit. * #7902 * Oschowa@fddb143 * #9587

t-8ch deleted the ao-pipewire-2 branch January 17, 2022 22:58

notgood mentioned this pull request Jan 18, 2022

Pipewire causing unstable vsync ratio and mistimed/dropped frames with video-sync=display-resample? #8839

Closed

This was referenced Jan 18, 2022

feature request: pipewire audio output driver #9462

Closed

Mistimed with pulse audio output. #9473

Closed

behunin mentioned this pull request Nov 13, 2022

PipeWire support mozilla/cubeb#705

Open

Native pipewire AO #9587

Native pipewire AO #9587

Conversation

t-8ch commented Dec 10, 2021 • edited

etircopyh commented Dec 11, 2021

t-8ch commented Dec 11, 2021

etircopyh Dec 12, 2021

Choose a reason for hiding this comment

t-8ch Dec 12, 2021

Choose a reason for hiding this comment

etircopyh Dec 12, 2021

Choose a reason for hiding this comment

etircopyh commented Dec 13, 2021 • edited

t-8ch commented Dec 14, 2021

etircopyh commented Dec 16, 2021

t-8ch commented Dec 16, 2021

etircopyh commented Dec 17, 2021

t-8ch commented Dec 17, 2021

etircopyh commented Dec 17, 2021

etircopyh commented Dec 17, 2021

t-8ch commented Dec 17, 2021

etircopyh commented Dec 17, 2021 • edited

t-8ch commented Dec 17, 2021 • edited

LaserEyess commented Dec 31, 2021

etircopyh commented Jan 1, 2022

philipl commented Jan 4, 2022

philipl left a comment

Choose a reason for hiding this comment

philipl commented Jan 6, 2022

t-8ch commented Jan 6, 2022

philipl commented Jan 11, 2022

t-8ch commented Jan 11, 2022

etircopyh commented Jan 11, 2022

t-8ch commented Jan 11, 2022

etircopyh commented Jan 11, 2022

philipl commented Jan 11, 2022

t-8ch commented Jan 11, 2022

v-fox commented Jan 11, 2022

philipl commented Jan 11, 2022

t-8ch commented Jan 11, 2022

philipl commented Jan 11, 2022

notgood commented Jan 12, 2022

etircopyh commented Jan 12, 2022

haasn commented Jan 13, 2022 • edited

haasn commented Jan 13, 2022 • edited

philipl commented Jan 17, 2022

t-8ch commented Jan 17, 2022

philipl commented Jan 17, 2022

laichiaheng commented Jan 18, 2022

Dudemanguy commented Jan 18, 2022

t-8ch commented Jan 20, 2022 • edited

t-8ch commented Dec 10, 2021 •

edited

etircopyh commented Dec 13, 2021 •

edited

etircopyh commented Dec 17, 2021 •

edited

t-8ch commented Dec 17, 2021 •

edited

haasn commented Jan 13, 2022 •

edited

haasn commented Jan 13, 2022 •

edited

t-8ch commented Jan 20, 2022 •

edited