ask about the assumption of min volume and max volume got from ALSA

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Col,

On Thu, May 26, 2011 at 04:30:03PM +0800, Colin Guthrie wrote:
> 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?

Yes, you're right. The volume here is not related to dB. It's just plain
volume number I got from ALSA via this API (snd_mixer_selem_get_playback_volume_range).
In some ALSA drivers, I got the min_vol to be less than zero. So when I
try to set it to be a constant volume, it fails to do so.

> 
> 
> 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.

I'll change it to bool and send another separate patch for 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/]

-- 
guanqun



[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux