-
Notifications
You must be signed in to change notification settings - Fork 3k
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: reactivate cubic volume control #15835
base: master
Are you sure you want to change the base?
Conversation
You're aware that you can force-push PR branches to change the contents without opening a new one, right? |
Yeah, but I didn't want to have to edit the PR message, including title, which stayed the same after my amendments. Sorry, if this messed things up or something. I don't yet have muscle memory for this kind of stuff. Wasn't even expecting anybody to notice |
Changing the commit message is a normal course of action if requested to make this change, for instance. The PR title can also be modified if it accidentally included incorrect info. There's no need to edit prior posts though. Next time, just fix the title and force-push an updated commit.
Opening and closing PRs shows up as messages at the developers IRC channel, and as emails at the developers inboxes. |
Will do.
Thanks for explaining, I do hope this didn't cause too much trouble for you. Sorry! |
No worries. Sh*t happens. |
Download the artifacts for this pull request: |
of all the 12+ backends, only pulseaudio uses cubic volumes. Why do do you think PipeWire should use cubic volumes as well and not the other backends? If anything, the pulseaudio backend should use linear volumes like everything else. |
No, this is not at all what I want. I just want to be able to adjust volume inside mpv on a more reasonable scale, because as it is now, there is very little difference over long swaths above 20. And below 20 it falls off too quickly. And that is exactly why HiFi amps, and anything audio really, uses a logarithmic scale, as it best matches human loudness perception. What this does, is simply translating mpv's internal representation to the linear scale of PipeWire. So mpv submits the proper value on the linear scale to pipewire, but internally handles it on a cubic one. TBH, I am not too happy with cubic volume control either, see my RFC draft PR, if it is visible to you, please. But for the time being, a cubic scale seems to be the agreed upon best next thing, in lieu of a logarithmic scale, which would be the correct for all intents an purpose audio. Now, I don't want anything changed in PipeWire. It is actually a good thing that it uses the linear scale. Also, please see your own comments regarding a related topic on the PW tracker, which gave me the idea, basically. Further down in that thread, the cubic scale is mentioned as well. And in that context you seem to be fine with PipeWire's volume value being represented on whatever scale the client wants. Please, don't take this as a dig at you. As the commit message says, I simply believe that you misunderstood what the intent of this code is and what its implications are. mpv does communicate linear values to and from pulsewire. |
I understand that linear volumes don't make for a great UI but would the translation from ui to volume not better be done in the higher layers instead of the backends? As it is now, all backends have crappy volume scales except pulse and pipewire because they got special cased? |
I am not the right person to answer that. But that is actually what I am eyeing with said RFC, hence the () around ao_pipewire. Edit: this should be considered a stop gap measure. |
@wtay or would it be possible for PipeWire to expose an interface that accepts/returns decibel values. I'm thinking something like I am asking because I believe that would be the best place to implement. Otherwise every PW client would need to implement it themselves. And I have yet to find one that actually tries. P.S.: Please also do not take any of this as a dig at Lennart. He is a great guy and a prolific dev. Just to make extra sure that this does not go down the wrong throat, given all that awful abuse he has to endure. I feel for him. You may tell him that, if you happen to be in contact. |
I don't think we've established the terminology enough to state what is correct and what isn't when using these terms.
I don't think the invariant is "transform the ao-volume value the same way in all AOs". To establish what is the invariant, let's start with the docs:
Here's how I interpret it: ao-volume controls a volume. If the audio backend supports it (libpulse, libpipewire, wasapi, whatever), then this can be either the global backend volume, or an application-specific volume within this backend. It is a user-facing value which represents a physical volume knob, like the volume knob in a hifi system or a TV. Accordingly, the mpv value is 0-100, which should map 1:1 to user-facing volume slider at the backend's mixer with reasonable behavior which the user expects. This means two things:
Can we agree on the above? If not, please state what would you like to agree on instead. If yes, then the problem becomes very simple, at least as far as verifying that it behaves correctly:
So, if we can also agree on this, and such reasonably-behaving backend mixer slider does exist, then the only problem to solve is to find the formula which the AO code in mpv should use - which can differ between AOs, depending on each specific backend. Assuming the documentation of the backend is correct and sufficient, it should be pretty easy. There's no need for opinions or RFCs. We only need to read the docs, write the formula, and then confirm that it moves the backend's mixer slider as expected. If the backend does not have a reasonably-behaving user-facing mixer volume slider, then the problem can become a bit harder. But the goal should still be the same: that the ao-volume value represents a resonably-behaving physical volume slider, and when moving this slider in constant speed (i.e. if it takes X seconds from 0 to 50, then it's also X seconds from 50 to 100), it results in reasonably evenly perceived progression of the loudness. So, do we have such reasonably-behaving user-facing volume slider which controls the pipewire volume, regardless of mpv? If yes, does the formula in this PR make ao-volume behave as specified above? does the alternative formula in #15836 make it behave as specified above? If one of those formulas adheres to the specifications above, then there's nothing else to discuss, and we just take the new formula, and that's it. If none of them behave like specified above, then find the right formula which matches the spec. If we don't have this reasonably-behaving user-facing volume slider which controls pipewire, then it makes it harder, but it should still be possible to find a good formula afer reading the PW docs. Right? |
This is already true for all PA/PW. The only reason the user's slider doesn't "match" for Pipewire specifically is because they're checking the the volume with frontends like wpctl/pavucontrol which only show the volume on cubic scale. This is also not possible to do for all AOs, the user can for example configure pipewire-alsa to be linear or cubic and mpv can't know this. Moreover, the property documents itself perfectly:
No where does it promise that the volume units will remain the same across all backends. And I don't think it's a worthwhile thing to do, given mpv has its own internal volume system that the users should be using instead. |
I don't care about the technical terms at this stage. I care that there exists a user-facing backend volume slider which has a reasonable behavior as far as general user-facing volume knobs go. I.e. that it's percieved with reasonably linear progression of the loudness when moving/rotating this knob in a constant speed. If more than one front end user-facing volume sliders exists, then we first need to identify which one of those has this reasonable user-facing behavior, and this is what ao-volume should map to.
That's a fair point. I presume each backend still has some default or main behavior, and if yes, then this is probably also what ao-volume should do with this backend by default. Additionally, if indeed it's impossible or very hard or too hard for mpv to tell which mapping is used later down the chain, then it might also make sense to add a user-configurable mapping, at which case we'd need to think what kind of form it should be (and I'm not getting into the specifics currently, but it should be doable, and probably useful in such cases).
Sure. Which is exactly why I specified it in terms of reasonably-behaving user-facing slider with imaginary evenly-spaced 0-100 markers, and not in terms of any specific units or transformation formulas. ao-volume should map 1:1 to a reasonably-behaving backend mixer volume slider. It's really simple. How close we can get there or even if it's all possible to do automatically in mpv is a different question. But the goal should be stupid simple to verify whether or not an AO code in mpv got it right in any specific use case. |
@llyyr, using mpv's internal software mixer leaves an awful lot to be desired, once one has been spoiled by the niceties of audio servers like PA and PW, as I am as are many others who actually use Do you, @llyyr, even use ao-volume with wasapi? Is that even the right one? Because I seem to remember that there was another one for interaction with Windows' audio server, and wasapi being for different stuff and exclusive access to the backend devices. I might be wrong though, haven't been near a Windows machine in a long time. |
People, let's try to stay on topic, which is the pipewire AO formula. It's already hard enough even before discussing unrelated things like preserving the mpv volume between sessions, so unless it is specifically related to the pw formula, please drop other subjects for now. Thanks. |
It literally is doing that. Use tools like For example when I do
I have no idea why you guys insist we try to map mpv volume to frontends that do magic unit conversion instead of Pipewire's internal truth. |
What you're asking for is to bring back The |
@avih as I have said in IRC, the ao-volume "slider", when using ao=pipewire, does not feel right, quite the opposite: it's the worst kind of wrong. See above: hardly any difference above 20, and below way too sensitive. And that's the problem with adjusting volume on a linear scale. It is simply not done and renders ao-volume useless for this AO. Just to establish some terminology percecptual linearity is achieved only on a logarithmic scale which turns multiplication into addition and power calculation (x to the power of e) into multiplication. Thus one can use linear math on that scale: add/subtract increments and get the linear feel, or multiply dB value by 2 and actually hear double loudness. Since it is not so easy to map that scale to a range of 0-100, simply using cubic values is kind of the next best thing. But it is just less wrong. So yes, PulseAudio is kind of "broken by design" for accepting linear scale values and internally taking them to the power of 3. But it also is not, since that is documented behaviour and should be accounted for on the API-consumer end. And I also get why @wtay refuses to fall into the same trap as PA did with this approach. You set linear volume, you get linear volume. I can only see two ways to get proper, or at least less bad, feel of ao-volume with PW. Either mpv does the cubic or, even better, logarthmic transformation. Or PW provides different API knobs, i.e. the ability to set sink volume in dB, and do its magic. I'd prefer the latter, TBH. But if that ever happens, I don't know. I'd like @wtay to weigh in on this, because I think as an audio server which provides volume "knobs", PW should provide them in user-friendly format: dB, at least as an option. |
@llyyr Why does mpv even have to do internal volume filtering if I set ao-volume? Does it, really? And no, I do not want softvol back, because that would also not work with saving volume state inside PW. |
Is it also perceived with linear loudness progression when this value progresses linearly? If yes, then great, we found this slider, and all we need is to agree on this. I didn't say we need to change anything. I only described the spec I think the AO should adhere to. If it already does, it's the best. But it it's not percieved with linearly progressive loudness (this user-facing volume knob with imaginary markers), then we didn't find it. I don't know how it's perceived. You tell me.
I'm not persisting on anything. I'm saying what is the best as far as the user is concerned, and that is that ao-volume value behaves like a reasonably-behaving user facing volume slider or knob, and then I specified how exactly, in terms of user-facing sliders. The user shouldn't care which formula the mpv AO uses, or which units the backend uses, or which transformations need to happen. The user only cares that ao-volume behaves like a good user-facing volume knob. Whether or not mpv can adhere to this lofty goal with pipewire is exactly the topic of this PR. |
Nope, since it is literally linear, no transformation. Which is the reason for this PR. |
I don't see the point, linear volume is a good low-level representation of the volume. You can trivially convert dB or cubic or logarithmic to and from it. |
PW is pedantically correct in using linear values as it says on the box. And that is a good thing in my book. Because then one can do transformations before calling the API and be done with it, knowing there is no second guessing on PW's part, as opposed to PA's behaviour, which, while well intentioned, complicates things. Now, I do not remember why exactly PA does what it does, but I think it had to do with the broken state of affairs with clients expecting something like virtually all mixer frontend suggest: a scale from 0-100 percent. So instead of fixing every single client, they chose to just "do the less bad thing" internally. |
But I think it should not be expected from every programmer that uses the interface. I believe many don't even know the ins and outs of audio and why we use logarithmic scales, and how those work. That should be abstracted away by a backend such as yours I think. Do the math in one place, PW. |
@wtay, I think this discussion and @llyyr's and @avih's replies suggest, that it is better to have such a thing in your API. BTW, I've grepped for math.h in the ao directory. There are 8 AOs that include it! Why? What kind of complicated math is there to do on that level? |
Is that your final, definite, forever set in stone answer on this? If so, I can live with that, at least I can refer to this, should the need ever arise. I may not like it, but I will accept it and not pester you any more. |
100% yes |
Because there's no maintenance burden to having a property that is just a raw setter for a value. But this whole ordeal has convinced me otherwise, so at this point I'm +1 to deprecating ao-volume and removing it.
Patches welcome, but they must do this for all AOs. |
There is also no maintenance burden with doing the correct transformation. It's a one time thing.
-1 My remarks were rhetorical.
Good, I should be able to this on anything Linux, i.e. ALSA. But with ALSA I am having a really hard time seeing the point of |
@llyyr, since you don't like maintenance burden, don't you see the irony? This PR exists because of your stance on how one should interpret And I distinctly remember someone in IRC saying that passing raw values to the API "is just lazy". I agree, wholeheartedly. Maybe you missed it, or weren't present. |
Here I am, I find it simpler to use a single global volume. |
Since this seems to widen the scope of this PR, I just want to establish, which AO's should have their WASAPI for windows, if I am not mistaken, is the equivalent of PW/PA. Anything else that supports this kind of audio server functionality? P.S.: When I said "widen the scope", I actually meant to say, that this should be done in another PR. (Edit: added ao_sndio) |
But really, do you have to adjust it from inside mpv? That's what those fancy volume buttons on most any modern keyboard are for. Also, mpv's own software volume implementation seems a better choice. |
I don't have to though showing the osd-bar for ao-volume is nice. |
Speaking of irony, you could also ask yourself this. |
FYI OP's request is easily fulfilled by just using something like https://github.com/d87/mpv-persist-properties and just using mpv's We're deep in some hellfire of X-Y-Z-α-β Problem:
For whatever reason, there is ideological opposition to using a Lua script for the purpose of making mpv remember volume from a past session and they insist that it must be handled by mpv setting the volume on the audio server mixer. |
2c26174
to
cf8fed7
Compare
Commit c7b17be by @wtay disabled cubic volume control. This reintroduces it. There must have been a misunderstanding. *Because* pipewire uses linear scale volume and we *want* cubic volume control this translates between the two. Otherwise we would just adjust volume on a linear scale which is the worst way to do it, because loudness is measured on a logarithmic scale. In lieu of that a cubic curve is a good enough approximation until such time as actual logarithmic volume control becomes a possibility/reality. This makes volume control in ao_pipewire the same as in ao_pulse. This author does know that @wtay is the author of PipeWire but believes that he misunderstood the intent in a drive-by included in a collection of fixes. Fixes: mpv-player#11065 Probably fixes: mpv-player#12223
cf8fed7
to
e8f0fe0
Compare
Forgot to push my changes. Now this is done by a macro which was borrowed and adapted from ao_pulse.c And the very fact that said macro exists in ao_pulse.c is a strong counterpoint to @llyyr's stance. That should not exist either, then. |
The very existence of said hack is a case in point for my argument, really. Just look at the lengths one has to go to for things that one can get for free with modern audio servers. |
That's the point of such audio server functionality. It should not be done inside the app, which also translates to less maintenance burden, BTW. What are your ideological reasons for being against using 21st century audio interfaces? |
One more feature of PA/PW, that cannot be emulated by mpv's software volume processing, is "flat volumes". Any software audio processing incurs a bit of quality loss, which is why PA's author came up with flat volume: the loudest client is the reference, and PA/PW set the closest dB value on the backend devices (depending on its granularity) and only add/subtract the difference to get the exact requested value. This also maximizes SNR (signal to noise ratio) because having the master at 0dB (100%) and attenuating client volumes by, say -30dB, means that SNR just went down 30dB. Not so with flat volumes. I know this has been a contentious subject in the past but only because people misunderstood or keep misunderstanding it. P.S.: And those are my very well informed reasons for avoiding software volume processing wherever I can. |
@llyyr from your statements I infer that you are using mpv's internal software volume processing then. So why do you even care? You won't notice any of this. And there will be no maintenance burden. You are blowing this triviality way out of proportion. |
For me the problem with -volume is that it does not do what I as a normal user wants. As can seen above there are many different ways people use audio. So it may be that you should look at a broader scope the just pipewire |
This is already the case for --volume. --volume is cubic.
the ao is irrelevant for --volume.
This used to be the case for mpv too until 995c47d
You want --volume to be linear? |
Now why did nobody tell me this in IRC? And why is it a problem to bring ao_pulse in line with that?
Perceptually linear, is what the author most certainly meant. And this is as close as one can get to that in lieu of a logarithmic scale. |
Agreed, I hinted at that above already. This is just a stop gap to make ao_pipewire's But I also think that the fact that you maintain your own patched mpv, points to some shortcoming which I am on about. If you want to create an issue, please do, and let's discuss there. I am not yet quite sure what that issue report should be. |
Flat volumes, however, caused problems. Once apps (firefox) figured out that they could use a start volume, they just set it to 100% and things would almost literally blow up. There were other problems like syncing the hard and software volumes when clients appeared and inaccurate decibel levels from the hardware. Flat volumes were disabled in fedora in 2016 and finally disabled in pulseaudio by default since 2019. PipeWire no longer supports flat volumes. I think the most common use case these days is a global volume (acting as a limit) per hardware device (using a combination of soft and hardware volumes) and a per stream (software) volume. Both those volumes are remembered per device and per stream (or group of streams, insofar the stream can be identified correctly).
|
@llyyr, why am I reminded of the "good ol'days" when the principal author of MPlayer refused to even explore multi-threading. Think back at what I said in IRC. |
Oh, must have missed that. Thanks for explaining. Disregard my comment on that then.
But that is, kind of, what I outlined, right? The better "flat volumes" so to speak. |
The important part of the device volume is that it should not be changed by applications ever (or at least not by untrusted apps). |
I'm telling you that mpv used to have what you're asking for, but it was too hard to get right so it was removed. If you think you can do better than wm4 then please show it with patches. This PR does not do what is asked. If you want to make
mpv shouldn't accept any stop-gap solutions that actively contradict the documentation.
When you told us "That might just come back to haunt you"? I await your mpv fork. |
Then why are you opposed to having the backend (PW) handle that?
I don't and I don't want to. Are you actually reading my comments? I want this feature which comes for free with ao_{pipewire,pulse}. No code necessary in mpv. For all I care, mpv's internal volume processing is "legacy" and should be deprecated.
It does, you just fail to, or don't want to, understand the question.
I never objected to that, but one thing at a time. In fact, that is very much what I want: a consistent UX, no matter the AO, no matter the
Nothing, except that its "quirks" need to be accounted for, just as those of PA so clearly are; that macro again. And I put those "" because that is not really a quirk. PW is a low-level API. Using such requires higher levels, such as mpv, to put in some extra effort. And this is soooooo trivial.
And I did ask where I can put that macro so as to be reused, didn't I? |
Fixing PW doesn't require fixing all other AOs. If we can fix (and test and confirm) more AOs, then great, if not, fixing only PW for now is better than nothing. |
Sorry I missed this. So keep the getter, lose the setter? Would that be OK? But I think, that should really be a separate issue, because it is out of scope for this PR, but very much on my radar. |
I mean the bar is shown when changing ao-volume. But it's not a big deal if it's easier to remove it. |
Commit c7b17be by @wtay disabled cubic volume control. This reintroduces it. There must have been a misunderstanding. Because pipewire uses linear scale volume and we want cubic volume control this translates between the two.
Otherwise we would just adjust volume on a linear scale which is the worst way to do it, because loudness is measured on a logarithmic scale. In lieu of that a cubic curve is a good enough approximation until such time as actual logarithmic volume control becomes a possibility/reality. This makes volume control in ao_pipewire the same as in ao_pulse.
This author does know that @wtay is the author of PipeWire but believes that he misunderstood the intent in a drive-by included in a collection of fixes.
Fixes: #11065
Probably fixes: #12223
I have read this:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md