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

[Maciej S. Szmigiero]

Hi Takashi,

On 05.12.2017 06:50, Takashi Sakamoto wrote:
> Hi,
> 
> On Nov 30 2017 07:17, Maciej S. Szmigiero wrote:
(..)
>> 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.

snd_pcm_linear_get_index() and snd_pcm_linear_put_index() should
be called for linear formats only (like their names suggest).

If they were called for 'SNDRV_PCM_FORMAT_IMA_ADPCM' this will result
in a negative conversion arrays index.
The same goes for formats that return '-EINVAL' as their width.

This will mean that an application will try to access these arrays
outside of their bounds, which may produce a hard to find bugs.
I think it is really better to fail with an assertion failure in such
cases.

> 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?

If these assertions are really controversial I will remove it, but please
think again about it in the light of the paragraph above.
Asserts can also be split out to a separate patch.

> (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')

Hmm, but this series already had this prefix ('[alsa-lib][PATCH]')?
Had no version number, since it was the first iteration.

> 2. use '--cover-letter' option for git-format-patch so that you can
>   write changelog of your successive work.

Will do.

> 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.
> 

Will do.

Also, will add commas at the ends of last entries in enums, as you had
suggested in your other message.

> Thanks
> 
> Takashi Sakamoto

Thanks,
Maciej