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

[Colin Guthrie]

Hi,

'Twas brillig, and xing wang at 26/05/11 04:52 did gyre and gimble:
> Would you please give some comments about Guanqun's patch?
> We're working on Pulseaudio and meet some bugs, i think we will run
> faster with community's help.

Absolutely!

To be honest tho', I was hoping Tanu or David would give their takes on
this patch (which they may still do) as they have a better grasp of the
audio control side than I do.


One thing I will say is that PA will use the info from alsa in such a
way as to provide a consistent dB range further up the stack.

We take the alsa-provided 0dB point and mark that as our "Base volume"
this is exposed e.g. in the pavucontrol and gnome-volume-control GUI's

This base volume is meant to indicate the "cleanest" path through the h/w.

This might not be super relevant to the specific issue in question, but
that's a little background on the dB levels exposed by alsa, but I get
the impression the volumes you're talking about are not dB related?


Looking at your patch, it seems logical enough (although for internal
code, I'd use pa_bool_t + TRUE/FALSE values, but that's just a minor
observation), so if Tanu ACKs it, I'd happily merge it.

Cheers

Col


> thanks
> --xingchao
> 
> 2011/5/25 Lu Guanqun <guanqun.lu at intel.com>:
>> Hi,
>>
>> This patch needs a little more love. :)
>> No one cared?
>>
>> On Wed, May 25, 2011 at 02:17:18PM +0800, Lu Guanqun wrote:
>>> 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
>>> _______________________________________________
>>> pulseaudio-discuss mailing list
>>> pulseaudio-discuss at mail.0pointer.de
>>> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
>>
>> --
>> guanqun
>> _______________________________________________
>> pulseaudio-discuss mailing list
>> pulseaudio-discuss at mail.0pointer.de
>> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
>>


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]