On 2014-09-11 21:56, Tanu Kaskinen wrote:
> On Fri, 2014-08-29 at 15:28 +0200, David Henningsson wrote:
>>
>> On 2014-08-29 15:08, Tanu Kaskinen wrote:
>>> On Fri, 2014-08-29 at 12:57 +0200, David Henningsson wrote:
>>>>
>>>> On 2014-08-29 12:32, Tanu Kaskinen wrote:
>>>>> On Fri, 2014-08-22 at 16:43 +0200, David Henningsson wrote:
>>>>>> 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.
>>>
>>> The place where I need validation right now is in the tagstruct
>>> deserialization for get_control_info reply. I was implementing
>>> pa_tagstruct_get_bvolume(), and I realized that I didn't have knowledge
>>> of how many balance values to expect. It's possible to avoid adding a
>>> channels field to pa_bvolume by adding an "expected_channels" argument
>>> to pa_tagstruct_get_bvolume(), or by sending the channel count for
>>> bvolume at protocol level and returning that value separately from
>>> pa_tagstruct_get_bvolume() like this:
>>>
>>> /* sender */
>>> pa_tagstruct put_bvolume(t, &bvolume, channels);
>>>
>>> /* receiver */
>>> pa_tagstruct_get_bvolume(t, &bvolume, &channels);
>>
>> Right, so you don't want to send a lot of unused channel information,
>> but sending the two extra "mute" and "volume" are okay even if the
>> control does not have mute or volume?
>>
>> Yeah, I guess an extra "channel" argument would do there. But sending
>> the full pa_bvolume_caps in instead would work too -
>> pa_tagstruct_get_bvolume could then fail in case the channel count did
>> not match.
>
> How do you suggest pa_context_set_control_volume() to work?
> pa_tagstruct_put_bvolume(t, &bvolume, channels) doesn't work as well as
> I imagined it to work, because the channels parameter is unknown.
>
> My proposal is (still) to add a channels field to pa_bvolume, but there
> are other ways too. The application could pass the number of channels
> explicitly to pa_context_set_control_volume(), or the application could
> pass the whole pa_control_volume_data object. libpulse could also
> somehow cache the channel count when the application queries the volume
> control info.
Hmm. I'm leaning towards either of the two later ones. Probably the most
flexible one would be to pass the whole pa_control_volume_data object.
Then the user would essentially do:
pa_context_get_control_volume(/*...*/, &control_volume_data);
pa_move_balance_to_the_left(&control_volume_data.volume,
&control_volume_data.caps);
pa_context_put_control_volume(/*...*/, &control_volume_data);
Now that leaves the question of pa_move_balance_to_the_left (the name is
just an example) should take a pa_control_volume_data or one bvolume and
one bvolume_caps. The reason to split them up would be that then the
bvolume_caps could be a const pointer and the bvolume be a non-const
pointer, but maybe it's overkill and pa_move_balance_to_the_left could
just use the entire pa_control_volume_data pointer instead.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
