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

@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
@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.)

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
Copy link
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
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
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
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.

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.

None yet

5 participants