[PATCH] Bugfix: Fix resampling when client and slave both use format float

[PATCH] Bugfix: Fix resampling when client and slave both use format float

[Maarten Baert]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

I found a bug in libasound. When both the client and the slave (in my
case, the JACK plugin) try to use the float format, but the sample rate
or channel count does not match, libasound *should* insert linear
conversion plugins to convert from float to linear, then resample/remap
channels, and then convert back to float (because apparently the
resamplers and channel remapper don't support floating point, only
'linear' i.e. integers). Currently this doesn't work,
snd_pcm_plug_change_format doesn't know what to do and simply returns
EINVAL. As a result, snd_pcm_hw_params fails even though the HW params
were perfectly valid (it indicates that both the float format and any
sample rate are supported).

In my test, this broke audio for WINE (and any other application that
tries to use float, such as aplay with the right settings) when I wanted
to use the JACK plugin (which only supports the float format).

This patch fixes this bug by doing a conversion to S16 and back when
resampling or remapping is needed. And while I was at it, I also removed
a check that had no effect because the exact same check is already being
done at the start of the function.

I still think it's a bit silly that libasound requires integers for
resampling, because both libsamplerate and libspeex use float internally
for resampling. So now ALSA is literally doing a
float-to-s16-to-float-to-s16-to-float conversion. But changing that
would have been a lot harder.

Maarten Baert

[-- Attachment #2: fix-float-resampling.patch --]
[-- Type: text/x-patch, Size: 864 bytes --]

diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c
index fa84eaa..ede9c15 100644
--- a/src/pcm/pcm_plug.c
+++ b/src/pcm/pcm_plug.c
@@ -522,15 +522,13 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p
 		}
 #ifdef BUILD_PCM_PLUGIN_LFLOAT
 	} else if (snd_pcm_format_float(slv->format)) {
-		/* Conversion is done in another plugin */
-		if (clt->format == slv->format &&
-		    clt->rate == slv->rate &&
-		    clt->channels == slv->channels)
-			return 0;
-		cfmt = clt->format;
-		if (snd_pcm_format_linear(clt->format))
+		if (snd_pcm_format_linear(clt->format)) {
+			cfmt = clt->format;
 			f = snd_pcm_lfloat_open;
-		else
+		} else if (clt->rate != slv->rate || clt->channels != slv->channels) {
+			cfmt = SND_PCM_FORMAT_S16;
+			f = snd_pcm_lfloat_open;
+		} else
 			return -EINVAL;
 #endif
 #ifdef BUILD_PCM_NONLINEAR

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Fri, 21 Feb 2014 21:01:00 +0100,
Maarten Baert wrote:
> 
> I found a bug in libasound. When both the client and the slave (in my
> case, the JACK plugin) try to use the float format, but the sample rate
> or channel count does not match, libasound *should* insert linear
> conversion plugins to convert from float to linear, then resample/remap
> channels, and then convert back to float (because apparently the
> resamplers and channel remapper don't support floating point, only
> 'linear' i.e. integers). Currently this doesn't work,
> snd_pcm_plug_change_format doesn't know what to do and simply returns
> EINVAL. As a result, snd_pcm_hw_params fails even though the HW params
> were perfectly valid (it indicates that both the float format and any
> sample rate are supported).
> 
> In my test, this broke audio for WINE (and any other application that
> tries to use float, such as aplay with the right settings) when I wanted
> to use the JACK plugin (which only supports the float format).
> 
> This patch fixes this bug by doing a conversion to S16 and back when
> resampling or remapping is needed. And while I was at it, I also removed
> a check that had no effect because the exact same check is already being
> done at the start of the function.
> 
> I still think it's a bit silly that libasound requires integers for
> resampling, because both libsamplerate and libspeex use float internally
> for resampling. So now ALSA is literally doing a
> float-to-s16-to-float-to-s16-to-float conversion. But changing that
> would have been a lot harder.

Can S32 work instead of S16?  Then we won't lose the accuracy so much.
Of course, handling float directly would be the best option.

In anyway, could you give your acked-by tag?

Thanks!


Takashi

> 
> Maarten Baert
> diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c
> index fa84eaa..ede9c15 100644
> --- a/src/pcm/pcm_plug.c
> +++ b/src/pcm/pcm_plug.c
> @@ -522,15 +522,13 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p
>  		}
>  #ifdef BUILD_PCM_PLUGIN_LFLOAT
>  	} else if (snd_pcm_format_float(slv->format)) {
> -		/* Conversion is done in another plugin */
> -		if (clt->format == slv->format &&
> -		    clt->rate == slv->rate &&
> -		    clt->channels == slv->channels)
> -			return 0;
> -		cfmt = clt->format;
> -		if (snd_pcm_format_linear(clt->format))
> +		if (snd_pcm_format_linear(clt->format)) {
> +			cfmt = clt->format;
>  			f = snd_pcm_lfloat_open;
> -		else
> +		} else if (clt->rate != slv->rate || clt->channels != slv->channels) {
> +			cfmt = SND_PCM_FORMAT_S16;
> +			f = snd_pcm_lfloat_open;
> +		} else
>  			return -EINVAL;
>  #endif
>  #ifdef BUILD_PCM_NONLINEAR
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Maarten Baert]

