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

[David Henningsson]

On 2014-08-04 11:49, Tanu Kaskinen wrote:
> On Fri, 2014-08-01 at 12:30 +0200, David Henningsson wrote:
>>
>> 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.
>
> I don't think that can work. Consider this function, for example:
>
> pa_context_set_control_volume(pa_context *c, uint32_t control_idx,
>                                pa_bvolume *bvolume, int set_volume,
>                                int set_balance, pa_success_cb_t cb,
>                                void *userdata);
>
> The bvolume parameter is supposed to be allocated from stack, so the
> application does something like this:
>
> pa_bvolume v;
> pa_bvolume_init_mono(&v, 0.3 * PA_VOLUME_NORM);
> pa_context_set_control_volume(context, idx, &v, true, false, NULL, NULL);
>
> pa_control_volume_data is supposed to be extensible. Now if pa_bvolume
> is actually the same thing as pa_control_volume_data, then when
> pa_bvolume is extended, the pa_bvolume_init_mono() and
> pa_context_set_control_volume() calls will probably break if the client
> was built against an older libpulse version, because the new libpulse
> assumes that the pa_bvolume size is bigger than what was actually
> allocated in this example.

To sum up today's discussions on IRC, I'm ending up with a separation 
between the actual data and the capabilities describing that data:

struct pa_bvolume {
     pa_volume_t volume;
     double balance[PA_CHANNELS_MAX];
     int mute;
};

struct pa_bvolume_caps {
     int has_mute;
     int has_volume;

     /* The following are only valid if has_volume = true */
     pa_channel_map channel_map;
     pa_volume_t *steps;
     unsigned n_steps;
     int convertible_to_dB; /* I still don't like this name, but... */
};

struct pa_control_volume_data {
     pa_bvolume vol;
     pa_bvolume_caps caps;
};

We discussed pa_context_set_control_volume(), which in my mind will look 
either like:

pa_context_set_control_volume(/* context and control parameters */, 
pa_bvolume *vol);

or possibly:

pa_context_set_control_volume(/* context and control parameters */, 
pa_bvolume *vol, bool set_volume, pa_channel_map set_balance, bool 
set_mute);

Where the channel map parameter is used to specify what channels in the 
balance array you actually want to set. Or perhaps we could use a bitmask...

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