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_pipewire: fix ao-volume #9830

Merged
merged 1 commit into from
Feb 11, 2022
Merged

ao_pipewire: fix ao-volume #9830

merged 1 commit into from
Feb 11, 2022

Conversation

pkunk
Copy link
Contributor

@pkunk pkunk commented Feb 4, 2022

Store volume only in mpv format, convert it from spa a soon as possible.
Pass volumes to pw_stream_set_control as array.

@pkunk
Copy link
Contributor Author

pkunk commented Feb 4, 2022

@t-8ch can you take a look please.

@t-8ch
Copy link
Contributor

t-8ch commented Feb 5, 2022

@pkunk What issue does it fix? That should be part of the commit message.

I assume that the call to pw_stream_set_control() is wrong. It should supply an array of floats.
Do we need the other changes?
The argument list should be terminated with a 0, can you add this too?
The same problem also exists in the logic for muting, can you fix that, too?

audio/out/ao_pipewire.c Outdated Show resolved Hide resolved
@pkunk
Copy link
Contributor Author

pkunk commented Feb 5, 2022

Updated the commit message.
Added terminating 0s.
Don't think muting is affected, we pass only one value here for entire stream. Also it works fine for me.
I'm not aware about any other required changes. Maybe 5.1 support will require some.

@t-8ch
Copy link
Contributor

t-8ch commented Feb 5, 2022

Thanks!

I'm curious about the incorrect scaling. Because this worked for me (and presumably the other testers).
Can you provide steps to reproduce this?
Can you also test if only fixing the calls to pw_stream_set_control() and not shuffling the scaling around works for you?

Muting will also be affected for some users. The trailing 0 is needed for all calls to pw_stream_set_control(). If it's missed, the behavior is system-dependent it seems however.

@pkunk
Copy link
Contributor Author

pkunk commented Feb 5, 2022

Just to be 100% sure. Did you tested ao-volume?
I have non-standard input.conf:

9 add ao-volume -4
/ add ao-volume -4
0 add ao-volume 4
* add ao-volume 4
m cycle ao-mute

Ah yes, I have added 0 to mute too.

@t-8ch
Copy link
Contributor

t-8ch commented Feb 5, 2022

Yes.

0 add ao-volume 2
9 add ao-volume -2
m cycle ao-mute
p cycle-values ao-volume 25 50 75 100

I can also see the volume in pavucontrol matching exactly the values in mpv.

@pkunk
Copy link
Contributor Author

pkunk commented Feb 5, 2022

Yes, just tested, you right. Seems fix for pw_stream_set_control() is enough.
I'll remove my changes for scaling. I personally still prefer them, but they are not related to the problem.

On unrelated note, just tested 5.1 and ao-volume basically nonfunctional here, just resets everything to 0.

Pass channel volumes to `pw_stream_set_control` as array.
This is correct calling conventions and prevents
right channel muting every time ao-volume property is changed.

Terminate `pw_stream_set_control` calls with 0.
@t-8ch
Copy link
Contributor

t-8ch commented Feb 5, 2022 via email

@t-8ch
Copy link
Contributor

t-8ch commented Feb 6, 2022

This is now ready to merge from my PoV.

@etircopyh
Copy link
Contributor

LGTM.

@philipl philipl merged commit bc9805c into mpv-player:master Feb 11, 2022
@philipl
Copy link
Member

philipl commented Feb 11, 2022

Thanks!

@etircopyh
Copy link
Contributor

It seems to break if media file is using more than 2 channels. It can be worked around by passing --audio-channels=stereo.

@etircopyh
Copy link
Contributor

cc @pkunk @t-8ch

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

Successfully merging this pull request may close these issues.

None yet

4 participants