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

[Laurențiu Nicola]

Hello,

If I'm allowed to give my two cents here, I would prefer Tanu's design.
I find it much more natural (and useful) to reason about a general
volume and balance controls than about per-channel volumes. Of course,
an application can easily convert between the two representations.

As for the has_mute flag, I suppose mute can easily be emulated by the
server, so it seems like an unnecessary concern for the applications.

Laurentiu

On Tue, Jul 29, 2014, at 21:46, Tanu Kaskinen wrote:
> On Tue, 2014-07-29 at 14:01 +0200, David Henningsson wrote:
> > (Sorry for the late answer, forgot about it during the vacation)
> 
> No problem. Next time you have vacation I'll know to remind you about
> ongoing stuff right after you get back :)
> 
> > On 2014-07-04 14:06, Tanu Kaskinen wrote:
> > > On Tue, 2014-07-01 at 13:16 +0200, David Henningsson wrote:
> > >> It's more common than not to have volume bundled with a mute: almost all
> > >> sinks, sources, and streams have that. Therefore, it feels logically
> > >> more correct to keep them together, rather having every GUI having to
> > >> link them together themselves.
> > >>
> > >> That said, I do acknowledge that there are valid use cases, especially
> > >> for mutes without volumes. But it the answer to that really to separate
> > >> *all* mutes from *all* volumes?
> > >
> > > My proposal is that yes, that is the answer. I see the point of
> > > optimizing for the common case, though, so you could give a more
> > > concrete proposal that covers also cases where there is only one of
> > > volume and mute. Just adding a mute field to pa_bvolume doesn't work,
> > > because it doesn't cover the only-one-or-the-other cases.
> > 
> > Ok, here is what I think it could look like (I don't remember if you had 
> > another names for cvolume and master_volume):
> > 
> > struct pa_bvolume {
> >      pa_cvolume cvolume;
> >      double master_volume;
> >      int mute; /* 0 means unmuted, all other values muted */
> >      int has_volume;
> >      int has_mute; /* Should we skip this one? */
> 
> I'd say yes, we should skip it. We can add it later if necessary. Not
> having it makes things simpler for applications. The downside is that if
> we add it later, applications need to be updated, i.e. it's unnecessary
> churn for applications. It seems possible that we will never need to
> have the has_mute field, however.
> 
> >      int volume_is_linear; /* better name for convertible_to_dB */
> > 
> >      /* Potentially add more advanced capabilities stuff here, like 
> > n_volume_steps, or even a value for every step. */
> > }
> > 
> > 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;
> };
> 
> typedef enum {
>     PA_CONTROL_TYPE_UNKNOWN,
>     PA_CONTROL_TYPE_VOLUME,
> } pa_control_type_t;
> 
> struct pa_control_info {
>     uint32_t index;
>     const char *name;
>     const char *description;
>     pa_proplist *proplist;
> 
>     pa_control_type_t type;
> 
>     /* XXX: Are anonymous unions ok in the public api? They were
>      * standardized in C11, GCC has supported them longer. We could
>      * also use a void pointer if an anonymous union is not ok. */
>     union {
>         pa_control_volume_data *volume_data;
>         /* Other data pointers can be added later when new control types
>          * are introduced. */
>     };
> };
> 
> Some comments about the differences to your suggestion:
> 
> I don't use pa_cvolume in pa_bvolume. I don't see the benefit of using
> pa_cvolume. pa_cvolume has the channels field, which is redundant
> because the channel count is also available in the channel map, and
> otherwise pa_cvolume is just an array of volume values. If the idea is
> that the pa_cvolume field could be used for simple conversion of
> pa_bvolume into pa_cvolume, that's not possible, because in such
> conversion the overall volume needs to be taken into account.
> 
> I use pa_volume_t for the overall volume in pa_bvolume and double for
> the balance values, and in your suggestions they are the other way
> around. I don't know which way is better, or should we perhaps use
> double or pa_volume_t for both the overall volume and the balance
> values.
> 
> I still used name "convertible_to_dB". I think "volume_is_linear" is not
> an improvement (even "decibel_volume" would be better). I don't think
> it's very clear what "linear" means. Note that there's function
> pa_sw_volume_to_linear(), and "linear" in that context means a
> completely different thing. Also, think why the application writer needs
> to care about this flag in the first place: the only reason why the flag
> may be interesting, is that the application writer wants to convert the
> volume to decibels, and the flag tells whether that is possible.
> 
> I used name "volume" for the overall volume instead of "master_volume",
> because some people (at least Jaska ;) seem to have a strong association
> from term "master volume" to a device volume. "overall_volume" doesn't
> sound very good to me either. I don't feel very strongly about this,
> though. Having "volume" and "balance" as the two components of
> pa_bvolume sounds good to me otherwise, but when the pa_bvolume struct
> itself is stored in a variable named "volume", things get a bit awkward,
> since referring to the overall volume of a volume control becomes
> control->volume.volume.
> 
> -- 
> Tanu
> 
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss