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
