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

[Takashi Iwai]

On Tue, 05 Dec 2017 15:37:09 +0100,
Takashi Sakamoto wrote:
> 
> 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.

Well, it's basically moot to discuss about it, as
snd_pcm_lib_linear_get_index() is an internal function, i.e. it's not
supposed to be called from applications.

In that sense, assert() wouldn't be too bad.   However, the primary
question is -- would it really catch anything in practice?  What drove
you to put that assert() at all?

If a check of the valid formats, then calling snd_pcm_format_linear()
is the safest.


thanks,

Takashi