[alsa-devel] alsa-lib: Rate conversion broken for non-S16?

[Mark Hills]

On Wed, 11 Aug 2010, Takashi Iwai wrote:

> At Wed, 11 Aug 2010 10:59:59 +0200,
> I wrote:
> > 
> > At Wed, 11 Aug 2010 09:04:32 +0100 (BST),
> > Mark Hills wrote:
> > > 
> > > Hi, does anybody have any thoughts on my email below? The rate conversion 
> > > code looks stable and untouched for a while.
> > 
> > I guess it's a missing support of 3-byte formats in 16bit conversion,
> > so not specific to rate plugin.

Indeed, it's an obvious ommision now that you mention it! Thanks for the 
hint.
 
> > Adding the support shouldn't be too hard, since it's already found
> > in 32bit conversion.
> 
> A quick (untested) fix is below.

Thanks, I tested your patch.

There's a minor mistake that prevents it working, and a couple of comments 
don't seem quite right (see below). I'll follow this email with a revised 
patch.

With my modified patch, I was able to play back reliably in both aplay and 
xwax on my S24_3BE device with rate conversion.

> diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c
> index 12e2e7f..01b6742 100644
> --- a/src/pcm/pcm_linear.c
> +++ b/src/pcm/pcm_linear.c
> @@ -114,10 +114,9 @@ int snd_pcm_linear_get32_index(snd_pcm_format_t src_format, snd_pcm_format_t dst
>  
>  int snd_pcm_linear_put_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_format)
>  {
> -	int sign, width, endian;
> +	int sign, width, pwidth, endian;
>  	sign = (snd_pcm_format_signed(src_format) != 
>  		snd_pcm_format_signed(dst_format));
> -	width = snd_pcm_format_width(dst_format) / 8 - 1;
>  #ifdef SND_LITTLE_ENDIAN
>  	endian = snd_pcm_format_big_endian(dst_format);
>  #else
> @@ -125,7 +124,23 @@ int snd_pcm_linear_put_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f
>  #endif
>  	if (endian < 0)
>  		endian = 0;
> -	return width * 4 + endian * 2 + sign;
> +	pwidth = snd_pcm_format_physical_width(dst_format);
> +	width = snd_pcm_format_width(dst_format) / 8 - 1;

I concluded this conversion is mis-placed, as it is done again, below (and 
affects decision making when pwidth == 24)

> +	if (pwidth == 24) {
> +		switch (width) {
> +		case 24:
> +			width = 0; break;
> +		case 20:
> +			width = 1; break;
> +		case 18:
> +		default:
> +			width = 2; break;
> +		}
> +		return width * 4 + endian * 2 + sign + 16;
> +	} else {
> +		width = width / 8 - 1;

I left this conversion in.

> +		return width * 4 + endian * 2 + sign;
> +	}
>  }
>  
>  int snd_pcm_linear_put32_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_format)
> diff --git a/src/pcm/plugin_ops.h b/src/pcm/plugin_ops.h
> index 04220d6..0c63ca6 100644
> --- a/src/pcm/plugin_ops.h
> +++ b/src/pcm/plugin_ops.h
> @@ -407,7 +407,7 @@ get16_123_B2_18: sample = (_get_triple_s(src) >> 2) ^ 0x8000; goto GET16_END;
>  
>  #ifdef PUT16_LABELS
>  /* dst_wid dst_endswap sign_toggle */
> -static void *const put16_labels[4 * 2 * 2] = {
> +static void *const put16_labels[4 * 2 * 2 + 4 * 3] = {
>  	&&put16_12_1,		 /* 16h ->  8h */
>  	&&put16_12_9,		 /* 16h ^>  8h */
>  	&&put16_12_1,		 /* 16h ->  8s */
> @@ -424,6 +424,19 @@ static void *const put16_labels[4 * 2 * 2] = {
>  	&&put16_12_9200,	 /* 16h ^> 32h */
>  	&&put16_12_0021,	 /* 16h -> 32s */
>  	&&put16_12_0029,	 /* 16h ^> 32s */
> +	/* 3bytes format */
> +	&&put16_12_120,	 /* 16h -> 24h */
> +	&&put16_12_920,	 /* 16h ^> 24h */
> +	&&put16_12_021,	 /* 16h -> 24s */
> +	&&put16_12_029,	 /* 16h ^> 24h */
> +	&&put16_12_120_20,	 /* 16h -> 20h */
> +	&&put16_12_920_20,	 /* 16h ^> 20h */
> +	&&put16_12_021_20,	 /* 16h -> 20s */
> +	&&put16_12_029_20,	 /* 16h ^> 20h */
> +	&&put16_12_120_18,	 /* 16h -> 18h */
> +	&&put16_12_920_18,	 /* 16h ^> 18h */
> +	&&put16_12_021_18,	 /* 16h -> 18s */
> +	&&put16_12_029_18,	 /* 16h ^> 18h */

A couple of the comments here look wrong, as they are repeats. '20h' and 
'18h' become '20s' and '18s' respectively. I have corrected that in the 
follow up patch.

>  };
>  #endif
>
[...]

-- 
Mark