On Fri, 2014-02-14 at 11:55 +0100, David Henningsson wrote: > On 02/14/2014 10:49 AM, Tanu Kaskinen wrote: > > Does the grouping have any benefits? If pa_volume_control_info has a > > field with this pa_cvolume2 type, then it could be argued that for > > symmetry there should also be function > > pa_context_set_volume_control_volume() that takes a pa_cvolume2 struct > > as a parameter. However, when an application sets the attributes of a > > volume control object, usually it will only modify the overall volume or > > the balance, not both, and the channel map is immutable, so passing a > > pa_cvolume2 struct to the setter function is inconvenient. > > A volume control UI with presets would typically set both. So would any > volume control UI do that does not look exactly the way you anticipate. > > E g, pavucontrol has two sliders, one left and one right, and gnome > volume control has main volume + balance. I thought about pavucontrol, but not thoroughly enough. I thought that if you change only one channel, only balance needs to be changed, but of course that only works if the channel in question isn't the highest volume channel. > The grouping would also enable potential helper functions such as those > we have for pa_cvolume today. Good point. I think grouping the overall volume, the balance and the channel map together makes sense. About the struct naming: do you know what "c" in "cvolume" refers to? I would guess that it means "channel". pa_cvolume very concretely has per-channel volume (a pa_volume_t value for each channel). It could be argued that pa_cvolume2 is more balance-oriented than channel volume oriented, so here's an idea for a name without numbers: pa_bvolume. Thoughts? > > I'd like to keep mute controls completely separate from volume. They are > > not inherently linked to each other (even though they very often appear > > together). > > I don't understand this argument. A mute control *is* a volume control > (with two distinct values). That doesn't change the fact that the volume and mute controls should be separate: I don't want to bundle two volume controls together either. However, I'd be ok with adding a possibly-NULL reference from a volume control object to a mute control object and vice versa. There are other ways to correlate a volume control with a mute control, but direct reference would definitely make it easier. > In this case, it's a way to avoid volume > information lost on muted audio. > Well, and that everything said so far in this thread would apply equally > well to mute controls, so I guess you will come back in a few months and > want to add another core object for mute controls if we don't get it in > now ;-) Indeed, I'd like to have mute control objects too. That's not so high priority for me, however, so I didn't include it in my initial proposal. It can be added cleanly later. > > What would you think about putting the icon name as a separate field to > > the pa_volume_control_info struct? I'm not against adding a proplist for > > undocumented metadata, but let's not abuse the proplist for things that > > are part of the documented stable API (any more than what we already > > do). > > IIRC, icon names are in proplists everywhere else, so having them in a > proplist here as well would avoid inconsistency. True. While I'd prefer a separate field, this is a very small issue that can be fixed at any point in the future if we want, so let's agree to put the icon name to a proplist for now. > >> The volume classes however seem more like routing/policy implementation > >> internals. They could be kept inside the routing/policy module, exported > >> through property lists, or something like that. Either that, or I didn't > >> really understand the concept. > > > > Applications should have a unified way to query what volume classes > > exist and what volume control they are associated with. An example of a > > volume class is the event volume class. If applications want to access > > that, they currently need to use a module-stream-restore specific API, > > even though the concept of an event volume class is not specific to > > module-stream-restore. > > The concept "volume class" does not exist in module-stream-restore, at > least not with that name. > > Could you give a clear definition of what "volume classes" are? If it's > just a way of grouping volume control objects, then volume control > objects could just have some property of what class they belong to. An attempt at a definition: a volume class is a named set of rules for grouping volumes. "A way of grouping volume control objects" is not an entirely accurate description. You probably were thinking of a setup where each stream has a volume control object, and those are somehow grouped by a volume class. In my proposal streams whose volume is governed by the volume class do not have their own volume control objects. Instead, the streams reference the volume control object of the volume class. This is not a very important distinction, however, because your suggestion of using a property works either way. You could set a "volume_class = event" property on a volume control to tell clients that this volume control object controls the volume of event sounds. Apart from my usual complaint about "abusing" proplists, there are some other issues too with the property-on-a-volume-control approach: 1) You can't get a list of all volume classes in the system. Ok, you can, by checking the "volume_class" property on every volume control object. Not pretty, but you can do it. 2) After getting the list of all volume classes, you can't show a human-readable label for them in a user interface, because all you have is the non-translatable volume class identifier. The volume control description can probably be usually used as a replacement, however, so this is probably not a big issue in practice. (An example where reusing the volume control label for volume classes might be problematic: on the Nokia N9 the volume classes point to different volume controls depending on the current routing.) 3) The human-readable label is one example of a volume class attribute. There could be others in the future, but those can't be implemented if there's no object to attach the attributes to. In my opinion volume classes deserve to be treated as full-blown objects, but if I'm the only one thinking that, I won't insist on doing it that way. The property-on-a-volume-control approach works too. -- Tanu
