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

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

[Maciej S. Szmigiero]

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

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

[Takashi Sakamoto]

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


Thanks

Takashi Sakamoto

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

[Maciej S. Szmigiero]

Hi,

On 17.12.2017 09: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@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.


Thanks.

> Thanks
> 
> Takashi Sakamoto


Best regards,
Maciej Szmigiero
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Sakamoto]

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


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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@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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel