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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
