[pulseaudio-discuss] [PATCH 4/8] alsa-mixer: Detect and then drop pointless paths in the path set.

Colin Guthrie gmane at colin.guthr.ie
Mon Jul 18 02:33:05 PDT 2011


'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/]



More information about the pulseaudio-discuss mailing list