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.
And then we could rename pa_control_volume_data to pa_bvolume because
pa_bvolume_init_* is shorter than pa_control_volume_data_init_*.
>
> 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. */
Good question. I think we should have a void* pointer for the best
compatibility with as many compilers as possible.
> 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.
Ok.
> 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.
Good point. I suggest using pa_volume_t overall because it would be more
consistent, and avoid unnecessary FPU calculations (and int <-> float
conversions), but it's not a very strong opinion.
> 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.
This is bikeshedding, I guess. It just felt more natural to tell what
something currently is, than what it can be converted to. But maybe
linear was not a good name either.
> 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.
Ok.
> "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.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