On 24/02/14 10:17, Takashi Iwai wrote:
> Can S32 work instead of S16?  Then we won't lose the accuracy so much.
> Of course, handling float directly would be the best option.
The samplerate and speexrate plugins currently take S16 (see
pcm_src_convert_s16 in alsa-plugins/rate/rate_samplerate.c and
alsa-plugins/rate/rate_speexrate.c), so just using S32 will not improve
the accuracy. It would be easy to replace those functions with
pcm_src_convert_float (both resampler libraries have functions that take
float directly), but that will break the plugin ABI. Is that acceptable?
The same would have to be done for the channel remapping (route
conversion) plugin.

> In anyway, could you give your acked-by tag?
Sorry, I don't know what you mean - this is the first time I've
submitted a patch here.

Maarten Baert

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Pavel Hofman]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On 24.2.2014 13:13, Maarten Baert wrote:
> On 24/02/14 10:17, Takashi Iwai wrote:
>> Can S32 work instead of S16?  Then we won't lose the accuracy so much.
>> Of course, handling float directly would be the best option.
> The samplerate and speexrate plugins currently take S16 (see
> pcm_src_convert_s16 in alsa-plugins/rate/rate_samplerate.c and
> alsa-plugins/rate/rate_speexrate.c), so just using S32 will not improve
> the accuracy. It would be easy to replace those functions with
> pcm_src_convert_float (both resampler libraries have functions that take
> float directly), but that will break the plugin ABI. Is that acceptable?

Here is my older patch for the convertor, I know it needs a bit of work
before accepting (originally discussed at
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/041368.html )

If this project gets done, there is a CPU-efficient libsoxr convertor
plugin in the pipeline too.

Thanks a lot,

Pavel.

