[alsa-devel] [PATCH] Bugfix: Fix resampling when client and slave both use format float
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
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
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
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.
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);- /**
*/ snd_pcm_uframes_t (*input_frames)(void *obj, snd_pcm_uframes_t frames);
- compute the frame size for input
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);
rate->dst_buf = malloc(channels * rate->info.out.period_size * 2);
rate->dst_buf = (int16_t *) malloc(channels * rate->info.out.period_size * 2);@@ -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 firstgoto *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 firstsample = (int32_t) *buf++;// converting int32 to the destination formatgoto *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;elsedst = 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;
dst = dst_areas->addr + dst_offset * 2 * channels;
dst = dst_areas->addr + dst_offset * sizeof(int16_t) * channels;@@ -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
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
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
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
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
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
participants (3)
-
Maarten Baert -
Pavel Hofman -
Takashi Iwai