On Fri, 2014-09-12 at 08:35 +0200, David Henningsson wrote:
>
> On 2014-09-11 21:56, Tanu Kaskinen wrote:
> > 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);
What does this function do? There's no precedent for this kind of thing
in the introspection API. I'd expect the application to get the volume
data from a control info callback.
> 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.
You missed the part where you free the volume data. You can't allocate
the volume data from the stack, unless you also want to add this macro
and the pa_control_volume_data_size() function:
#define pa_control_volume_data_alloca() ((pa_control_volume_data *) alloca(pa_control_volume_data_size()))
By the way, alloca() is not in the POSIX standard. I'm not sure if
that's a problem regarding using it in the public API.
Here are two functions that do the same thing. The first version is what
I think you meant in your proposal, and the second is my proposal. Your
proposal has the disadvantage that the volume data has to be freed after
use, but my proposal has the (unexpected) disadvantage that it needs one
more variable to avoid excessive casting from the void data pointer to
pa_control_volume_data. I'll implement the first version, unless you
change your mind.
/*** first version, pa_bvolume without channels field ***/
static void get_control_info_cb(pa_context *context,
const pa_control_info *info,
int is_last, void *userdata) {
App *app = userdata;
pa_control_volume_data *data;
if (is_last < 0)
abort();
if (is_last > 0)
return;
if (info->type != PA_CONTROL_TYPE_VOLUME)
abort();
data = pa_control_volume_data_copy(info->data);
pa_control_volume_data_move_to_the_left(data);
o = pa_context_set_control_volume_by_index(context, info->index, data,
NULL, NULL);
if (!o)
abort();
pa_operation_unref(o);
pa_control_volume_data_free(data);
}
/*** second version, pa_bvolume with channels field ***/
static void get_control_info_cb(pa_context *context,
const pa_control_info *info,
int is_last, void *userdata) {
App *app = userdata;
pa_control_volume_data *data;
pa_bvolume bvolume;
if (is_last < 0)
abort();
if (is_last > 0)
return;
if (info->type != PA_CONTROL_TYPE_VOLUME)
abort();
data = info->data;
bvolume = data->volume;
pa_bvolume_move_to_the_left(&bvolume, &data->caps.channel_map);
o = pa_context_set_control_volume_by_index(context, info->index, &bvolume,
NULL, NULL);
if (!o)
abort();
pa_operation_unref(o);
}
--
Tanu
