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

[Takashi Sakamoto]

Hi,

On Dec 5 2017 22:22, Maciej S. Szmigiero wrote:
>>>> @@ -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.

Your have correct understanding of this issue. However, it's better for
us to achieve your aim (addition of the new pcm formats) at first. Then
we start discussion about better solution to handle the non-Linear
formats.

In my opinion, addition of constraint of PCM hardware parameters to
these PCM plugins is preferable to insertion of the assertions, because 
abort inner library is bad idea itself. Proper errors should be returned
to applications in protocol via alsa-lib PCM APIs. It's more friendly to
usual developers and users.

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

This patchset[1] should have v2, because I did comment to your first
version[2]. So the next version should have v3.

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

OK. Thanks.

[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128335.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128336.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128337.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128338.html
[2] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128278.html

Takashi Sakamoto