[pulseaudio-discuss] RFC: New volume functionality for PulseAudio

[David Henningsson]

On 2014-07-29 20:46, Tanu Kaskinen wrote:
>> The idea is that pa_cvolume is the "bare minimum" struct and the
>> pa_bvolume is the "extra everything" struct. Does that make sense?
>
> I don't really see the point of bundling all that stuff in pa_bvolume.
> IIRC, in my original proposal there was no pa_bvolume, but you suggested
> putting the overall volume, balance and channel map together in a
> struct, and that was a good idea, because those bits are needed together
> by several functions, so having a struct reduces the need to have many
> function parameters. The rest of the stuff that you now suggested to add
> to pa_bvolume doesn't have such benefit. The fields are irrelevant for
> functions that need to take a pa_bvolume as a parameter, so it's better
> to store those things in pa_control_info.
>
> I think the important thing here, though, is that you think that adding
> has_volume (and maybe has_mute) fields is ok. I don't think it makes
> sense for me to continue pushing for separate volume and mute controls
> (I'm not sure any more I'd even prefer separate controls), so it's
> starting to be clear what kind of design I should implement. The
> variable naming and whether to store things in pa_bvolume or
> pa_control_info are details that don't necessarily need to be decided
> before I can start preparing some patches.
>
> Here's my current plan how the structs in the client API should look
> like:
>
> struct pa_bvolume {
>      pa_volume_t volume;
>      double balance[PA_CHANNELS_MAX];
>      pa_channel_map channel_map;
> };
>
> struct pa_control_volume_data {
>      pa_bvolume volume;
>      pa_volume_t *steps;
>      unsigned n_steps;
>      int convertible_to_dB;
>
>      int mute;
>
>      int has_volume;
>      int has_mute;
> };

Hmm, as it looks now, I don't think the pa_bvolume encapsulation has any 
benefit. Any initialization functions (or similar) should just take the 
full pa_control_volume_data struct instead.
And then we could rename pa_control_volume_data to pa_bvolume because 
pa_bvolume_init_* is shorter than pa_control_volume_data_init_*.

>
> typedef enum {
>      PA_CONTROL_TYPE_UNKNOWN,
>      PA_CONTROL_TYPE_VOLUME,
> } pa_control_type_t;
>
> struct pa_control_info {
>      uint32_t index;
>      const char *name;
>      const char *description;
>      pa_proplist *proplist;
>
>      pa_control_type_t type;
>
>      /* XXX: Are anonymous unions ok in the public api? They were
>       * standardized in C11, GCC has supported them longer. We could
>       * also use a void pointer if an anonymous union is not ok. */

Good question. I think we should have a void* pointer for the best 
compatibility with as many compilers as possible.

>      union {
>          pa_control_volume_data *volume_data;
>          /* Other data pointers can be added later when new control types
>           * are introduced. */
>      };
> };
>
> Some comments about the differences to your suggestion:
>
> I don't use pa_cvolume in pa_bvolume. I don't see the benefit of using
> pa_cvolume. pa_cvolume has the channels field, which is redundant
> because the channel count is also available in the channel map, and
> otherwise pa_cvolume is just an array of volume values. If the idea is
> that the pa_cvolume field could be used for simple conversion of
> pa_bvolume into pa_cvolume, that's not possible, because in such
> conversion the overall volume needs to be taken into account.

Ok.

> I use pa_volume_t for the overall volume in pa_bvolume and double for
> the balance values, and in your suggestions they are the other way
> around. I don't know which way is better, or should we perhaps use
> double or pa_volume_t for both the overall volume and the balance
> values.

Good point. I suggest using pa_volume_t overall because it would be more 
consistent, and avoid unnecessary FPU calculations (and int <-> float 
conversions), but it's not a very strong opinion.

> I still used name "convertible_to_dB". I think "volume_is_linear" is not
> an improvement (even "decibel_volume" would be better). I don't think
> it's very clear what "linear" means. Note that there's function
> pa_sw_volume_to_linear(), and "linear" in that context means a
> completely different thing. Also, think why the application writer needs
> to care about this flag in the first place: the only reason why the flag
> may be interesting, is that the application writer wants to convert the
> volume to decibels, and the flag tells whether that is possible.

This is bikeshedding, I guess. It just felt more natural to tell what 
something currently is, than what it can be converted to. But maybe 
linear was not a good name either.

> I used name "volume" for the overall volume instead of "master_volume",
> because some people (at least Jaska ;) seem to have a strong association
> from term "master volume" to a device volume.

Ok.

> "overall_volume" doesn't
> sound very good to me either. I don't feel very strongly about this,
> though. Having "volume" and "balance" as the two components of
> pa_bvolume sounds good to me otherwise, but when the pa_bvolume struct
> itself is stored in a variable named "volume", things get a bit awkward,
> since referring to the overall volume of a volume control becomes
> control->volume.volume.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic