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
