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
