[pulseaudio-discuss] ask about the assumption of min volume and max volume got from ALSA

[Lu Guanqun]

Hi list,

I may not have the background on this issue, but from the code, it seems
the code assumes min_volume and max_volume should be non negative. Is
this the right assumption we should take? What about some drivers
exposes that min_volume is -126 and max_volume is 0? Should we fix the
driver or generalize pulseaudio code to tolerate these issues?

If we allow min_volume could be less than zero, and how about the below
patch in your point of view? Could you have a review of the below patch?
If that's OK, I can send a standalone patch for it.

>From 1f5c8fa0e80d9fe2e3d56133afa8fe9a5dbe6693 Mon Sep 17 00:00:00 2001
From: Lu Guanqun <guanqun.lu at intel.com>
Date: Wed, 25 May 2011 14:12:52 +0800
Subject: [PATCH] fix the assumption that volume is always positive

Add a variable to track whether the actual volume is set or not.
Suppose this:
	min volume: -126	max volume: 0
then when user wants to set some constant volume to -10, it would fail.

Signed-off-by: Lu Guanqun <guanqun.lu at intel.com>
---
 src/modules/alsa/alsa-mixer.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 1bd709a..23651b0 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -1123,6 +1123,7 @@ static int element_set_constant_volume(pa_alsa_element *e, snd_mixer_t *m) {
     snd_mixer_selem_id_t *sid = NULL;
     int r = 0;
     long volume = -1;
+    int volume_set = 0;
 
     pa_assert(m);
     pa_assert(e);
@@ -1136,6 +1137,7 @@ static int element_set_constant_volume(pa_alsa_element *e, snd_mixer_t *m) {
     switch (e->volume_use) {
         case PA_ALSA_VOLUME_OFF:
             volume = e->min_volume;
+            volume_set = 1;
             break;
 
         case PA_ALSA_VOLUME_ZERO:
@@ -1143,18 +1145,20 @@ static int element_set_constant_volume(pa_alsa_element *e, snd_mixer_t *m) {
                 long dB = 0;
 
                 volume = decibel_fix_get_step(e->db_fix, &dB, +1);
+                volume_set = 1;
             }
             break;
 
         case PA_ALSA_VOLUME_CONSTANT:
             volume = e->constant_volume;
+            volume_set = 1;
             break;
 
         default:
             pa_assert_not_reached();
     }
 
-    if (volume >= 0) {
+    if (volume_set) {
         if (e->direction == PA_ALSA_DIRECTION_OUTPUT)
             r = snd_mixer_selem_set_playback_volume_all(me, volume);
         else
-- 
1.7.2.3

-- 
guanqun