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

ao/pulse: set similar properties as the pipewire backend #13724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pobrn
Copy link
Contributor

@pobrn pobrn commented Mar 18, 2024

Not setting media.role especially negatively affects wireplumber's restore-stream feature as it will save different stream settings for ao=pulse and ao=pipewire, so they will be restored differently.

For example:

  1. mpv --ao=pipewire ...
  2. mute the stream in pavucontrol
  3. mpv --ao=pulse ...
  4. note that the stream is not muted

To alleviate that issue, set media.role the same way it is set in the pipewire audio backend. (As well as some others.)

See https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/3848

@pobrn pobrn force-pushed the similar_props_pw_pa branch 2 times, most recently from 7867d44 to edc7b47 Compare March 18, 2024 23:12
Copy link

github-actions bot commented Mar 19, 2024

Download the artifacts for this pull request:

Windows
macOS

@sfan5 sfan5 added the ao:pulse label Mar 19, 2024
audio/out/ao_pulse.c Outdated Show resolved Hide resolved
@Traneptora
Copy link
Member

In that case this LGTM.

Copy link
Member

@jeeb jeeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that the functions with regards to proplist are apparently available from 0.9.11, so our requirement of 'libpulse', version: '>= 1.0' does not require bumping.

Thus otherwise LGTM.

audio/out/ao_pulse.c Outdated Show resolved Hide resolved
audio/out/ao_pulse.c Outdated Show resolved Hide resolved
@pobrn pobrn force-pushed the similar_props_pw_pa branch from edc7b47 to db80bdf Compare March 19, 2024 17:58
@jeeb
Copy link
Member

jeeb commented Mar 19, 2024

Looked at the logic and going to test a more centralized way of cleaning up the proplist. If it looks good, I'll note it here and otherwise pull the commit in with possibly a small change (keeping pa_threaded_mainloop_unlock as the last thing in the success case - technically it wouldn't matter since the context has its own copy, but just to keep the general idea of unlocking being last.)

@pobrn pobrn force-pushed the similar_props_pw_pa branch from db80bdf to 20942a4 Compare March 19, 2024 18:41
@pobrn
Copy link
Contributor Author

pobrn commented Mar 19, 2024

I am not sure if this is what you have in mind, but I simplified the cleanup of props.

@jeeb
Copy link
Member

jeeb commented Mar 19, 2024

Will finish groceries and test :) .

@jeeb
Copy link
Member

jeeb commented Mar 19, 2024

Apparently mpv used to set the video role and hit an issue, #1173 (fixed by b7325b2 ) . Will have to figure out if this is still valid.

@pobrn
Copy link
Contributor Author

pobrn commented Mar 19, 2024

Ahh right, that is unfortunate... as far as I can see module-role-cork is still likely in the default configuration on most distributions.

@jeeb
Copy link
Member

jeeb commented Mar 19, 2024

I think the initial mute/lowered volume was not the problem, just that it would still go that way in case of things switching...

more specifically, I think haasn's comment @ #1173 (comment) goes through it. But I'm still trying to 100% grasp it :) .

@pobrn
Copy link
Contributor Author

pobrn commented Mar 19, 2024

I think the initial mute/lowered volume was not the problem, just that it would still go that way in case of things switching...

I am afraid that is still happening. I just tested and I saw the following:

  1. the stream is muted if another playback stream appears;
  2. the stream is also muted when mpv finishes playing and switches to the next track (interestingly that does not happen if I use arrow buttons on the UI to switch tracks)

@sfan5
Copy link
Member

sfan5 commented Apr 5, 2024

So, is this good to go or are there still issues with this change?

@pobrn
Copy link
Contributor Author

pobrn commented Apr 5, 2024

The only slightly problematic part is

pa_proplist_sets(props, PA_PROP_MEDIA_ROLE, ao->init_flags & AO_INIT_MEDIA_ROLE_MUSIC ?  "music" : "video");

in the diff. Unfortunately that is what would "fix" the issue reported on the pipewire bug tracker. It would essentially "reopen" #1173, although one can easily fix that by not loading module-role-cork or excluding video from roles that module-role-cork corks.

@kasper93
Copy link
Contributor

kasper93 commented May 6, 2024

Do we maybe want to merge this without PA_PROP_MEDIA_ROLE, if there are problems? Just app names.

@pobrn
Copy link
Contributor Author

pobrn commented Aug 5, 2024

Do we maybe want to merge this without PA_PROP_MEDIA_ROLE, if there are problems? Just app names.

That does not address the reason I propose this change, which is the inconsistent stream restore behaviour depending on which audio backend mpv uses.


As mentioned in the referenced issue, VLC also sets media.role; and this behaviour can easily be avoided by unloding module-role-{cork,duck} (and possibly module-augment-properties) in ~/.config/pulse/default.pa.


I think it would be nice to keep the property list as consistent as possible between the two backends, but in the end it is a small thing. So how can we move forward from here?

Not setting `media.role` especially negatively affects wireplumber's
restore-stream feature as it will save different stream settings for
ao=pulse and ao=pipewire, so they will be restored differently.

For example:

 1. mpv --ao=pipewire ...
 2. mute the stream in pavucontrol
 3. mpv --ao=pulse ...
 4. note that the stream is not muted

To alleviate that issue, set `media.role` the same way it is set
in the pipewire audio backend. (As well as some others.)

See https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/3848
@pobrn pobrn force-pushed the similar_props_pw_pa branch from 20942a4 to 3828b2f Compare January 5, 2025 18:17
@pobrn
Copy link
Contributor Author

pobrn commented Jan 5, 2025

Any chance we can move forward somehow?

@llyyr
Copy link
Contributor

llyyr commented Jan 5, 2025

Maybe only set the media role if we're running under pipewire emulated pulseaudio rather than pulseaudio itself.

@pobrn
Copy link
Contributor Author

pobrn commented Jan 5, 2025

Maybe only set the media role if we're running under pipewire emulated pulseaudio rather than pulseaudio itself.

I am really not a fan of doing that.

@llyyr
Copy link
Contributor

llyyr commented Jan 5, 2025

Then this can't move forward until #13724 (comment) / #1173 is resolved on pulseaudio side. Open a bug report there perhaps.

Just use the pipewire AO if you're on a system with pipewire-pulse anyway.

one can easily fix that by not loading module-role-cork or excluding video from roles that module-role-cork corks.

Not a fan of this unless you can convince pulseaudio to make that the default configuration.

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

Successfully merging this pull request may close these issues.

6 participants