[-- Attachment #2: 0001-Support-for-float-samples-in-rate-converter-plugins.patch --]
[-- Type: text/x-diff, Size: 7066 bytes --]

>From 628930dabaa8b8e3f79a03a092a63ee44bfdb98e Mon Sep 17 00:00:00 2001
From: Pavel Hofman <pavel.hofman@ivitera.com>
Date: Tue, 28 Jun 2011 08:50:23 +0200
Subject: [ALSA-LIB 1/1] Support for float samples in rate converter plugins

Some rate converters have native float resolution, no need
to loose information by converting to s16.

Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>

diff --git a/include/pcm_rate.h b/include/pcm_rate.h
index 4d70df2..e92900b 100644
--- a/include/pcm_rate.h
+++ b/include/pcm_rate.h
@@ -91,6 +91,11 @@ typedef struct snd_pcm_rate_ops {
 	void (*convert_s16)(void *obj, int16_t *dst, unsigned int dst_frames,
 			    const int16_t *src, unsigned int src_frames);
 	/**
+	 * convert a float interleaved-data array; exclusive with convert
+	 */
+	void (*convert_float)(void *obj, float *dst, unsigned int dst_frames,
+			    const float *src, unsigned int src_frames);
+	/**
 	 * compute the frame size for input
 	 */
 	snd_pcm_uframes_t (*input_frames)(void *obj, snd_pcm_uframes_t frames);
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 54a3e67..2831f23 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -35,7 +35,7 @@
 #include "iatomic.h"
 
 #include "plugin_ops.h"
-
+typedef float float_t;
 #if 0
 #define DEBUG_REFINE
 #endif
@@ -66,8 +66,8 @@ struct _snd_pcm_rate {
 	snd_pcm_rate_ops_t ops;
 	unsigned int get_idx;
 	unsigned int put_idx;
-	int16_t *src_buf;
-	int16_t *dst_buf;
+	void *src_buf;
+	void *dst_buf;
 	int start_pending; /* start is triggered but not commited to slave */
 	snd_htimestamp_t trigger_tstamp;
 	unsigned int plugin_version;
@@ -310,13 +310,23 @@ static int snd_pcm_rate_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
 		rate->sareas[chn].step = swidth;
 	}
 
-	if (rate->ops.convert_s16) {
+	if (rate->ops.convert_float) {
+		rate->get_idx = snd_pcm_linear_get_index(rate->info.in.format, SND_PCM_FORMAT_S32);
+		rate->put_idx = snd_pcm_linear_put_index(SND_PCM_FORMAT_S32, rate->info.out.format);
+		free(rate->src_buf);
+		rate->src_buf = (float_t *) malloc(channels * rate->info.in.period_size * sizeof(float_t));
+		free(rate->dst_buf);
+		rate->dst_buf = (float_t *) malloc(channels * rate->info.out.period_size * sizeof(float_t));
+		if (! rate->src_buf || ! rate->dst_buf)
+			goto error;
+	}
+	else if (rate->ops.convert_s16) {
 		rate->get_idx = snd_pcm_linear_get_index(rate->info.in.format, SND_PCM_FORMAT_S16);
 		rate->put_idx = snd_pcm_linear_put_index(SND_PCM_FORMAT_S16, rate->info.out.format);
 		free(rate->src_buf);
-		rate->src_buf = malloc(channels * rate->info.in.period_size * 2);
+		rate->src_buf = (int16_t *) malloc(channels * rate->info.in.period_size * 2);
 		free(rate->dst_buf);
-		rate->dst_buf = malloc(channels * rate->info.out.period_size * 2);
+		rate->dst_buf = (int16_t *) malloc(channels * rate->info.out.period_size * 2);
 		if (! rate->src_buf || ! rate->dst_buf)
 			goto error;
 	}
@@ -503,6 +513,85 @@ static void convert_from_s16(snd_pcm_rate_t *rate, const int16_t *buf,
 		}
 	}
 }
+static void convert_to_float(snd_pcm_rate_t *rate, float_t *buf,
+			   const snd_pcm_channel_area_t *areas,
+			   snd_pcm_uframes_t offset, unsigned int frames,
+			   unsigned int channels)
+{
+#ifndef DOC_HIDDEN
+#define GET32_LABELS
+#include "plugin_ops.h"
+#undef GET32_LABELS
+#endif /* DOC_HIDDEN */
+	void *get32 = get32_labels[rate->get_idx];
+	const char *src;
+	int32_t sample;
+	const char *srcs[channels];
+	int src_step[channels];
+	unsigned int c;
+
+	for (c = 0; c < channels; c++) {
+		srcs[c] = snd_pcm_channel_area_addr(areas + c, offset);
+		src_step[c] = snd_pcm_channel_area_step(areas + c);
+	}
+
+	while (frames--) {
+		for (c = 0; c < channels; c++) {
+			src = srcs[c];
+			// converting the source format to int32 first
+			goto *get32;
+#ifndef DOC_HIDDEN
+#define GET32_END after_get
+#include "plugin_ops.h"
+#undef GET32_END
+#endif /* DOC_HIDDEN */
+		after_get:
+			// converting to float for the plugin
+			*buf++ = (float_t) sample;
+			srcs[c] += src_step[c];
+		}
+	}
+}
+
+static void convert_from_float(snd_pcm_rate_t *rate, const float_t *buf,
+			     const snd_pcm_channel_area_t *areas,
+			     snd_pcm_uframes_t offset, unsigned int frames,
+			     unsigned int channels)
+{
+#ifndef DOC_HIDDEN
+#define PUT32_LABELS
+#include "plugin_ops.h"
+#undef PUT32_LABELS
+#endif /* DOC_HIDDEN */
+	void *put32 = put32_labels[rate->put_idx];
+	char *dst;
+	int32_t sample;
+	char *dsts[channels];
+	int dst_step[channels];
+	unsigned int c;
+
+	for (c = 0; c < channels; c++) {
+		dsts[c] = snd_pcm_channel_area_addr(areas + c, offset);
+		dst_step[c] = snd_pcm_channel_area_step(areas + c);
+	}
+
+	while (frames--) {
+		for (c = 0; c < channels; c++) {
+			dst = dsts[c];
+			// plugin returned float, converting to int32 first
+			sample = (int32_t) *buf++;
+			// converting int32 to the destination format
+			goto *put32;
+#ifndef DOC_HIDDEN
+#define PUT32_END after_put
+#include "plugin_ops.h"
+#undef PUT32_END
+#endif /* DOC_HIDDEN */
+		after_put:
+			dsts[c] += dst_step[c];
+		}
+	}
+}
 
 static void do_convert(const snd_pcm_channel_area_t *dst_areas,
 		       snd_pcm_uframes_t dst_offset, unsigned int dst_frames,
@@ -511,18 +600,36 @@ static void do_convert(const snd_pcm_channel_area_t *dst_areas,
 		       unsigned int channels,
 		       snd_pcm_rate_t *rate)
 {
-	if (rate->ops.convert_s16) {
+	if (rate->ops.convert_float) {
+		const float_t *src;
+		float_t *dst;
+		if (! rate->src_buf)
+			src = src_areas->addr + src_offset * sizeof(float_t) * channels;
+		else {
+			convert_to_float(rate, rate->src_buf, src_areas, src_offset,
+				       src_frames, channels);
+			src = rate->src_buf;
+		}
+		if (! rate->dst_buf)
+			dst = dst_areas->addr + dst_offset * sizeof(float_t) * channels;
+		else
+			dst = rate->dst_buf;
+		rate->ops.convert_float(rate->obj, dst, dst_frames, src, src_frames);
+		if (dst == rate->dst_buf)
+			convert_from_float(rate, rate->dst_buf, dst_areas, dst_offset,
+					 dst_frames, channels);
+	} else if (rate->ops.convert_s16) {
 		const int16_t *src;
 		int16_t *dst;
 		if (! rate->src_buf)
-			src = src_areas->addr + src_offset * 2 * channels;
+			src = src_areas->addr + src_offset * sizeof(int16_t) * channels;
 		else {
 			convert_to_s16(rate, rate->src_buf, src_areas, src_offset,
 				       src_frames, channels);
 			src = rate->src_buf;
 		}
 		if (! rate->dst_buf)
-			dst = dst_areas->addr + dst_offset * 2 * channels;
+			dst = dst_areas->addr + dst_offset * sizeof(int16_t) * channels;
 		else
 			dst = rate->dst_buf;
 		rate->ops.convert_s16(rate->obj, dst, dst_frames, src, src_frames);
@@ -1416,7 +1523,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
 	}
 #endif
 
-	if (! rate->ops.init || ! (rate->ops.convert || rate->ops.convert_s16) ||
+	if (! rate->ops.init || ! (rate->ops.convert || rate->ops.convert_s16 || rate->ops.convert_float) ||
 	    ! rate->ops.input_frames || ! rate->ops.output_frames) {
 		SNDERR("Inproper rate plugin %s initialization", type);
 		snd_pcm_free(pcm);
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Mon, 24 Feb 2014 13:57:24 +0100,
Pavel Hofman wrote:
> 
> On 24.2.2014 13:13, Maarten Baert wrote:
> > On 24/02/14 10:17, Takashi Iwai wrote:
> >> Can S32 work instead of S16?  Then we won't lose the accuracy so much.
> >> Of course, handling float directly would be the best option.
> > The samplerate and speexrate plugins currently take S16 (see
> > pcm_src_convert_s16 in alsa-plugins/rate/rate_samplerate.c and
> > alsa-plugins/rate/rate_speexrate.c), so just using S32 will not improve
> > the accuracy. It would be easy to replace those functions with
> > pcm_src_convert_float (both resampler libraries have functions that take
> > float directly), but that will break the plugin ABI. Is that acceptable?
> 
> Here is my older patch for the convertor, I know it needs a bit of work
> before accepting (originally discussed at
> http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/041368.html )
> 
> If this project gets done, there is a CPU-efficient libsoxr convertor
> plugin in the pipeline too.

Yeah, we'll need some change like this, but need the version checks
and process the new process_float ops only for the new protocol
version for keeping the backward compatibility, too.


thanks,

Takashi

> 
> Thanks a lot,
> 
> Pavel.
> >From 628930dabaa8b8e3f79a03a092a63ee44bfdb98e Mon Sep 17 00:00:00 2001
> From: Pavel Hofman <pavel.hofman@ivitera.com>
> Date: Tue, 28 Jun 2011 08:50:23 +0200
> Subject: [ALSA-LIB 1/1] Support for float samples in rate converter plugins
> 
> Some rate converters have native float resolution, no need
> to loose information by converting to s16.
> 
> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> 
> diff --git a/include/pcm_rate.h b/include/pcm_rate.h
> index 4d70df2..e92900b 100644
> --- a/include/pcm_rate.h
> +++ b/include/pcm_rate.h
> @@ -91,6 +91,11 @@ typedef struct snd_pcm_rate_ops {
>  	void (*convert_s16)(void *obj, int16_t *dst, unsigned int dst_frames,
>  			    const int16_t *src, unsigned int src_frames);
>  	/**
> +	 * convert a float interleaved-data array; exclusive with convert
> +	 */
> +	void (*convert_float)(void *obj, float *dst, unsigned int dst_frames,
> +			    const float *src, unsigned int src_frames);
> +	/**
>  	 * compute the frame size for input
>  	 */
>  	snd_pcm_uframes_t (*input_frames)(void *obj, snd_pcm_uframes_t frames);
> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
> index 54a3e67..2831f23 100644
> --- a/src/pcm/pcm_rate.c
> +++ b/src/pcm/pcm_rate.c
> @@ -35,7 +35,7 @@
>  #include "iatomic.h"
>  
>  #include "plugin_ops.h"
> -
> +typedef float float_t;
>  #if 0
>  #define DEBUG_REFINE
>  #endif
> @@ -66,8 +66,8 @@ struct _snd_pcm_rate {
>  	snd_pcm_rate_ops_t ops;
>  	unsigned int get_idx;
>  	unsigned int put_idx;
> -	int16_t *src_buf;
> -	int16_t *dst_buf;
> +	void *src_buf;
> +	void *dst_buf;
>  	int start_pending; /* start is triggered but not commited to slave */
>  	snd_htimestamp_t trigger_tstamp;
>  	unsigned int plugin_version;
> @@ -310,13 +310,23 @@ static int snd_pcm_rate_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
>  		rate->sareas[chn].step = swidth;
>  	}
>  
> -	if (rate->ops.convert_s16) {
> +	if (rate->ops.convert_float) {
> +		rate->get_idx = snd_pcm_linear_get_index(rate->info.in.format, SND_PCM_FORMAT_S32);
> +		rate->put_idx = snd_pcm_linear_put_index(SND_PCM_FORMAT_S32, rate->info.out.format);
> +		free(rate->src_buf);
> +		rate->src_buf = (float_t *) malloc(channels * rate->info.in.period_size * sizeof(float_t));
> +		free(rate->dst_buf);
> +		rate->dst_buf = (float_t *) malloc(channels * rate->info.out.period_size * sizeof(float_t));
> +		if (! rate->src_buf || ! rate->dst_buf)
> +			goto error;
> +	}
> +	else if (rate->ops.convert_s16) {
>  		rate->get_idx = snd_pcm_linear_get_index(rate->info.in.format, SND_PCM_FORMAT_S16);
>  		rate->put_idx = snd_pcm_linear_put_index(SND_PCM_FORMAT_S16, rate->info.out.format);
>  		free(rate->src_buf);
> -		rate->src_buf = malloc(channels * rate->info.in.period_size * 2);
> +		rate->src_buf = (int16_t *) malloc(channels * rate->info.in.period_size * 2);
>  		free(rate->dst_buf);
> -		rate->dst_buf = malloc(channels * rate->info.out.period_size * 2);
> +		rate->dst_buf = (int16_t *) malloc(channels * rate->info.out.period_size * 2);
>  		if (! rate->src_buf || ! rate->dst_buf)
>  			goto error;
>  	}
> @@ -503,6 +513,85 @@ static void convert_from_s16(snd_pcm_rate_t *rate, const int16_t *buf,
>  		}
>  	}
>  }
> +static void convert_to_float(snd_pcm_rate_t *rate, float_t *buf,
> +			   const snd_pcm_channel_area_t *areas,
> +			   snd_pcm_uframes_t offset, unsigned int frames,
> +			   unsigned int channels)
> +{
> +#ifndef DOC_HIDDEN
> +#define GET32_LABELS
> +#include "plugin_ops.h"
> +#undef GET32_LABELS
> +#endif /* DOC_HIDDEN */
> +	void *get32 = get32_labels[rate->get_idx];
> +	const char *src;
> +	int32_t sample;
> +	const char *srcs[channels];
> +	int src_step[channels];
> +	unsigned int c;
> +
> +	for (c = 0; c < channels; c++) {
> +		srcs[c] = snd_pcm_channel_area_addr(areas + c, offset);
> +		src_step[c] = snd_pcm_channel_area_step(areas + c);
> +	}
> +
> +	while (frames--) {
> +		for (c = 0; c < channels; c++) {
> +			src = srcs[c];
> +			// converting the source format to int32 first
> +			goto *get32;
> +#ifndef DOC_HIDDEN
> +#define GET32_END after_get
> +#include "plugin_ops.h"
> +#undef GET32_END
> +#endif /* DOC_HIDDEN */
> +		after_get:
> +			// converting to float for the plugin
> +			*buf++ = (float_t) sample;
> +			srcs[c] += src_step[c];
> +		}
> +	}
> +}
> +
> +static void convert_from_float(snd_pcm_rate_t *rate, const float_t *buf,
> +			     const snd_pcm_channel_area_t *areas,
> +			     snd_pcm_uframes_t offset, unsigned int frames,
> +			     unsigned int channels)
> +{
> +#ifndef DOC_HIDDEN
> +#define PUT32_LABELS
> +#include "plugin_ops.h"
> +#undef PUT32_LABELS
> +#endif /* DOC_HIDDEN */
> +	void *put32 = put32_labels[rate->put_idx];
> +	char *dst;
> +	int32_t sample;
> +	char *dsts[channels];
> +	int dst_step[channels];
> +	unsigned int c;
> +
> +	for (c = 0; c < channels; c++) {
> +		dsts[c] = snd_pcm_channel_area_addr(areas + c, offset);
> +		dst_step[c] = snd_pcm_channel_area_step(areas + c);
> +	}
> +
> +	while (frames--) {
> +		for (c = 0; c < channels; c++) {
> +			dst = dsts[c];
> +			// plugin returned float, converting to int32 first
> +			sample = (int32_t) *buf++;
> +			// converting int32 to the destination format
> +			goto *put32;
> +#ifndef DOC_HIDDEN
> +#define PUT32_END after_put
> +#include "plugin_ops.h"
> +#undef PUT32_END
> +#endif /* DOC_HIDDEN */
> +		after_put:
> +			dsts[c] += dst_step[c];
> +		}
> +	}
> +}
>  
>  static void do_convert(const snd_pcm_channel_area_t *dst_areas,
>  		       snd_pcm_uframes_t dst_offset, unsigned int dst_frames,
> @@ -511,18 +600,36 @@ static void do_convert(const snd_pcm_channel_area_t *dst_areas,
>  		       unsigned int channels,
>  		       snd_pcm_rate_t *rate)
>  {
> -	if (rate->ops.convert_s16) {
> +	if (rate->ops.convert_float) {
> +		const float_t *src;
> +		float_t *dst;
> +		if (! rate->src_buf)
> +			src = src_areas->addr + src_offset * sizeof(float_t) * channels;
> +		else {
> +			convert_to_float(rate, rate->src_buf, src_areas, src_offset,
> +				       src_frames, channels);
> +			src = rate->src_buf;
> +		}
> +		if (! rate->dst_buf)
> +			dst = dst_areas->addr + dst_offset * sizeof(float_t) * channels;
> +		else
> +			dst = rate->dst_buf;
> +		rate->ops.convert_float(rate->obj, dst, dst_frames, src, src_frames);
> +		if (dst == rate->dst_buf)
> +			convert_from_float(rate, rate->dst_buf, dst_areas, dst_offset,
> +					 dst_frames, channels);
> +	} else if (rate->ops.convert_s16) {
>  		const int16_t *src;
>  		int16_t *dst;
>  		if (! rate->src_buf)
> -			src = src_areas->addr + src_offset * 2 * channels;
> +			src = src_areas->addr + src_offset * sizeof(int16_t) * channels;
>  		else {
>  			convert_to_s16(rate, rate->src_buf, src_areas, src_offset,
>  				       src_frames, channels);
>  			src = rate->src_buf;
>  		}
>  		if (! rate->dst_buf)
> -			dst = dst_areas->addr + dst_offset * 2 * channels;
> +			dst = dst_areas->addr + dst_offset * sizeof(int16_t) * channels;
>  		else
>  			dst = rate->dst_buf;
>  		rate->ops.convert_s16(rate->obj, dst, dst_frames, src, src_frames);
> @@ -1416,7 +1523,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
>  	}
>  #endif
>  
> -	if (! rate->ops.init || ! (rate->ops.convert || rate->ops.convert_s16) ||
> +	if (! rate->ops.init || ! (rate->ops.convert || rate->ops.convert_s16 || rate->ops.convert_float) ||
>  	    ! rate->ops.input_frames || ! rate->ops.output_frames) {
>  		SNDERR("Inproper rate plugin %s initialization", type);
>  		snd_pcm_free(pcm);
> -- 
> 1.7.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Mon, 24 Feb 2014 13:13:44 +0100,
Maarten Baert wrote:
> 
> On 24/02/14 10:17, Takashi Iwai wrote:
> > Can S32 work instead of S16?  Then we won't lose the accuracy so much.
> > Of course, handling float directly would be the best option.
> The samplerate and speexrate plugins currently take S16 (see
> pcm_src_convert_s16 in alsa-plugins/rate/rate_samplerate.c and
> alsa-plugins/rate/rate_speexrate.c), so just using S32 will not improve
> the accuracy.

Ah, I forgot it.  We should fix these plugins to allow S32 if
available, too...

> It would be easy to replace those functions with
> pcm_src_convert_float (both resampler libraries have functions that take
> float directly), but that will break the plugin ABI. Is that acceptable?
> The same would have to be done for the channel remapping (route
> conversion) plugin.

It's fine as long as the plugin is backward compatible.
That is, pcm_rate.c checks the plugin version and uses the new ops
only for objects advertising the newer version.  See pcm_extplug.c.
There are some codes checking version numbers.

> > In anyway, could you give your acked-by tag?
> Sorry, I don't know what you mean - this is the first time I've
> submitted a patch here.

Just give a line "Signed-off-by: Your Name <your@mail>" in the patch
changelog.  See Documentation/SubmittingPatches (section "sign your
work") for details.  This is a standard procedure required for
linux-kernel patch management, and we follow it for alsa-lib and
others in general.


thanks,

Takashi

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Mon, 24 Feb 2014 14:19:15 +0100,
Takashi Iwai wrote:
> 
> > It would be easy to replace those functions with
> > pcm_src_convert_float (both resampler libraries have functions that take
> > float directly), but that will break the plugin ABI. Is that acceptable?
> > The same would have to be done for the channel remapping (route
> > conversion) plugin.
> 
> It's fine as long as the plugin is backward compatible.
> That is, pcm_rate.c checks the plugin version and uses the new ops
> only for objects advertising the newer version.  See pcm_extplug.c.
> There are some codes checking version numbers.

Looking at the code again, the PCM rate plugin version checks are done
slightly differently from ext-plugin or io-plugin; it's rather done in
the plugin side.  pcm_rate.c repeats to trying to hook a plugin until
it matches by degrading the version.  In the plugin side, it provides
additional ops depending on the version it's asked. 


Takashi

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Maarten Baert]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On 24/02/14 14:37, Takashi Iwai wrote:

> Looking at the code again, the PCM rate plugin version checks are done
> slightly differently from ext-plugin or io-plugin; it's rather done in
> the plugin side.  pcm_rate.c repeats to trying to hook a plugin until
> it matches by degrading the version.  In the plugin side, it provides
> additional ops depending on the version it's asked.

That complicates things a lot. Currently the format conversion is
decided before the rate conversion is done. If not all rate conversion
plugins support float, then the format conversion plugin needs to know
this ahead of time to make the right decision. Or the plugin insertion
code needs to be changed so the plugins aren't created in a fixed order.
They are currently created back-to-front, it would probably make more
sense to choose a rate conversion and channel remapping plugin first,
and then insert format conversions where needed.

Re-submitting my original patch as requested.

Maarten Baert


[-- Attachment #2: fix-float-resampling.patch --]
[-- Type: text/x-patch, Size: 1211 bytes --]

snd_pcm_plug_change_format: Insert linear-to-float conversion when rate
or channel count is incorrect.

This fixes a bug where snd_pcm_plug_insert_plugins fails when both
client and slave use format float, but the rate or channel count does
not match. I also removed some redundant code.

Signed-off-by: Maarten Baert <maarten-baert@hotmail.com>

diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c
index fa84eaa..ede9c15 100644
--- a/src/pcm/pcm_plug.c
+++ b/src/pcm/pcm_plug.c
@@ -522,15 +522,13 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p
 		}
 #ifdef BUILD_PCM_PLUGIN_LFLOAT
 	} else if (snd_pcm_format_float(slv->format)) {
-		/* Conversion is done in another plugin */
-		if (clt->format == slv->format &&
-		    clt->rate == slv->rate &&
-		    clt->channels == slv->channels)
-			return 0;
-		cfmt = clt->format;
-		if (snd_pcm_format_linear(clt->format))
+		if (snd_pcm_format_linear(clt->format)) {
+			cfmt = clt->format;
 			f = snd_pcm_lfloat_open;
-		else
+		} else if (clt->rate != slv->rate || clt->channels != slv->channels) {
+			cfmt = SND_PCM_FORMAT_S16;
+			f = snd_pcm_lfloat_open;
+		} else
 			return -EINVAL;
 #endif
 #ifdef BUILD_PCM_NONLINEAR

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Mon, 24 Feb 2014 21:52:56 +0100,
Maarten Baert wrote:
> 
> On 24/02/14 14:37, Takashi Iwai wrote:
> 
> > Looking at the code again, the PCM rate plugin version checks are done
> > slightly differently from ext-plugin or io-plugin; it's rather done in
> > the plugin side.  pcm_rate.c repeats to trying to hook a plugin until
> > it matches by degrading the version.  In the plugin side, it provides
> > additional ops depending on the version it's asked.
> 
> That complicates things a lot. Currently the format conversion is
> decided before the rate conversion is done. If not all rate conversion
> plugins support float, then the format conversion plugin needs to know
> this ahead of time to make the right decision. Or the plugin insertion
> code needs to be changed so the plugins aren't created in a fixed order.
> They are currently created back-to-front, it would probably make more
> sense to choose a rate conversion and channel remapping plugin first,
> and then insert format conversions where needed.

Yeah, this might work better.

> Re-submitting my original patch as requested.

It's not applicable via git am.  Please resubmit with the right format
(that is cleanly applied via git am).


thanks,

Takashi

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Maarten Baert]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]

On 26/02/14 08:28, Takashi Iwai wrote:
> It's not applicable via git am. Please resubmit with the right format
> (that is cleanly applied via git am). 
Sorry, this one should work.

[-- Attachment #2: 0001-snd_pcm_plug_change_format-Insert-linear-to-float-co.patch --]
[-- Type: text/x-patch, Size: 1491 bytes --]

>From 3a576686760ec6bb76892443e3c3c9ed2657bf93 Mon Sep 17 00:00:00 2001
From: Maarten Baert <maarten-baert@hotmail.com>
Date: Wed, 26 Feb 2014 14:23:45 +0100
Subject: [PATCH] snd_pcm_plug_change_format: Insert linear-to-float conversion
 when rate or channel count is incorrect.

This fixes a bug where snd_pcm_plug_insert_plugins fails when both
client and slave use format float, but the rate or channel count does
not match. I also removed some redundant code.

Signed-off-by: Maarten Baert <maarten-baert@hotmail.com>
---
 src/pcm/pcm_plug.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c
index fa84eaa..ede9c15 100644
--- a/src/pcm/pcm_plug.c
+++ b/src/pcm/pcm_plug.c
@@ -522,15 +522,13 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p
 		}
 #ifdef BUILD_PCM_PLUGIN_LFLOAT
 	} else if (snd_pcm_format_float(slv->format)) {
-		/* Conversion is done in another plugin */
-		if (clt->format == slv->format &&
-		    clt->rate == slv->rate &&
-		    clt->channels == slv->channels)
-			return 0;
-		cfmt = clt->format;
-		if (snd_pcm_format_linear(clt->format))
+		if (snd_pcm_format_linear(clt->format)) {
+			cfmt = clt->format;
 			f = snd_pcm_lfloat_open;
-		else
+		} else if (clt->rate != slv->rate || clt->channels != slv->channels) {
+			cfmt = SND_PCM_FORMAT_S16;
+			f = snd_pcm_lfloat_open;
+		} else
 			return -EINVAL;
 #endif
 #ifdef BUILD_PCM_NONLINEAR
-- 
1.9.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Bugfix: Fix resampling when client and slave both use format float

[Takashi Iwai]

At Wed, 26 Feb 2014 14:32:51 +0100,
Maarten Baert wrote:
> 
> On 26/02/14 08:28, Takashi Iwai wrote:
> > It's not applicable via git am. Please resubmit with the right format
> > (that is cleanly applied via git am). 
> Sorry, this one should work.

Thanks, applied now.


Takashi