[alsa-devel] [alsa-lib][PATCH 3/4] pcm: linear, route: handle linear formats with 20-bit sample on 4 bytes

[Takashi Sakamoto]

Hi,

On Nov 30 2017 07:17, Maciej S. Szmigiero wrote:
> The previous patch has added 20-bit PCM formats SND_PCM_FORMAT_{S,U}20 to
> alsa-lib.
> We need to extend the linear format conversion code with handling of these
> sample formats so they can also be utilized by applications that only
> recognize the more typical ones like SND_PCM_FORMAT_S16.
> 
> Since the conversion arrays are indexed by a format bit width divided by 8
> the easiest way to handle these formats is to treat them like they were
> 40-bit wide (the next free integer multiple of 8 bits).
> This doesn't create a collision risk with a future format since there can't
> really be a 40-bit sample format that occupies 4 bytes.
> 
> Make sure we use the getput conversion method for these formats since a
> direct conversion from / to them is not supported.
> 
> While we are at it let's also add asserts on a format bit width value
> in snd_pcm_linear_get_index() and snd_pcm_linear_put_index() received from
> snd_pcm_format_width() so we won't try to access memory outside a
> conversion array if for some reason this function had returned a value that
> is not in the expected range.
> 
> Signed-off-by: Maciej S. Szmigiero <mail at maciej.szmigiero.name>
> ---
>   src/pcm/pcm_linear.c | 16 +++++++++++++---
>   src/pcm/pcm_route.c  |  6 ++++--
>   src/pcm/plugin_ops.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
>   3 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c
> index 0d61e927323f..e4973a308004 100644
> --- a/src/pcm/pcm_linear.c
> +++ b/src/pcm/pcm_linear.c
> @@ -90,6 +90,7 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f
>   		endian = 0;
>   	pwidth = snd_pcm_format_physical_width(src_format);
>   	width = snd_pcm_format_width(src_format);
> +	assert(width >= 8 && width <= 32);
> 
> ...
> > @@ -121,6 +125,7 @@ int snd_pcm_linear_put_index(snd_pcm_format_t 
src_format, snd_pcm_format_t dst_f
>   		endian = 0;
>   	pwidth = snd_pcm_format_physical_width(dst_format);
>   	width = snd_pcm_format_width(dst_format);
> +	assert(width >= 8 && width <= 32);
>   	if (pwidth == 24) {
>   		switch (width) {
>   		case 24:

I can understand your intention to insert these assert(), while it's 
better not to include them in this series of patch.

A call of 'snd_pcm_format_physical_width()' and 'snd_pcm_format_width()'
returns 4 for 'SNDRV_PCM_FORMAT_IMA_ADPCM'. Furthermore, both of them
returns -EINVAL for 'SNDRV_PCM_FORMAT_MPEG', 'SNDRV_PCM_FORMAT_GSM' and
'SNDRV_PCM_FORMAT_SPECIAL'. They can bring wrong calculation to the
resampling function, in the worse case result in any fault.

However, this is another issue, at least out of your aim for this
patchset. It's a kind of bug for wider influence. I think to have
another opportunity to discuss about the way to handle such special
formats in the resampling layer. Would you please post this patchset
again without these assertions?

(Additionally, I suspect that current implementation of the resampling
layer itself is not enough proper for non-PCM samples such as float, DSD
and so on.)

And next time, please follow a below instruction.
1.use '--subject-prefix' option with 'alsa-lib][PATCH v3' for
   git-format-patch so that subject of your patches includes better
   information for reviewers.
   (If it's for kernel stuffs, just use '--v')
2. use '--cover-letter' option for git-format-patch so that you can
   write changelog of your successive work.
3.use '--thread' option for git-send-email (or add 'sendemail.thread'
   entry into your local repository) so that posted patches spin in one
   message thread.


Thanks

Takashi Sakamoto