[alsa-devel] [PATCH] Add support for gain in softvol plugin
Hi all,
What do you think of this patch? I tested this using a stac9205 HDA codec with all sample formats supported by softvol (s32, s16, s24, big and little endian).
The potential drawback I see is that softvol now has to use a intermediate long long multiply and add when computing amplified 32-bit samples. This wasn't an issue with the system I tested on, but possibly could be on slower machines. Also, are their any portability issues with using a long long datatype in alsa-lib? What is the list of embedded architectures that alsa-lib is targeted to build on? I see long long used in many other places in alsa-lib, so I'm hoping this isn't an issue.
Steve
On Thu, 3 May 2007, Steve Longerbeam wrote:
Hi all,
What do you think of this patch? I tested this using a stac9205 HDA codec with all sample formats supported by softvol (s32, s16, s24, big and little endian).
I would not add extra defines to alsa.conf:
+defaults.pcm.dsnoop_softvol.card defaults.pcm.card +defaults.pcm.dsnoop_softvol.device defaults.pcm.device
Simply reuse pcm.dsnoop.card / device.
Otherwise - it looks good. Nice softvol extension.
Thanks, Jaroslav
----- Jaroslav Kysela perex@suse.cz Linux Kernel Sound Maintainer ALSA Project, SUSE Labs
Jaroslav Kysela wrote:
On Thu, 3 May 2007, Steve Longerbeam wrote:
Hi all,
What do you think of this patch? I tested this using a stac9205 HDA codec with all sample formats supported by softvol (s32, s16, s24, big and little endian).
I would not add extra defines to alsa.conf:
+defaults.pcm.dsnoop_softvol.card defaults.pcm.card +defaults.pcm.dsnoop_softvol.device defaults.pcm.device
Simply reuse pcm.dsnoop.card / device.
Hi Jaroslav,
How do I do this exactly? Should I add:
@args.CARD { type string default { @func refer name defaults.pcm.card } }
to dsnoop_softvol.conf?
Otherwise - it looks good. Nice softvol extension.
thanks, Steve
On Thu, 3 May 2007, Steve Longerbeam wrote:
Jaroslav Kysela wrote:
On Thu, 3 May 2007, Steve Longerbeam wrote:
Hi all,
What do you think of this patch? I tested this using a stac9205 HDA codec with all sample formats supported by softvol (s32, s16, s24, big and little endian).
I would not add extra defines to alsa.conf:
+defaults.pcm.dsnoop_softvol.card defaults.pcm.card +defaults.pcm.dsnoop_softvol.device defaults.pcm.device
Simply reuse pcm.dsnoop.card / device.
Hi Jaroslav,
How do I do this exactly? Should I add:
@args.CARD { type string default { @func refer name defaults.pcm.card
Replace with: name defaults.pcm.dsnoop.card
Please, resend your patch with this modification. Thanks.
Jaroslav
----- Jaroslav Kysela perex@suse.cz Linux Kernel Sound Maintainer ALSA Project, SUSE Labs
Jaroslav Kysela wrote:
On Thu, 3 May 2007, Steve Longerbeam wrote:
Jaroslav Kysela wrote:
On Thu, 3 May 2007, Steve Longerbeam wrote:
Hi all,
What do you think of this patch? I tested this using a stac9205 HDA codec with all sample formats supported by softvol (s32, s16, s24, big and little endian).
I would not add extra defines to alsa.conf:
+defaults.pcm.dsnoop_softvol.card defaults.pcm.card +defaults.pcm.dsnoop_softvol.device defaults.pcm.device
Simply reuse pcm.dsnoop.card / device.
Hi Jaroslav,
How do I do this exactly? Should I add:
@args.CARD { type string default { @func refer name defaults.pcm.card
Replace with: name defaults.pcm.dsnoop.card
Please, resend your patch with this modification. Thanks.
ok, here it is again, with CARD/DEV defaults moved out of alsa.conf and into dsnoop_softvol.conf.
Steve
Steve Longerbeam wrote:
Jaroslav Kysela wrote:
<snip>
Please, resend your patch with this modification. Thanks.
ok, here it is again, with CARD/DEV defaults moved out of alsa.conf and into dsnoop_softvol.conf.
I realized I wasn't handling max_dB < 0 (svol->zero_dB_val = 0) in all cases.
Patch attached again along with an interdiff from my last patch.
Steve
diff -u b/src/pcm/pcm_softvol.c b/src/pcm/pcm_softvol.c --- b/src/pcm/pcm_softvol.c Fri May 04 09:22:49 2007 -0700 +++ b/src/pcm/pcm_softvol.c Fri May 04 10:25:39 2007 -0700 @@ -594,14 +594,17 @@ { int err; int i; - + unsigned int def_val; + err = snd_ctl_elem_add_integer(svol->ctl, &cinfo->id, count, 0, svol->max_val, 0); if (err < 0) return err; add_tlv_info(svol, cinfo); - /* set zero dB value as default */ + /* set zero dB value as default, or max_val if + there is no 0 dB setting */ + def_val = svol->zero_dB_val ? svol->zero_dB_val : svol->max_val; for (i = 0; i < count; i++) - svol->elem.value.integer.value[i] = svol->zero_dB_val; + svol->elem.value.integer.value[i] = def_val; return snd_ctl_elem_write(svol->ctl, &svol->elem); }
@@ -706,7 +709,8 @@ double v = (pow(10.0, db / 20.0) * (double)(1 << VOL_SCALE_SHIFT)); svol->dB_value[i] = (unsigned int)v; } - svol->dB_value[svol->zero_dB_val] = 65535; + if (svol->zero_dB_val) + svol->dB_value[svol->zero_dB_val] = 65535; #else SNDERR("Cannot handle the given dB range and resolution"); return -EINVAL;
At Fri, 04 May 2007 10:33:16 -0700, Steve Longerbeam wrote:
Steve Longerbeam wrote:
Jaroslav Kysela wrote:
<snip>
Please, resend your patch with this modification. Thanks.
ok, here it is again, with CARD/DEV defaults moved out of alsa.conf and into dsnoop_softvol.conf.
I realized I wasn't handling max_dB < 0 (svol->zero_dB_val = 0) in all cases.
Patch attached again along with an interdiff from my last patch.
Thanks for the patch. Now I checked this thread.
Some suggestions:
- "Capture SoftVol" doesn't sound like a valid mixer name. "Digital Capture Volume" would be more suitable, IMO.
- The range from -30 to 40dB seems too big.
- We don't need a new definition of dsnoop_softvol PCM there as it's specific to HDA-Intel right now. Let's define locally like:
# default with dmix+softvol & dsnoop HDA-Intel.pcm.default { @args [ CARD ] @args.CARD { type string } type asym playback.pcm { ... } capture.pcm { type plug slave.pcm { type softvol slave.pcm { @func concat strings [ "dsnoop:" $CARD ] } control { name "Digital Capture Volume" card $CARD } min_dB -30.0 max_dB 40.0 } } }
Takashi
Takashi Iwai wrote:
At Fri, 04 May 2007 10:33:16 -0700, Steve Longerbeam wrote:
Steve Longerbeam wrote:
Jaroslav Kysela wrote:
<snip>
Please, resend your patch with this modification. Thanks.
ok, here it is again, with CARD/DEV defaults moved out of alsa.conf and into dsnoop_softvol.conf.
I realized I wasn't handling max_dB < 0 (svol->zero_dB_val = 0) in all cases.
Patch attached again along with an interdiff from my last patch.
Thanks for the patch. Now I checked this thread.
Some suggestions:
"Capture SoftVol" doesn't sound like a valid mixer name. "Digital Capture Volume" would be more suitable, IMO.
The range from -30 to 40dB seems too big.
We don't need a new definition of dsnoop_softvol PCM there as it's specific to HDA-Intel right now. Let's define locally like:
# default with dmix+softvol & dsnoop HDA-Intel.pcm.default { @args [ CARD ] @args.CARD { type string } type asym playback.pcm { ... } capture.pcm { type plug slave.pcm { type softvol slave.pcm { @func concat strings [ "dsnoop:" $CARD ] } control { name "Digital Capture Volume" card $CARD } min_dB -30.0 max_dB 40.0 } } }
Takashi
Hi Takashi,
Thanks, that's much better. I tried doing that myself a while ago, but I must have been doing something wrong because the capture device wasn't being recognized. But your version works.
Version 4 of the patch is attached. It's much better, only two modified files: pcm_softvol.c and HDA-Intel.conf.
I also reduced the gain range, it's now -30 to +30 dB.
Steve
At Thu, 10 May 2007 12:57:10 -0700, Steve Longerbeam wrote:
Hi Takashi,
Thanks, that's much better. I tried doing that myself a while ago, but I must have been doing something wrong because the capture device wasn't being recognized. But your version works.
Version 4 of the patch is attached. It's much better, only two modified files: pcm_softvol.c and HDA-Intel.conf.
I also reduced the gain range, it's now -30 to +30 dB.
I'm afraid that this choice would be bad for the current TLV representation. The default resolution is 256, so -30/30 dB range doesn't match to this resolution. We should change either resolution (e.g. 120, corresponding to 0.5dB step), or the range.
Otherwise it looks fine to me.
Thanks,
Takashi
Takashi Iwai wrote:
At Thu, 10 May 2007 12:57:10 -0700, Steve Longerbeam wrote:
Hi Takashi,
Thanks, that's much better. I tried doing that myself a while ago, but I must have been doing something wrong because the capture device wasn't being recognized. But your version works.
Version 4 of the patch is attached. It's much better, only two modified files: pcm_softvol.c and HDA-Intel.conf.
I also reduced the gain range, it's now -30 to +30 dB.
I'm afraid that this choice would be bad for the current TLV representation. The default resolution is 256, so -30/30 dB range doesn't match to this resolution. We should change either resolution (e.g. 120, corresponding to 0.5dB step), or the range.
ok, that's no problem. How about we change the range to (in HDA-Intel.conf):
min_dB: -21.0 max_dB: 30.0
Steve
Otherwise it looks fine to me.
Thanks,
Takashi
Steve Longerbeam wrote:
Takashi Iwai wrote:
At Thu, 10 May 2007 12:57:10 -0700, Steve Longerbeam wrote:
Hi Takashi,
Thanks, that's much better. I tried doing that myself a while ago, but I must have been doing something wrong because the capture device wasn't being recognized. But your version works.
Version 4 of the patch is attached. It's much better, only two modified files: pcm_softvol.c and HDA-Intel.conf.
I also reduced the gain range, it's now -30 to +30 dB.
I'm afraid that this choice would be bad for the current TLV representation. The default resolution is 256, so -30/30 dB range doesn't match to this resolution. We should change either resolution (e.g. 120, corresponding to 0.5dB step), or the range.
ok, that's no problem. How about we change the range to (in HDA-Intel.conf):
min_dB: -21.0 max_dB: 30.0
Hi Takashi, should I send another patch with this change?
Steve
Otherwise it looks fine to me.
Thanks,
Takashi
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Thu, 17 May 2007 15:00:35 -0700, Steve Longerbeam wrote:
Steve Longerbeam wrote:
Takashi Iwai wrote: At Thu, 10 May 2007 12:57:10 -0700, Steve Longerbeam wrote: Hi Takashi, Thanks, that's much better. I tried doing that myself a while ago, but I must have been doing something wrong because the capture device wasn't being recognized. But your version works. Version 4 of the patch is attached. It's much better, only two modified files: pcm_softvol.c and HDA-Intel.conf. I also reduced the gain range, it's now -30 to +30 dB. I'm afraid that this choice would be bad for the current TLV representation. The default resolution is 256, so -30/30 dB range doesn't match to this resolution. We should change either resolution (e.g. 120, corresponding to 0.5dB step), or the range. ok, that's no problem. How about we change the range to (in HDA-Intel.conf): min_dB: -21.0 max_dB: 30.0Hi Takashi, should I send another patch with this change?
Well, -21/30 isn't better than -30/30 since it doesn't match with the default resolutions (= 256). So, I think -30/30 choice is good, but it just needs the adjustment of resolution.
I committed your last patch with this fix now to ALSA HG tree.
Thanks!
Takashi
participants (3)
-
Jaroslav Kysela -
Steve Longerbeam -
Takashi Iwai