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

Tanu Kaskinen tanuk at iki.fi
Wed Jul 13 08:26:44 PDT 2011


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. IIRC David suggested removing _ZERO
and _OFF altogether from the enumeration and using _CONSTANT instead -
that would solve also this problem.

> +        /* "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".

> +        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.)

> +                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.

> +                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.)

> +
> +                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;
> +        }

I think this is enough copy-pasted code to warrant a helper function.

-- 
Tanu



More information about the pulseaudio-discuss mailing list