'Twas brillig, and Tanu Kaskinen at 13/07/11 16:26 did gyre and gimble:
> On Thu, 2011-07-07 at 10:55 +0100, Colin Guthrie wrote:
>> In order to try and avoid 'spamming' the user with port choices,
>> attempt to detect and remove any pointless paths in a path set. That is
>> any paths which are subsets of other paths.
>>
>> This should solve a problem case with some USB Headsets which result in
>> two paths both involving the 'Speaker' element. When no 'Master' element
>> exists (which is quite common on head/handsets), then the first path
>> (analog-output) will contain the 'Speaker' in a way that completely fits
>> with in the use of the 'Speaker' element in the other path
>> (analog-output-speaker).
>> ---
>> src/modules/alsa/alsa-mixer.c | 167 ++++++++++++++++++++++++++++++++++++++++
>> src/modules/alsa/alsa-sink.c | 3 -
>> src/modules/alsa/alsa-source.c | 3 -
>> 3 files changed, 167 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
>> index abd3bf2..842fba2 100644
>> --- a/src/modules/alsa/alsa-mixer.c
>> +++ b/src/modules/alsa/alsa-mixer.c
>> @@ -2873,6 +2873,166 @@ void pa_alsa_path_set_dump(pa_alsa_path_set *ps) {
>> pa_alsa_path_dump(p);
>> }
>>
>> +/**
>> + * Compares two elements to see if a is a subset of b
>> + */
>> +static pa_bool_t element_is_subset(pa_alsa_element *a, pa_alsa_element *b) {
>> + pa_assert(a);
>> + pa_assert(b);
>> +
>> + /* General rules:
>> + * Every state is a subset of itself (with caveats for volume_limits and options)
>> + * IGNORE is a subset of every other state */
>> +
>> + /* Check the volume_use */
>> + if (a->volume_use != PA_ALSA_VOLUME_IGNORE) {
>> +
>> + /* "Constant" is subset of "Constant" only when their constant values are equal */
>> + if (a->volume_use == PA_ALSA_VOLUME_CONSTANT && b->volume_use == PA_ALSA_VOLUME_CONSTANT && a->constant_volume != b->constant_volume)
>> + return FALSE;
>
> I think "constant", "zero" and "off" should somehow be merged in this
> check. Currently the code considers for example "constant" with value 4
> to be a subset of "zero", because all the combinations of "constant",
> "zero" and "off" are not checked.
ACK.
I've added this check after the above one:
/* Different volume uses when b is not "Merge" means we are
definitely not a subset */
if (a->volume_use != b->volume_use && b->volume_use !=
PA_ALSA_VOLUME_MERGE)
return FALSE;
which I think covers this case (if my morning brain is working... which
is not guaranteed)
> IIRC David suggested removing _ZERO
> and _OFF altogether from the enumeration and using _CONSTANT instead -
> that would solve also this problem.
Yeah, I think this is valid but I don't want to do this just now... too
much churn for an add-on feature late in the cycle. I'd say it's a
candidate for cleanup once the dust has settled.
>> + /* "Constant" is a subset of "Merge", if there is not a "volume-limit" in "Merge" below the actual constant.
>> + * "Zero" and "Off" are just special cases of "Constant" when comparing to "Merge"
>> + * "Merge" with a "volume-limit" is a subset of "Merge" without a "volume-limit" or with a higher "volume-limit".
>> + * "Zero" is a subset of "Off" */
>
> As discussed in IRC, "zero" shouldn't be a subset of "off".
ACK.
>> + if (a->volume_use != b->volume_use || b->volume_use == PA_ALSA_VOLUME_MERGE) {
>> + if (a->volume_use == PA_ALSA_VOLUME_OFF && b->volume_use == PA_ALSA_VOLUME_ZERO)
>> + return FALSE;
>> +
>> + if (b->volume_use == PA_ALSA_VOLUME_MERGE && b->volume_limit >= 0) {
>
> This assumes that volume_limit can't be negative. As it has turned out,
> it's acceptable for alsa to use negative volumes. (It may actually very
> well be the case that volume_limit can't be negative, though, because
> some other piece of code assumes so - I had that assumption when I
> implemented the volume limit feature.)
Yeah I've seen a few places where "if (volume_limit >= 0)" is used, so I
don't think this is a practical problem right now. As the volume limit
case is a little special, I'd say I'm happy to live with this limitation
for the time being, even if the underlying alsa volume *could* be
negative. As Takashi expressed a desire to fix this at the alsa level, I
don't want to spend hours getting myself tied in knots over it. WDYT?
>> + long a_limit = -1;
>> + if (a->volume_use == PA_ALSA_VOLUME_CONSTANT)
>> + a_limit = a->constant_volume;
>> + else if (a->volume_use == PA_ALSA_VOLUME_ZERO || a->volume_use == PA_ALSA_VOLUME_OFF)
>> + a_limit = 0;
>
> This is wrong. If a->volume_use is "zero", then a_limit should be what
> snd_mixer_selem_ask_playback_dB_vol() returns, or if a->db_fix is set,
> then what decibel_fix_get_step() returns. If a->volume_use is "off",
> then a_limit should be a->min_volume.
ACK.
However, I don't want to be calling those functions here.... is
a->min_volume not set regardless? Can I not just always use this as
opposed to 0?
>> + else if (a->volume_use == PA_ALSA_VOLUME_MERGE)
>> + a_limit = a->volume_limit;
>
> Since the volume limit can be negative, the if condition should check if
> a->volume_limit_is_set is TRUE. (No, that variable doesn't exist
> currently.)
Again, I think I'll live with this for the time being if you are happy
to accept that there are other problems with volume limit being
negative... this wont break things *more*, even if I concede that this
should be fixed at some point...
>
>> +
>> + if (a_limit < 0 || a_limit > b->volume_limit)
>> + return FALSE;
>> + }
>> + }
>> + }
>> +
>> + if (a->switch_use != PA_ALSA_SWITCH_IGNORE) {
>> + /* "On" is a subset of "Mute".
>> + * "Off" is a subset of "Mute".
>> + * "On" is a subset of "Select", if there is an "Option:On" in B.
>> + * "Off" is a subset of "Select", if there is an "Option:Off" in B.
>> + * "Select" is a subset of "Select", if they have the same options (is this always true?). */
>> +
>> + if (a->switch_use != b->switch_use) {
>> +
>> + if (a->switch_use == PA_ALSA_SWITCH_SELECT || a->switch_use == PA_ALSA_SWITCH_MUTE
>> + || b->switch_use == PA_ALSA_SWITCH_OFF || b->switch_use == PA_ALSA_SWITCH_ON)
>> + return FALSE;
>> +
>> + if (b->switch_use == PA_ALSA_SWITCH_SELECT) {
>> + if (a->switch_use == PA_ALSA_SWITCH_ON) {
>> + pa_alsa_option *o;
>> + pa_bool_t found = FALSE;
>> + PA_LLIST_FOREACH(o, b->options) {
>> + if (pa_streq(o->alsa_name, "on"))
>> + found = TRUE;
>> + }
>> + if (!found)
>> + return FALSE;
>> + } else if (a->switch_use == PA_ALSA_SWITCH_OFF) {
>> + pa_alsa_option *o;
>> + pa_bool_t found = FALSE;
>> + PA_LLIST_FOREACH(o, b->options) {
>> + if (pa_streq(o->alsa_name, "off"))
>> + found = TRUE;
>> + }
>> + if (!found)
>> + return FALSE;
>> + }
>> + }
>> + } else if (a->switch_use == PA_ALSA_SWITCH_SELECT) {
>> + /* Treat this like an enumeration */
>> + /* If there is an option A offers that B does not, then A is not a subset of B. */
>> + pa_alsa_option *oa, *ob;
>> + PA_LLIST_FOREACH(oa, a->options) {
>> + pa_bool_t found = FALSE;
>> + PA_LLIST_FOREACH(ob, b->options) {
>> + if (pa_streq(oa->alsa_name, ob->alsa_name)) {
>> + found = TRUE;
>> + break;
>> + }
>> + }
>> + if (!found)
>> + return FALSE;
>> + }
>> + }
>> + }
>> +
>> + if (a->enumeration_use != PA_ALSA_ENUMERATION_IGNORE) {
>> + /* If there is an option A offers that B does not, then A is not a subset of B. */
>> + pa_alsa_option *oa, *ob;
>> + PA_LLIST_FOREACH(oa, a->options) {
>> + pa_bool_t found = FALSE;
>> + PA_LLIST_FOREACH(ob, b->options) {
>> + if (pa_streq(oa->alsa_name, ob->alsa_name)) {
>> + found = TRUE;
>> + break;
>> + }
>> + }
>> + if (!found)
>> + return FALSE;
>> + }
ACK. Two helpers now made.
Col
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/
Day Job:
Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
Mageia Contributor [http://www.mageia.org/]
PulseAudio Hacker [http://www.pulseaudio.org/]
Trac Hacker [http://trac.edgewall.org/]
