[alsa-devel] [alsa-lib][PATCH 3/4] pcm: linear, route: handle linear formats with 20-bit sample on 4 bytes
The previous patch has added 20-bit PCM formats SND_PCM_FORMAT_{S,U}20 to alsa-lib. We need to extend the linear format conversion code with handling of these sample formats so they can also be utilized by applications that only recognize the more typical ones like SND_PCM_FORMAT_S16.
Since the conversion arrays are indexed by a format bit width divided by 8 the easiest way to handle these formats is to treat them like they were 40-bit wide (the next free integer multiple of 8 bits). This doesn't create a collision risk with a future format since there can't really be a 40-bit sample format that occupies 4 bytes.
Make sure we use the getput conversion method for these formats since a direct conversion from / to them is not supported.
While we are at it let's also add asserts on a format bit width value in snd_pcm_linear_get_index() and snd_pcm_linear_put_index() received from snd_pcm_format_width() so we won't try to access memory outside a conversion array if for some reason this function had returned a value that is not in the expected range.
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name --- src/pcm/pcm_linear.c | 16 +++++++++++++--- src/pcm/pcm_route.c | 6 ++++-- src/pcm/plugin_ops.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c index 0d61e927323f..e4973a308004 100644 --- a/src/pcm/pcm_linear.c +++ b/src/pcm/pcm_linear.c @@ -90,6 +90,7 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f endian = 0; pwidth = snd_pcm_format_physical_width(src_format); width = snd_pcm_format_width(src_format); + assert(width >= 8 && width <= 32); if (pwidth == 24) { switch (width) { case 24: @@ -100,8 +101,11 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f default: width = 2; break; } - return width * 4 + endian * 2 + sign + 16; + return width * 4 + endian * 2 + sign + 20; } else { + if (width == 20) + width = 40; + width = width / 8 - 1; return width * 4 + endian * 2 + sign; } @@ -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: @@ -131,8 +136,11 @@ int snd_pcm_linear_put_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f default: width = 2; break; } - return width * 4 + endian * 2 + sign + 16; + return width * 4 + endian * 2 + sign + 20; } else { + if (width == 20) + width = 40; + width = width / 8 - 1; return width * 4 + endian * 2 + sign; } @@ -303,7 +311,9 @@ static int snd_pcm_linear_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params) if (err < 0) return err; linear->use_getput = (snd_pcm_format_physical_width(format) == 24 || - snd_pcm_format_physical_width(linear->sformat) == 24); + snd_pcm_format_physical_width(linear->sformat) == 24 || + snd_pcm_format_width(format) == 20 || + snd_pcm_format_width(linear->sformat) == 20); if (linear->use_getput) { if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { linear->get_idx = snd_pcm_linear_get_index(format, SND_PCM_FORMAT_S32); diff --git a/src/pcm/pcm_route.c b/src/pcm/pcm_route.c index 1f485a8e79f9..bbcc6118b593 100644 --- a/src/pcm/pcm_route.c +++ b/src/pcm/pcm_route.c @@ -565,10 +565,12 @@ static int snd_pcm_route_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params) } if (err < 0) return err; - /* 3 bytes formats? */ + /* 3 bytes or 20-bit formats? */ route->params.use_getput = (snd_pcm_format_physical_width(src_format) + 7) / 8 == 3 || - (snd_pcm_format_physical_width(dst_format) + 7) / 8 == 3; + (snd_pcm_format_physical_width(dst_format) + 7) / 8 == 3 || + snd_pcm_format_width(src_format) == 20 || + snd_pcm_format_width(dst_format) == 20; route->params.get_idx = snd_pcm_linear_get_index(src_format, SND_PCM_FORMAT_S32); route->params.put_idx = snd_pcm_linear_put_index(SND_PCM_FORMAT_S32, dst_format); route->params.conv_idx = snd_pcm_linear_convert_index(src_format, dst_format); diff --git a/src/pcm/plugin_ops.h b/src/pcm/plugin_ops.h index 5a6016adb13a..a784d9c2b320 100644 --- a/src/pcm/plugin_ops.h +++ b/src/pcm/plugin_ops.h @@ -21,6 +21,12 @@
#ifndef SX_INLINES #define SX_INLINES +static inline uint32_t sx20(uint32_t x) +{ + if(x&0x00080000) + return x|0xFFF00000; + return x&0x000FFFFF; +} static inline uint32_t sx24(uint32_t x) { if(x&0x00800000) @@ -341,7 +347,7 @@ conv_1234_123C: as_u32(dst) = as_u32c(src) ^ 0x80; goto CONV_END;
#ifdef GET16_LABELS /* src_wid src_endswap sign_toggle */ -static void *const get16_labels[4 * 2 * 2 + 4 * 3] = { +static void *const get16_labels[5 * 2 * 2 + 4 * 3] = { &&get16_1_10, /* 8h -> 16h */ &&get16_1_90, /* 8h ^> 16h */ &&get16_1_10, /* 8s -> 16h */ @@ -350,6 +356,7 @@ static void *const get16_labels[4 * 2 * 2 + 4 * 3] = { &&get16_12_92, /* 16h ^> 16h */ &&get16_12_21, /* 16s -> 16h */ &&get16_12_A1, /* 16s ^> 16h */ + /* 4 byte formats */ &&get16_0123_12, /* 24h -> 16h */ &&get16_0123_92, /* 24h ^> 16h */ &&get16_1230_32, /* 24s -> 16h */ @@ -358,6 +365,10 @@ static void *const get16_labels[4 * 2 * 2 + 4 * 3] = { &&get16_1234_92, /* 32h ^> 16h */ &&get16_1234_43, /* 32s -> 16h */ &&get16_1234_C3, /* 32s ^> 16h */ + &&get16_0123_12_20, /* 20h -> 16h */ + &&get16_0123_92_20, /* 20h ^> 16h */ + &&get16_1230_32_20, /* 20s -> 16h */ + &&get16_1230_B2_20, /* 20s ^> 16h */ /* 3bytes format */ &&get16_123_12, /* 24h -> 16h */ &&get16_123_92, /* 24h ^> 16h */ @@ -390,6 +401,10 @@ get16_1234_12: sample = as_u32c(src) >> 16; goto GET16_END; get16_1234_92: sample = (as_u32c(src) >> 16) ^ 0x8000; goto GET16_END; get16_1234_43: sample = bswap_16(as_u32c(src)); goto GET16_END; get16_1234_C3: sample = bswap_16(as_u32c(src) ^ 0x80); goto GET16_END; +get16_0123_12_20: sample = as_u32c(src) >> 4; goto GET16_END; +get16_0123_92_20: sample = (as_u32c(src) >> 4) ^ 0x8000; goto GET16_END; +get16_1230_32_20: sample = bswap_32(as_u32c(src)) >> 4; goto GET16_END; +get16_1230_B2_20: sample = (bswap_32(as_u32c(src)) >> 4) ^ 0x8000; goto GET16_END; get16_123_12: sample = _get_triple(src) >> 8; goto GET16_END; get16_123_92: sample = (_get_triple(src) >> 8) ^ 0x8000; goto GET16_END; get16_123_32: sample = _get_triple_s(src) >> 8; goto GET16_END; @@ -407,7 +422,7 @@ get16_123_B2_18: sample = (_get_triple_s(src) >> 2) ^ 0x8000; goto GET16_END;
#ifdef PUT16_LABELS /* dst_wid dst_endswap sign_toggle */ -static void *const put16_labels[4 * 2 * 2 + 4 * 3] = { +static void *const put16_labels[5 * 2 * 2 + 4 * 3] = { &&put16_12_1, /* 16h -> 8h */ &&put16_12_9, /* 16h ^> 8h */ &&put16_12_1, /* 16h -> 8s */ @@ -416,6 +431,7 @@ static void *const put16_labels[4 * 2 * 2 + 4 * 3] = { &&put16_12_92, /* 16h ^> 16h */ &&put16_12_21, /* 16h -> 16s */ &&put16_12_29, /* 16h ^> 16s */ + /* 4 byte formats */ &&put16_12_0120, /* 16h -> 24h */ &&put16_12_0920, /* 16h ^> 24h */ &&put16_12_0210, /* 16h -> 24s */ @@ -424,6 +440,10 @@ static void *const put16_labels[4 * 2 * 2 + 4 * 3] = { &&put16_12_9200, /* 16h ^> 32h */ &&put16_12_0021, /* 16h -> 32s */ &&put16_12_0029, /* 16h ^> 32s */ + &&put16_12_0120_20, /* 16h -> 20h */ + &&put16_12_0920_20, /* 16h ^> 20h */ + &&put16_12_0210_20, /* 16h -> 20s */ + &&put16_12_0290_20, /* 16h ^> 20s */ /* 3bytes format */ &&put16_12_120, /* 16h -> 24h */ &&put16_12_920, /* 16h ^> 24h */ @@ -456,6 +476,10 @@ put16_12_1200: as_u32(dst) = (uint32_t)sample << 16; goto PUT16_END; put16_12_9200: as_u32(dst) = (uint32_t)(sample ^ 0x8000) << 16; goto PUT16_END; put16_12_0021: as_u32(dst) = (uint32_t)bswap_16(sample); goto PUT16_END; put16_12_0029: as_u32(dst) = (uint32_t)bswap_16(sample) ^ 0x80; goto PUT16_END; +put16_12_0120_20: as_u32(dst) = sx20((uint32_t)sample << 4); goto PUT16_END; +put16_12_0920_20: as_u32(dst) = sx20((uint32_t)(sample ^ 0x8000) << 4); goto PUT16_END; +put16_12_0210_20: as_u32(dst) = bswap_32(sx20((uint32_t)sample << 4)); goto PUT16_END; +put16_12_0290_20: as_u32(dst) = bswap_32(sx20((uint32_t)(sample ^ 0x8000) << 4)); goto PUT16_END; put16_12_120: _put_triple(dst, (uint32_t)sample << 8); goto PUT16_END; put16_12_920: _put_triple(dst, (uint32_t)(sample ^ 0x8000) << 8); goto PUT16_END; put16_12_021: _put_triple_s(dst, (uint32_t)sample << 8); goto PUT16_END; @@ -478,7 +502,7 @@ put16_12_029_18: _put_triple_s(dst, (uint32_t)(sample ^ 0x8000) << 2); goto PUT1
#ifdef GET32_LABELS /* src_wid src_endswap sign_toggle */ -static void *const get32_labels[4 * 2 * 2 + 4 * 3] = { +static void *const get32_labels[5 * 2 * 2 + 4 * 3] = { &&get32_1_1000, /* 8h -> 32h */ &&get32_1_9000, /* 8h ^> 32h */ &&get32_1_1000, /* 8s -> 32h */ @@ -487,6 +511,7 @@ static void *const get32_labels[4 * 2 * 2 + 4 * 3] = { &&get32_12_9200, /* 16h ^> 32h */ &&get32_12_2100, /* 16s -> 32h */ &&get32_12_A100, /* 16s ^> 32h */ + /* 4 byte formats */ &&get32_0123_1230, /* 24h -> 32h */ &&get32_0123_9230, /* 24h ^> 32h */ &&get32_1230_3210, /* 24s -> 32h */ @@ -495,6 +520,10 @@ static void *const get32_labels[4 * 2 * 2 + 4 * 3] = { &&get32_1234_9234, /* 32h ^> 32h */ &&get32_1234_4321, /* 32s -> 32h */ &&get32_1234_C321, /* 32s ^> 32h */ + &&get32_0123_1230_20, /* 20h -> 32h */ + &&get32_0123_9230_20, /* 20h ^> 32h */ + &&get32_1230_3210_20, /* 20s -> 32h */ + &&get32_1230_B210_20, /* 20s ^> 32h */ /* 3bytes format */ &&get32_123_1230, /* 24h -> 32h */ &&get32_123_9230, /* 24h ^> 32h */ @@ -531,6 +560,10 @@ get32_1234_1234: sample = as_u32c(src); goto GET32_END; get32_1234_9234: sample = as_u32c(src) ^ 0x80000000; goto GET32_END; get32_1234_4321: sample = bswap_32(as_u32c(src)); goto GET32_END; get32_1234_C321: sample = bswap_32(as_u32c(src) ^ 0x80); goto GET32_END; +get32_0123_1230_20: sample = as_u32c(src) << 12; goto GET32_END; +get32_0123_9230_20: sample = (as_u32c(src) << 12) ^ 0x80000000; goto GET32_END; +get32_1230_3210_20: sample = bswap_32(as_u32c(src)) << 12; goto GET32_END; +get32_1230_B210_20: sample = (bswap_32(as_u32c(src)) << 12) ^ 0x80000000; goto GET32_END; get32_123_1230: sample = _get_triple(src) << 8; goto GET32_END; get32_123_9230: sample = (_get_triple(src) << 8) ^ 0x80000000; goto GET32_END; get32_123_3210: sample = _get_triple_s(src) << 8; goto GET32_END; @@ -553,7 +586,7 @@ __conv24_get: goto *put;
#ifdef PUT32_LABELS /* dst_wid dst_endswap sign_toggle */ -static void *const put32_labels[4 * 2 * 2 + 4 * 3] = { +static void *const put32_labels[5 * 2 * 2 + 4 * 3] = { &&put32_1234_1, /* 32h -> 8h */ &&put32_1234_9, /* 32h ^> 8h */ &&put32_1234_1, /* 32h -> 8s */ @@ -562,6 +595,7 @@ static void *const put32_labels[4 * 2 * 2 + 4 * 3] = { &&put32_1234_92, /* 32h ^> 16h */ &&put32_1234_21, /* 32h -> 16s */ &&put32_1234_29, /* 32h ^> 16s */ + /* 4 byte formats */ &&put32_1234_0123, /* 32h -> 24h */ &&put32_1234_0923, /* 32h ^> 24h */ &&put32_1234_3210, /* 32h -> 24s */ @@ -570,6 +604,10 @@ static void *const put32_labels[4 * 2 * 2 + 4 * 3] = { &&put32_1234_9234, /* 32h ^> 32h */ &&put32_1234_4321, /* 32h -> 32s */ &&put32_1234_4329, /* 32h ^> 32s */ + &&put32_1234_0123_20, /* 32h -> 20h */ + &&put32_1234_0923_20, /* 32h ^> 20h */ + &&put32_1234_3210_20, /* 32h -> 20s */ + &&put32_1234_3290_20, /* 32h ^> 20s */ /* 3bytes format */ &&put32_1234_123, /* 32h -> 24h */ &&put32_1234_923, /* 32h ^> 24h */ @@ -607,6 +645,10 @@ put32_1234_1234: as_u32(dst) = sample; goto PUT32_END; put32_1234_9234: as_u32(dst) = sample ^ 0x80000000; goto PUT32_END; put32_1234_4321: as_u32(dst) = bswap_32(sample); goto PUT32_END; put32_1234_4329: as_u32(dst) = bswap_32(sample) ^ 0x80; goto PUT32_END; +put32_1234_0123_20: as_u32(dst) = sx20(sample >> 12); goto PUT32_END; +put32_1234_0923_20: as_u32(dst) = sx20((sample ^ 0x80000000) >> 12); goto PUT32_END; +put32_1234_3210_20: as_u32(dst) = bswap_32(sx20(sample >> 12)); goto PUT32_END; +put32_1234_3290_20: as_u32(dst) = bswap_32(sx20((sample ^ 0x80000000) >> 12)); goto PUT32_END; put32_1234_123: _put_triple(dst, sample >> 8); goto PUT32_END; put32_1234_923: _put_triple(dst, (sample ^ 0x80000000) >> 8); goto PUT32_END; put32_1234_321: _put_triple_s(dst, sample >> 8); goto PUT32_END;
Hi,
On Nov 30 2017 07:17, Maciej S. Szmigiero wrote:
The previous patch has added 20-bit PCM formats SND_PCM_FORMAT_{S,U}20 to alsa-lib. We need to extend the linear format conversion code with handling of these sample formats so they can also be utilized by applications that only recognize the more typical ones like SND_PCM_FORMAT_S16.
Since the conversion arrays are indexed by a format bit width divided by 8 the easiest way to handle these formats is to treat them like they were 40-bit wide (the next free integer multiple of 8 bits). This doesn't create a collision risk with a future format since there can't really be a 40-bit sample format that occupies 4 bytes.
Make sure we use the getput conversion method for these formats since a direct conversion from / to them is not supported.
While we are at it let's also add asserts on a format bit width value in snd_pcm_linear_get_index() and snd_pcm_linear_put_index() received from snd_pcm_format_width() so we won't try to access memory outside a conversion array if for some reason this function had returned a value that is not in the expected range.
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name
src/pcm/pcm_linear.c | 16 +++++++++++++--- src/pcm/pcm_route.c | 6 ++++-- src/pcm/plugin_ops.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c index 0d61e927323f..e4973a308004 100644 --- a/src/pcm/pcm_linear.c +++ b/src/pcm/pcm_linear.c @@ -90,6 +90,7 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f endian = 0; pwidth = snd_pcm_format_physical_width(src_format); width = snd_pcm_format_width(src_format);
- assert(width >= 8 && width <= 32);
...
@@ -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.
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?
(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') 2. use '--cover-letter' option for git-format-patch so that you can write changelog of your successive work. 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.
Thanks
Takashi Sakamoto
Hi Takashi,
On 05.12.2017 06:50, Takashi Sakamoto wrote:
Hi,
On Nov 30 2017 07:17, Maciej S. Szmigiero wrote:
(..)
diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c index 0d61e927323f..e4973a308004 100644 --- a/src/pcm/pcm_linear.c +++ b/src/pcm/pcm_linear.c @@ -90,6 +90,7 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f endian = 0; pwidth = snd_pcm_format_physical_width(src_format); width = snd_pcm_format_width(src_format); + assert(width >= 8 && width <= 32);
...
@@ -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.
(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.
- 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.
Thanks
Takashi Sakamoto
Thanks, Maciej
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.
- 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.ht... http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128336.ht... http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128337.ht... http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128338.ht... [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128278.ht...
Takashi Sakamoto
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
On 05.12.2017 15:57, Takashi Iwai wrote:
On Tue, 05 Dec 2017 15:37:09 +0100, Takashi Sakamoto wrote:
Hi,
(..)
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.
The reason I added these asserts is that snd_pcm_linear_{get,put}_index() use result from snd_pcm_format_width() directly to calculate an array index. If, for any reason, this function returns an unexpected width a subtle bug will emerge.
Three reasons that I could think of why an unexpected width would be returned is a bug in a plugin that uses these functions (currently 9 plugins use them) that fail to properly restrict source / destination format space to linear formats only, a bug in general format refining code or somebody adding a format to linear formats mask without adapting conversion arrays, too.
I know that these may sound like rare occurrences but since this check is close to zero-cost (it only happens on plugin stack initialization time and not, for example, at every sample) I think it is worth the extra time (and one can always compile the library with NDEBUG to skip it).
thanks,
Takashi
Thanks, Maciej
On Tue, 05 Dec 2017 17:14:18 +0100, Maciej S. Szmigiero wrote:
On 05.12.2017 15:57, Takashi Iwai wrote:
On Tue, 05 Dec 2017 15:37:09 +0100, Takashi Sakamoto wrote:
Hi,
(..)
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.
The reason I added these asserts is that snd_pcm_linear_{get,put}_index() use result from snd_pcm_format_width() directly to calculate an array index. If, for any reason, this function returns an unexpected width a subtle bug will emerge.
Three reasons that I could think of why an unexpected width would be returned is a bug in a plugin that uses these functions (currently 9 plugins use them) that fail to properly restrict source / destination format space to linear formats only, a bug in general format refining code or somebody adding a format to linear formats mask without adapting conversion arrays, too.
I know that these may sound like rare occurrences but since this check is close to zero-cost (it only happens on plugin stack initialization time and not, for example, at every sample) I think it is worth the extra time (and one can always compile the library with NDEBUG to skip it).
So it has nothing to do with the 20bit PCM support itself. Please put in another patch with more contexts.
thanks,
Takashi
participants (3)
-
Maciej S. Szmigiero -
Takashi Iwai -
Takashi Sakamoto