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

[David Henningsson]

On 2014-08-29 12:32, Tanu Kaskinen wrote:
> On Fri, 2014-08-22 at 16:43 +0200, David Henningsson wrote:
>>
>> 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;
>> };
>
> Now that the channel map is not any more in pa_bvolume, the struct is
> missing the channel count information. This makes it impossible to
> validate the struct just by looking at its contents - the channel count
> needs to come from somewhere else when validating the struct.To avoid
> depending on external information, I propose adding a channels field to
> pa_bvolume. I think it's also fine to move the channel map back to
> pa_bvolume. What's your opinion?

I'm not sure what type of validating that is required here, but it seems 
to me like anything that needs to validate using channel count also 
needs to check has_volume, which is not in the struct.

That can be solved by skipping has_volume, and letting has_volume be 
represented as "channel_map.count > 0", but then you might want to 
validate that the volume is within what n_steps and steps dictate, and 
so on.

>> 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;
>> };
>
> How strongly do you feel about having the pa_bvolume_caps struct? I
> don't see much value in having that as a separate struct (instead, I'd
> just put the pa_bvolume_caps fields directly to pa_control_volume_data).

It's mostly for my easier thinking, but I'm imagining that some 
functions could take a pa_bvolume to manipulate with a const 
pa_bvolume_caps pointer to be able to manipulate it correctly.

>> 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...
>
> Having thought about this a bit, I think I prefer the simpler version,
> even though it has a couple of downsides (need to always query the
> volume first, and if two applications set different fields at the same
> time, only the last one will have effect). We can add the other one
> later, if necessary.

Ok. And by "simpler version" I assume you mean:

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


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