[alsa-devel] [alsa-lib][PATCH v3 0/4] add SNDRV_PCM_FORMAT_{S, U}20

[Takashi Sakamoto]

Hi,

On Dec 18 2017 23:48, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 15:20:59 +0100,
> Takashi Sakamoto wrote:
>>
>> Iwai-san,
>>
>> On Dec 17 2017 17:37, Takashi Sakamoto wrote:
>>> Hi,
>>>
>>> On Dec 14 2017 22:50, Maciej S. Szmigiero wrote:
>>>> This format is similar to an existing 20-bit PCM format
>>>> SNDRV_PCM_FORMAT_{S,U}20_3, however it occupies 4 bytes instead of 3.
>>>>
>>>> Changes from v1: Split the monolithic submission into separate
>>>> commits. (Note that v2 wasn't tagged as such.)
>>>>
>>>> Changes from v2: Add commas at the end of two possible last entries of
>>>> the snd_pcm_format_t enum so diffs will be more readable when new PCM
>>>> formats are added in the future, remove asserts from
>>>> snd_pcm_linear_{get,put}_index().
>>>>
>>>> Maciej S. Szmigiero (4):
>>>>     asound.h: add SNDRV_PCM_FORMAT_{S,U}20
>>>>     pcm: add and describe SND_PCM_FORMAT_{S,U}20
>>>>     pcm: linear, route: handle linear formats with 20-bit sample on 4
>>>>       bytes
>>>>     pcm: plug: add SND_PCM_FORMAT_{S,U}20 to linear_preferred_formats
>>>>
>>>>    include/pcm.h          | 20 ++++++++++++++++++--
>>>>    include/sound/asound.h |  9 +++++++++
>>>>    src/pcm/pcm.c          | 10 ++++++++++
>>>>    src/pcm/pcm_linear.c   | 14 +++++++++++---
>>>>    src/pcm/pcm_local.h    |  4 ++++
>>>>    src/pcm/pcm_misc.c     | 41 ++++++++++++++++++++++++++++++++++++++---
>>>>    src/pcm/pcm_plug.c     | 11 +++++++++++
>>>>    src/pcm/pcm_route.c    |  6 ++++--
>>>>    src/pcm/plugin_ops.h   | 50
>>>> ++++++++++++++++++++++++++++++++++++++++++++++----
>>>>    9 files changed, 151 insertions(+), 14 deletions(-)
>>>
>>> I reviewed all of these four patches.
>>>
>>> Reviewed-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>>>
>>>
>>> I found some minor issues in current implementation of linear
>>> interpolation in alsa-lib.
>>>
>>> * 'src/pcm/plugin_ops.h' includes some unused macros:
>>>    * COPY_LABELS/COPY_END
>>>    * GETU_LABELS/GETU_END
>>>    * NORMS_LABELS/NORMS_END
>>> * 'put32_labels' includes wrong comments for 18/20 bits formats.
>>> * A reorder of entries in below tables may allow us to simplify
>>>     implementation of snd_pcm_linear_get_index() and
>>>     snd_pcm_linear_put_index().
>>>    * get16_labels
>>>    * put16_labels
>>>    * get32_labels
>>>    * put32_labels
>>>
>>> Anyway, the above issues are irrelevant to your patchset. Your work
>>> can be merged to alsa-lib independently.
>>
>> His patchset is good enough to me, except for failure to spin them
>> with this cover letter (at least on my MUA). Could I ask you to deal
>> with them for current alsa-lib master? The patchset are:
>>
>> * [alsa-devel] [alsa-lib][PATCH v3 1/4] asound.h: add
>> SNDRV_PCM_FORMAT_{S, U}20
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/129371.html
>> * [alsa-devel] [alsa-lib][PATCH v3 2/4] pcm: add and describe
>> SND_PCM_FORMAT_{S, U}20
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/129372.html
>> * [alsa-devel] [alsa-lib][PATCH v3 3/4] pcm: linear, route: handle
>> linear formats with 20-bit sample on 4 bytes
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/129373.html
>> * [alsa-devel] [alsa-lib][PATCH v3 4/4] pcm: plug: add
>> SND_PCM_FORMAT_{S, U}20 to linear_preferred_formats
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/129374.html
> 
> Don't worry, there is a time zone and a week end in the peaceful
> world.  I applied the patches now.
>
> The patch 4 is still not sure whether it's the best: actually 3 bytes
> format isn't so rare, it's the standard format for USB-audio after
> all.  So, only judging from "typical" format, the precedence must be
> S24_3.  But since the conflicts between S20 and S24_3 are likely very
> rare, practically seen, it doesn't matter which format is put earlier
> in the list, so I took as is.

Yes. From the point of view, the 4th patch would need more discussions.
However, actually, the 'linear_preferred_formats' table is just used
for a below branch:

(src/pcm/pcm_plug.c)
  266         if (!snd_pcm_format_mask_test(&lin, format) &&
  267             !snd_pcm_format_mask_test(&fl, format)) {
  268                 unsigned int i;
  269                 switch (format) {
  270 #ifdef BUILD_PCM_PLUGIN_MULAW
  271                 case SND_PCM_FORMAT_MU_LAW:
  272 #endif
  273 #ifdef BUILD_PCM_PLUGIN_ALAW
  274                 case SND_PCM_FORMAT_A_LAW:
  275 #endif
  276 #ifdef BUILD_PCM_PLUGIN_ADPCM
  277                 case SND_PCM_FORMAT_IMA_ADPCM:
  278 #endif
  279                         for (i = 0; i < 
sizeof(linear_preferred_formats) / sizeof(linear_preferred_formats[0]); 
++i) {
  280                                 snd_pcm_format_t f = 
linear_preferred_formats[i];
  281                                 if 
(snd_pcm_format_mask_test(format_mask, f))
  282                                         return f;
  283                         }
  284                         /* Fall through */
  285                 default:
  286                         return SND_PCM_FORMAT_UNKNOWN;
  287                 }
  288
  289         }

Some formats, 'mulaw', 'alaw' and 'ima-adpcm', are relevant. As long as
I know, recent major devices doesn't support them. An optimistic view
is reasonable in this case, in my opinion.


Thanks
(from the peaceful world as well)

Takashi Sakamoto