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.ht... * [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.ht... * [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.ht... * [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.ht...

Thanks

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

• [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.ht...

• [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.ht...

• [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.ht...

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