PA_VOLUME_MAX and PA_VOLUME_UI_MAX incorrect

[matthijskooijman]

The pulseaudio value is:

#define PA_VOLUME_MAX ((pa_volume_t) UINT32_MAX/2)

But pulsectl has:

https://github.com/mk-fg/python-pulse-control/blob/9fab19ef34ae9f419feb8cb6f6da59eff2c5a6f1/pulsectl/_pulsectl.py#L48

Which misses parenthesis, I think it should be:

PA_VOLUME_MAX = (2**32-1) // 2

(Note that pulseaudio had a different value before 1.0, but that does not seem relevant anymore: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/179b291b18b9a7366955948ce0ddfb2e65a6b66e)

For PA_VOLUME_UI_MAX, pulseaudio has:

#define PA_VOLUME_UI_MAX   (pa_sw_volume_from_dB(+11.0))

pulsectl has a hardcoded value, that seems to have been calculated by a python lambda version of pa_sw_volume_from_dB:

https://github.com/mk-fg/python-pulse-control/blob/9fab19ef34ae9f419feb8cb6f6da59eff2c5a6f1/pulsectl/_pulsectl.py#L51-L53

However, that lambda is wrong, the cbrt function used by pulse is the cube root, not the cube. So the correct version is:

>>> pa_sw_volume_from_dB = lambda db:  min(PA_VOLUME_MAX, int(round(((10.0 ** (db / 20.0)) ** (1/3)) * PA_VOLUME_NORM)))
>>> pa_sw_volume_from_dB(+11.0)
99957

Which matches the pulse version:

matthijs@grubby:~$ cat foo.c
#include <pulse/volume.h>
#include <stdio.h>

int main() {
        printf("%u\n", PA_VOLUME_UI_MAX);
}
matthijs@grubby:~$ gcc foo.c -lpulse && ./a.out 
99957

I wonder, though: Why not just provide a binding for pa_sw_volume_from_dB and calculate the value using that (this is how I originally ended up here, I needed pa_sw_volume_from_dB and while writing my own version, I looked at PA_VOLUME_MAX and noticed it was wrong).

[mk-fg]

Thanks!

Did fix these in the code atm.

Not really sure why I didn't wrap pa_sw_volume_from_dB to begin with, probably just that it'd be inconvenient code-structure-wise to define it in the middle of _pulsectl.py file and having to move that constant to the end of it, either apart or along from/with the rest of them. And given that presumably formula there isn't going to change often (or more likely ever), there doesn't seem to be a good reason not to pre-calculate that constant, potential bugs notwithstanding. Latter indeed should be much easier to make when re-implementing things instead of just wrapping them, but oh well, hopefully should be both fixed now. Thanks again for looking into this and pointing them out.

There's a whole bunch of various volume-value-conversion functions in volume.c, which can potentially be wrapped, and probably exposed on Volume objects somehow. This module kinda hides raw pa volume ints though, so they'd probably have to be operating on floats in the public API to avoid making them a trap, so not sure how useful that might be to your use-case, if you're looking at and using such raw values already.

[matthijskooijman]

Cool, thanks. I think this issue can be closed, then.

There's a whole bunch of various volume-value-conversion functions in volume.c, which can potentially be wrapped, and probably exposed on Volume objects somehow. This module kinda hides raw pa volume ints though, so they'd probably have to be operating on floats in the public API to avoid making them a trap, so not sure how useful that might be to your use-case, if you're looking at and using such raw values already.

Right, good point about raw vs floats. I'm currently using pulsectl to list sinks and get properties for them, for actually setting volumes I'm using vlc instead. But since vlc just passes volumes directly to pulse, I needed to calculate db to pulse scale :-) As for my particular usecase, raw numbers (0-PA_VOLUME_NORM) or floats (0-1) does not matter much, I need to scale to 0-100 for VLC anyway :-p