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

[Tanu Kaskinen]

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