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

[Tanu Kaskinen]

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.

-- 
Tanu