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

[Tanu Kaskinen]

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:
> >>
> >> 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.

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);

> >> 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.

Ok, I'll use pa_bvolume_caps at least for now.

> >> 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);

Yes.

-- 
Tanu