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
@@ -1344,6 +1344,82 @@ 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;
@@ -1627,9 +1703,10 @@ 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;
@@ -2922,54 +2999,31 @@ 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;
@@ -2977,6 +3031,9 @@ 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.
@@ -3022,9 +3079,8 @@ 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;
@@ -3043,6 +3099,8 @@ 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;
@@ -3050,6 +3108,8 @@ 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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
