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

Commit Message

Maarten Baert Feb. 21, 2014, 8:01 p.m. UTC

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

Comments

Takashi Iwai Feb. 24, 2014, 9:17 a.m. UTC | #1

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
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Maarten Baert Feb. 24, 2014, 12:13 p.m. UTC | #2

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

Takashi Iwai Feb. 24, 2014, 1:19 p.m. UTC | #3

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

Takashi Iwai Feb. 24, 2014, 1:37 p.m. UTC | #4

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

Patch

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