-
Notifications
You must be signed in to change notification settings - Fork 349
Audio: PCM converter: Add support for A-law and mu-law #9980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4d240b7 to
42c1ed3
Compare
e834b3b to
ef38e9e
Compare
ef38e9e to
b9751ff
Compare
b9751ff to
9b0d224
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for both A-law and mu-law codecs, including new source and header files, test cases, and updates to format conversion and PCM converter functions.
- Added tests and codec implementations for mu-law and A-law encoding/decoding.
- Updated audio format conversion functions and PCM converter mappings to handle the new codec types.
- Updated IPC stream and module configuration headers to include new codec types.
Reviewed Changes
Copilot reviewed 18 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/cmocka/src/math/arithmetic/{a_law_codec, mu_law_codec}.c | New test files for A-law and mu-law codecs. |
| src/math/{a_law.c, mu_law.c} | New codec implementations following ITU-T G.711 standard. |
| src/include/sof/math/{a_law.h, mu_law.h} | Public API headers for new codecs. |
| src/include/module/{ipc4/base-config.h, ipc/stream.h} | Extended enums to include A-law and mu-law types. |
| src/include/module/audio/format.h | Added new cases for A-law and mu-law in sample byte/bit depth functions. |
| src/audio/pcm_converter/pcm_converter_generic.c | Added new PCM conversion functions for A-law and mu-law. |
| src/audio/copier/copier_host.c | Updated error handling to reflect new audio format conversion returns. |
| src/include/sof/audio/audio_stream.h | Updated format conversion function to handle new codec types. |
Files not reviewed (8)
- app/boards/intel_adsp_cavs25.conf: Language not supported
- src/audio/Kconfig: Language not supported
- src/math/CMakeLists.txt: Language not supported
- src/math/Kconfig: Language not supported
- test/cmocka/m/export_headerfile_open.m: Language not supported
- test/cmocka/src/math/arithmetic/CMakeLists.txt: Language not supported
- test/cmocka/src/math/arithmetic/a_law_mu_law_test_vectors.m: Language not supported
- tools/topology/topology2/cavs-nocodec.conf: Language not supported
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Not pressing +1 as you have the "Do not merge" commit in the series, but no blockers. Some minor notes on Kconfig and double-check on the IPC4 ABI extension.
src/math/Kconfig
Outdated
|
|
||
| config MATH_A_LAW_CODEC | ||
| bool "A-law encoder and decoder" | ||
| default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"default n" is superfluous, can be omitted https://docs.zephyrproject.org/latest/build/kconfig/tips.html#redundant-defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've fixed those all now (actually in previous push but forgot to update here).
src/math/Kconfig
Outdated
|
|
||
| config MATH_MU_LAW_CODEC | ||
| bool "mu-law encoder and decoder" | ||
| default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here.
src/audio/Kconfig
Outdated
|
|
||
| config FORMAT_A_LAW | ||
| bool "Support A-law" | ||
| default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here.
src/audio/Kconfig
Outdated
|
|
||
| config PCM_CONVERTER_FORMAT_A_LAW | ||
| bool "Support A-law" | ||
| default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more.
| SOF_IPC_FRAME_U8, | ||
| SOF_IPC_FRAME_S16_4LE /* 16-bit in 32-bit container */ | ||
| SOF_IPC_FRAME_S16_4LE, /* 16-bit in 32-bit container */ | ||
| SOF_IPC_FRAME_A_LAW, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double check, is this in sync with Intel reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an internal email thread started, should mark those as reserved there to avoid future conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now agreed with the reference.
src/audio/Kconfig
Outdated
|
|
||
| config PCM_CONVERTER_FORMAT_MU_LAW | ||
| bool "Support mu-law" | ||
| default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more.
Thanks for review, fixing those! I fixed also one missing header include seen from sparse build. I'll still push once more with kconfigs enabled to better see CI results for on/off configured. If it looks good, I'll remove the cavs25 kconfigs enable commit. Need to check later where to enable these (plus float format add) for testing. |
|
Note: Failures like this with nocodec are due to missing kernel support (thesofproject/linux#5401) while topology lists U8 as a supported format: |
src/math/a_law.c
Outdated
| uint8_t byte; | ||
|
|
||
| /* Convert to 13 bits with shift */ | ||
| sample = sample >> 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sample >>= 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
src/math/mu_law.c
Outdated
| uint8_t byte; | ||
|
|
||
| /* Convert to 14 bits with shift */ | ||
| sample = sample >> 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fixed
| uint32_t ioffset, struct audio_stream *sink, | ||
| uint32_t ooffset, uint32_t samples, uint32_t chmap) | ||
| { | ||
| uint8_t *src = audio_stream_get_rptr(source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general - source pointers probably can be const? Might also give the compiler a clue for some slight optimisations here and there. Would be good to use everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, added to mu_law and previous a_law
| ret = audio_stream_fmt_conversion(copier_cfg->base.audio_fmt.depth, | ||
| copier_cfg->base.audio_fmt.valid_bit_depth, | ||
| &in_frame_fmt, &in_valid_fmt, | ||
| copier_cfg->base.audio_fmt.s_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a notification to the host in case of such errors? We have them now, but should be rate-limited...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to do it, but I think already this is a large improvement to previous. But I added SOF_IPC_FRAME_INVALID as default for this function. Then functions those do not handle the return value (a lot of them, and also void functions themselves) will fail with formats those can't be supported.
This patch adds to math library functions sofm_a_law_encode() and sofm_a_law_decode(). The main usage is support for VoIP and 8-bit A-law encoded wav files support. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
9b0d224 to
eb684ec
Compare
|
Note, still the don't merge patch included. I'll check CI and then remove it in next push. I think I need to remove also the topology patch for nocodec and submit it later when kernel work is merged. |
eb684ec to
3e410ea
Compare
src/math/mu_law.c
Outdated
| } | ||
|
|
||
| /** | ||
| * sofm_mu_law_decode() - Decode A-law encoded code word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mu-law?
src/math/mu_law.c
Outdated
| }; | ||
|
|
||
| /** | ||
| * sofm_mu_law_encode() - Encode sample with A-law coding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mu-law I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks! It's fixed now, and there was one more in addition to these.
This patch adds to math library functions sofm_mu_law_encode() and sofm_mu_law_decode(). The main usage is support for VoIP and 8-bit mu-law encoded wav files support. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds to the PCM converter functions pcm_convert_alaw_to_s32() and pcm_convert_s32_to_alaw(). The function audio_stream_fmt_conversion() is also updated to check the sample type and depth to set up correctly conversion for A-law. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds to the PCM converter functions pcm_convert_mulaw_to_s32() and pcm_convert_s32_to_mulaw(). The header files with support functions are updated to handle the format. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch re-arranges function audio_stream_fmt_conversion() to use switch-case for IPC4 sample types handling. Errors are returned for non-enabled formats and for illegal depth and valid values. The U8 format must be unsigned type. The normal s16/s24/s32 SOF types are allowed to be MSB integer, LSB integer, or signed integer as relaxed check. Currently topology uses LSB integer as default. The handling of MSB integer that is used in some parts of topologies is done elsewhere. The SOF_IPC_FRAME_INVALID is added as second error indicator. It avoids e.g. in IPC helper function use of uninitialized frame_fmt. The function audio_stream_fmt_conversion() is called from a large number of other functions without check for return code. But the components usually check their format and without a handler for it, will block processing with unsupported format. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds the IPC4 sample type values to topology as preparation to later add U8, A-law, and mu-law support to host copier into e.g. nocodec topologies. The change requires also kernel support add for 8-bit formats. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds Octave script to generate test data for A-law and mu-law coding. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
3e410ea to
1ecabc6
Compare
As summary this adds to math library A-law and mu-law conversions. Adds to copier conversions from these to/from s32, adds checks to IPC4 to SOF types mapping, adds a nocodec topology to test these formats plus uint8_t sample type.