Fix assertion crashes when volume > 100

[yunake]

With PulseAudio, it is perfectly possible for the volume to go above 100%. In fact, even the GUI tools usually support up to 130, but one can go up to MAXINT with pacmd --allow-boost. When volume meter is being displayed on the tray icon, going above ~110 crashes pnmixer (actual number depends on the icon size of course). Full native support of PA is a lot of work, but like you say in the readme, it works rather well through the alsa backend anyway. This crash is the only problem I've seen in the last couple of years and I think this is a reasonable and simple fix.

[hasufell]

Wouldn't the proper fix involve consistently allowing >100 volume and reporting that in the tray icon/inotify messages etc?

This will just fix a crash, but isn't a consistent fix, is it? Also, how do you increase volume over 100% with just alsa? I cannot even test this properly, since I don't use any pulseaudio.

[yunake]

Wouldn't the proper fix involve consistently allowing >100 volume and reporting that in the tray icon/inotify messages etc?

It really depends on how you define "proper fix". IMHO, a "proper" fix in this case may be implementing full pulseaudio support. I am not interested in investing my time into this, i simply want to eliminate the crash in a meaningful way; i didn't simply remove the assertion, which would've worked too. From my perspective, this is a clear improvement, does not introduce regressions, and does not put further burden on maintainers by introducing large and complex new code. It is a nice alternative to a full-blown PA support and fixes a real-world issue. Displaying proper notifications etc would all be nice and all but it can easily be treated as an independent, orthogonal piece of work, don't you agree?

This will just fix a crash, but isn't a consistent fix, is it?

I have no idea what you mean by a "consistent fix". This MR is focused on fixing a very specific bug in a meaningful way.

Also, how do you increase volume over 100% with just alsa?

I don't think you can. I certainly don't know of a way to do that.

I cannot even test this properly, since I don't use any pulseaudio.

What do you suggest? It's a simple and straight-forward change, easy to audit. I've been using it for a couple of days, if it doesn't cause issues for pure-Alsa users, why not merge it?

[yunake]

you know, frankly, i think this is not only 'good enough' for real-world, this is actually the correct way to handle displaying >100 volume in the overlay of the tray icon. how else would you handle it? remember, the volume can grow into hundreds of thousands, so not even logarithmic scales would suffice; also it would be really confusing for 99.9% of the users that are unaware of this corner-case. there's simply no way to display these volume values nicely with a bar.

[hasufell]

To me, this is still a corner case and next time I look at the code, I won't really know why someone put fminf there, so I'd say an inline comment would be good.

[yunake]

Fair enough, let me update this.

[elboulangero]

Hey @yunake, thanks for your patch.

It's true that we don't expect the volume to go above 100, and hence the code just assumes that it never happens.

I think a better fix is to clip the volume directly in the function that gets the volume. Doing this, we ensure that there's no value above 100 anywhere else.

That's what I did in the branch clip-volume. Can you check it out and tell me if it fixes the problem ? And hopefully doesn't create any problem either ?

Cheers

[yunake]

i have done some extensive testing with this branch and it seems to be working just fine. thanks you @elboulangero! this is definitely a better fix than mine.

[elboulangero]

Great, thanks a bunch for spending time on that !