Re: [PATCH] alsa-mixer: Fix element_is_subset().
David Henningsson <david.henningsson <at> canonical.com>
2011-08-01 12:13:13 GMT
On 2011-07-22 19:20, Tanu Kaskinen wrote:
> Untested code, don't commit willy-nilly... I'll be away for
> the weekend, and I thought I'd already send this patch for
> review. I'll test this some day next week.
I've made a quick code review (no real testing). I think most of it
looks good.
> element_change_off_and_zero_to_constant() is introduced to
> make the volume subset checking easier. element_dB_to_step and
> element_alsa_dB_to_step help with getting the step for 0 dB
> without tons of annoying ifs. They could probably be used also
> elsewhere in the code, but I'll do that cleanup later.
As well as cleanup all PA_ALSA_VOLUME_OFF and PA_ALSA_VOLUME_ZERO usages.
> Colin, there are also a couple of questions for you in the
> comments.
I agree with the comments. Although not removing a redundant path is
less bad than removing something that should not be removed, so I'm okay
with marking them as FIXME's for the time being.
> ---
> src/modules/alsa/alsa-mixer.c | 132 ++++++++++++++++++++++++++++++-----------
> 1 files changed, 96 insertions(+), 36 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index f6a2a20..8f1fb4a 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> <at> <at> -1344,6 +1344,82 <at> <at> static int check_required(pa_alsa_element *e, snd_mixer_elem_t *me) {
> return 0;
> }
>
> +static int element_alsa_dB_to_step(pa_alsa_element *e, snd_mixer_elem_t *me, long *dB, long
*step) {
> + int r;
> +
> + pa_assert(e);
> + pa_assert(me);
> + pa_assert(dB);
> + pa_assert(step);
> +
> + if (e->direction == PA_ALSA_DIRECTION_OUTPUT) {
> + if ((r = snd_mixer_selem_ask_playback_dB_vol(me, *dB, +1, step))>= 0) {
> + *step = (e->volume_limit>= 0) ? PA_MIN(*step, e->volume_limit) : *step;
But can't volume_limit can be less than zero and still valid?
> +
> + if ((r = snd_mixer_selem_ask_playback_vol_dB(me, *step, dB)< 0)) {
> + pa_log("snd_mixer_selem_ask_playback_vol_dB() failed: %s", pa_alsa_strerror(r));
> + return -1;
> + }
Nitpick: could be worth adding a comment about why you need to ask first
dB -> vol and then vol -> dB again,
> + } else {
> + pa_log("snd_mixer_selem_ask_playback_dB_vol() failed: %s", pa_alsa_strerror(r));
> + return -1;
> + }
> + } else {
> + if ((r = snd_mixer_selem_ask_capture_dB_vol(me, *dB, -1, step))>= 0) {
> + *step = (e->volume_limit>= 0) ? PA_MIN(*step, e->volume_limit) : *step;
> +
> + if ((r = snd_mixer_selem_ask_capture_vol_dB(me, *step, dB)< 0)) {
> + pa_log("snd_mixer_selem_ask_capture_vol_dB() failed: %s", pa_alsa_strerror(r));
> + return -1;
> + }
> + } else {
> + pa_log("snd_mixer_selem_ask_capture_dB_vol() failed: %s", pa_alsa_strerror(r));
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int element_dB_to_step(pa_alsa_element *e, snd_mixer_elem_t *me, long *dB, long *step) {
> + pa_assert(e);
> + pa_assert(me);
> + pa_assert(dB);
> + pa_assert(step);
> +
> + if (e->db_fix) {
> + int rounding = (e->direction == PA_ALSA_DIRECTION_OUTPUT) ? +1 : -1;
> +
> + *step = decibel_fix_get_step(e->db_fix, dB, rounding);
> + return 0;
> + }
> +
> + return element_alsa_dB_to_step(e, me, dB, step);
> +}
> +
> +/* If element's volume_use is OFF or ZERO, it's changed to CONSTANT.
> + * If a problem occurs with alsa while doing this, volume_use will be
> + * changed to IGNORE. */
> +static void element_change_off_and_zero_to_constant(pa_alsa_element *e, snd_mixer_elem_t *me) {
> + pa_assert(e);
> + pa_assert(me);
> +
> + if (e->volume_use == PA_ALSA_VOLUME_OFF) {
> + e->volume_use = PA_ALSA_VOLUME_CONSTANT;
> + e->constant_volume = e->min_volume;
> + return;
> + }
> +
> + if (e->volume_use == PA_ALSA_VOLUME_ZERO) {
> + long dB = 0;
> + if (element_dB_to_step(e, me,&dB,&e->constant_volume)< 0) {
> + pa_log("Failed to query element's volume step for 0 dB. Ignoring volume for element %s.", e->alsa_name);
> + e->volume_use = PA_ALSA_VOLUME_IGNORE;
> + } else
> + e->volume_use = PA_ALSA_VOLUME_CONSTANT;
> + }
> +}
> +
> static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
> snd_mixer_selem_id_t *sid;
> snd_mixer_elem_t *me;
> <at> <at> -1627,9 +1703,10 <at> <at> static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
> }
> }
> }
> -
> }
>
> + element_change_off_and_zero_to_constant(e, me);
> +
> if (e->switch_use == PA_ALSA_SWITCH_SELECT) {
> pa_alsa_option *o;
>
> <at> <at> -2922,54 +2999,31 <at> <at> static pa_bool_t element_is_subset(pa_alsa_element *a, pa_alsa_element *b, snd_m
>
> /* Check the volume_use */
> if (a->volume_use != PA_ALSA_VOLUME_IGNORE) {
> + if (b->volume_use == PA_ALSA_VOLUME_IGNORE)
> + return FALSE;
> +
> + /* ZERO and OFF should have been changed into CONSTANT
> + * while probing the elements. */
> + pa_assert(a->volume_use == PA_ALSA_VOLUME_MERGE || a->volume_use == PA_ALSA_VOLUME_CONSTANT);
> + pa_assert(b->volume_use == PA_ALSA_VOLUME_MERGE || b->volume_use == PA_ALSA_VOLUME_CONSTANT);
>
> /* "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;
>
> - /* 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)
> + /* "Merge" is never a subset of "Constant" */
> + if (a->volume_use == PA_ALSA_VOLUME_MERGE&& b->volume_use == PA_ALSA_VOLUME_CONSTANT)
> return FALSE;
>
> /* "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" */
> if (b->volume_use == PA_ALSA_VOLUME_MERGE&& b->volume_limit>= 0) {
> long a_limit;
>
> if (a->volume_use == PA_ALSA_VOLUME_CONSTANT)
> a_limit = a->constant_volume;
> - else if (a->volume_use == PA_ALSA_VOLUME_ZERO) {
> - long dB = 0;
> -
> - if (a->db_fix) {
> - int rounding = (a->direction == PA_ALSA_DIRECTION_OUTPUT ? +1 : -1);
> - a_limit = decibel_fix_get_step(a->db_fix,&dB, rounding);
> - } else {
> - snd_mixer_selem_id_t *sid;
> - snd_mixer_elem_t *me;
> -
> - SELEM_INIT(sid, a->alsa_name);
> - if (!(me = snd_mixer_find_selem(m, sid))) {
> - pa_log_warn("Element %s seems to have disappeared.", a->alsa_name);
> - return FALSE;
> - }
> -
> - if (a->direction == PA_ALSA_DIRECTION_OUTPUT) {
> - if (snd_mixer_selem_ask_playback_dB_vol(me, dB, +1,&a_limit)< 0)
> - return FALSE;
> - } else {
> - if (snd_mixer_selem_ask_capture_dB_vol(me, dB, -1,&a_limit)< 0)
> - return FALSE;
> - }
> - }
> - } else if (a->volume_use == PA_ALSA_VOLUME_OFF)
> - a_limit = a->min_volume;
> - else if (a->volume_use == PA_ALSA_VOLUME_MERGE)
> - a_limit = a->volume_limit;
> else
> - /* This should never be reached */
> - pa_assert(FALSE);
> + a_limit = a->volume_limit;
>
> if (a_limit> b->volume_limit)
> return FALSE;
> <at> <at> -2977,6 +3031,9 <at> <at> static pa_bool_t element_is_subset(pa_alsa_element *a, pa_alsa_element *b, snd_m
> }
>
> if (a->switch_use != PA_ALSA_SWITCH_IGNORE) {
> + if (b->switch_use == PA_ALSA_SWITCH_IGNORE)
> + return FALSE;
> +
> /* "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.
> <at> <at> -3022,9 +3079,8 <at> <at> static void path_set_condense(pa_alsa_path_set *ps, snd_mixer_t *m) {
> if (!ps->paths || !ps->paths->next)
> return;
>
> - for (p = ps->paths; p; p = np) {
> + PA_LLIST_FOREACH_SAFE(p, np, ps->paths) {
> pa_alsa_path *p2;
> - np = p->next;
>
> PA_LLIST_FOREACH(p2, ps->paths) {
> pa_alsa_element *ea, *eb;
> <at> <at> -3043,6 +3099,8 <at> <at> static void path_set_condense(pa_alsa_path_set *ps, snd_mixer_t *m) {
> ea = ea->next;
> eb = eb->next;
> if ((ea&& !eb) || (!ea&& eb))
> + /* Question to Colin: Do we really want to require that p and p2 contain an equal amount of elements to
consider p a subset of p2?
> + * Isn't it enough if p2 contains all elements of p? */
> is_subset = FALSE;
> else if (!ea&& !eb)
> break;
> <at> <at> -3050,6 +3108,8 <at> <at> static void path_set_condense(pa_alsa_path_set *ps, snd_mixer_t *m) {
> is_subset = FALSE;
>
> } else
> + /* Question to Colin: Do we really want to require that p and p2 contain their elements in the exact same order?
> + * Is there a guarantee for that? (I haven't inspected the code that closely.) */
> is_subset = FALSE;
> }
>
--
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
RSS Feed