At Tue, 30 Sep 2008 22:51:23 +0200, Alex wrote:
This should be interesting for home theater installations, as I was personnally missing a "per channel delay configuration".
Feel free to tell me what should be modified, I am not really used to proposing a patch.
Thanks for the patch. Just looking quickly at it, some comments below.
diff -x libtool -x stamp-h1 -x .vimrc -x rate -x Makefile.in -x Makefile -x .deps -x configure -x config.status -x config.h -x config.log -x a52 -x '*cache*' -x '*aclocal*' -Naur alsa-plugins-1.0.17/configure.in alsa-plugins-1.0.17+delay/configure.in
Maybe it's a good chance to learn git now :)
--- alsa-plugins-1.0.17/configure.in 2008-07-14 10:57:59.000000000 +0200 +++ alsa-plugins-1.0.17+delay/configure.in 2008-09-24 10:52:23.000000000 +0200 @@ -89,6 +89,9 @@ fi
AM_CONDITIONAL(HAVE_PPH, test "$PPH" = "builtin" -o "$PPH" = "lib")
+AM_CONDITIONAL(HAVE_DELAY, test x$HAVE_DELAY = xyes)
AM_CONDITIONAL(USE_LIBSPEEX, test "$PPH" = "lib")
dnl ALSA plugin directory @@ -124,6 +127,7 @@ a52/Makefile rate-lavc/Makefile maemo/Makefile
- delay/Makefile
Try to keep the same indent.
--- alsa-plugins-1.0.17/delay/pcm_delay.c 1970-01-01 01:00:00.000000000 +0100 +++ alsa-plugins-1.0.17+delay/delay/pcm_delay.c 2008-09-30 21:57:56.000000000 +0200
(snip)
+/* Ambient temperature is 20� celsius */
Use ASCII as much as possible. If inevitably necessary, use UTF-8.
+typedef struct {
- snd_pcm_extplug_t ext;
- double *delay_ms;
- unsigned int *delay_sample;
- snd_pcm_channel_area_t *dly_areas;
- snd_pcm_uframes_t *dly_offset;
- unsigned int channels;
- unsigned int mode;
+} snd_pcm_delay_t;
Could you keep the indent level like other codes? Not mandatory, but would be better for us...
+static int snd_pcm_delay_parse_delay(snd_pcm_delay_t *pcm_delay,
snd_config_t *conf_delay,unsigned char mode)+{
- snd_config_iterator_t i, next;
- double *delays;
- double delay = 0.0;
- unsigned int cnt = 0;
- unsigned int channel;
- double val;
- pcm_delay->mode = mode;
- if (conf_delay) {
snd_config_for_each(i, next, conf_delay) {snd_config_t *n = snd_config_iterator_entry(i);const char *id;if (snd_config_get_id(n, &id) < 0)continue;cnt++;}
This means cnt includes comments, etc? Then the number is inaccurate (although a bigger doesn't matter much).
pcm_delay->delay_ms = delays = calloc(cnt, sizeof(double));if (delays == NULL)return -ENOMEM;pcm_delay->channels = cnt;
Now you have two "channels", one in this definition and another as pcm_delay->ext.channels. As far as I see, these are used sometimes wrongly, possibly resulting in a segfault.
+static snd_pcm_sframes_t snd_pcm_delay_transfer(snd_pcm_extplug_t *ext,
const snd_pcm_channel_area_t *dst_areas,snd_pcm_uframes_t dst_offset,const snd_pcm_channel_area_t *src_areas,snd_pcm_uframes_t src_offset,snd_pcm_uframes_t size)+{
- snd_pcm_delay_t *pcm_delay = (snd_pcm_delay_t *)ext;
- unsigned int i;
- snd_pcm_uframes_t left;
- for (i = 0; i < pcm_delay->ext.channels; i++) {
if (! pcm_delay->delay_sample[i]) {snd_pcm_area_copy(dst_areas + i, dst_offset,src_areas + i, src_offset,size, pcm_delay->ext.format);continue;}if (pcm_delay->delay_sample[i] <= size) {assert(! pcm_delay->dly_offset[i]);
I'm not sure whether this condition is assured... The size in this callback isn't guaranteed to be a period size.
left = size - pcm_delay->delay_sample[i];/* delay to dst */snd_pcm_area_copy(dst_areas + i, dst_offset,pcm_delay->dly_areas + i, 0,pcm_delay->delay_sample[i], pcm_delay->ext.format);if (left) {/* src to dst left */snd_pcm_area_copy(dst_areas + i, dst_offset + pcm_delay->delay_sample[i],src_areas + i, src_offset,left, pcm_delay->ext.format);}/* src left to delay */snd_pcm_area_copy(pcm_delay->dly_areas + i, 0,src_areas + i, src_offset + left,pcm_delay->delay_sample[i], pcm_delay->ext.format);continue;}/* usual (small) delays wont reach this part of the code */left = pcm_delay->dly_offset[i] + size > pcm_delay->delay_sample[i]? (pcm_delay->dly_offset[i] + size) % pcm_delay->delay_sample[i] : 0;/* copy dly to dst */snd_pcm_area_copy(dst_areas + i, dst_offset,pcm_delay->dly_areas + i, pcm_delay->dly_offset[i],size - left, pcm_delay->ext.format);/* copy src to dly */snd_pcm_area_copy(pcm_delay->dly_areas + i, pcm_delay->dly_offset[i],src_areas + i, src_offset,size - left, pcm_delay->ext.format);if (left) {/* copy dly to dst */snd_pcm_area_copy(dst_areas + i, dst_offset + size - left,pcm_delay->dly_areas + i, 0,left, pcm_delay->ext.format);/* copy src to dly */snd_pcm_area_copy(pcm_delay->dly_areas + i, 0,src_areas + i, src_offset + size - left,left, pcm_delay->ext.format);pcm_delay->dly_offset[i] = left;continue;}pcm_delay->dly_offset[i] += size;
Hmm, the whole copy mechanism is a bit too hard. I guess this could be simplified more.
+/*
- Main entry point
- */
+SND_PCM_PLUGIN_DEFINE_FUNC(delay)
...
- snd_pcm_extplug_set_param_minmax(&pcm_delay->ext,
SND_PCM_EXTPLUG_HW_CHANNELS,1, 6);- snd_pcm_extplug_set_param_list(&pcm_delay->ext,
SND_PCM_EXTPLUG_HW_CHANNELS,6, chlist);
You don't need both of them. Both do the same job.
- snd_pcm_extplug_set_param(&pcm_delay->ext, SND_PCM_EXTPLUG_HW_FORMAT,
SND_PCM_FORMAT_S16);- snd_pcm_extplug_set_slave_param(&pcm_delay->ext, SND_PCM_EXTPLUG_HW_FORMAT,
SND_PCM_FORMAT_S16);
Any reason to restrict to S16? Just for copying samples, the sample format doesn't matter much.
thanks,
Takashi