Repository navigation
audio: selector: pcm conversion error 16in16bit to 24in32bit - quality issues in audio - #11370
Open
piotrhoppeintel wants to merge 1 commit into
Open
piotrhoppeintel wants to merge 1 commit into
piotrhoppeintel wants to merge 1 commit into
Conversation
piotrhoppeintel
changed the title
softwarecki
reviewed
Oct 28, 2026
| * \param[in] src_channels Number of source channels. | ||
| * \param[in] coeffs_config IPC4 micsel config with Q10 coefficients. | ||
| */ | ||
| static void process_frame_s16le_to_s24le(int32_t dst[], int dst_channels, |
Collaborator
There was a problem hiding this comment.
Use unsigned int for the channels count
softwarecki
reviewed
Oct 28, 2026
| struct comp_data *cd = module_get_private_data(mod); | ||
| const unsigned int src_channels = cd->config.in_channels_count; | ||
| const unsigned int dst_channels = cd->config.out_channels_count; | ||
| const unsigned int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)src_channels); |
Contributor
Author
There was a problem hiding this comment.
The type conversion has been removed
softwarecki
reviewed
Oct 28, 2026
| * \param[in] src_channels Number of source channels. | ||
| * \param[in] coeffs_config IPC4 micsel config with Q10 coefficients. | ||
| */ | ||
| static void process_frame_s16le_to_s24le(int32_t dst[], int dst_channels, |
softwarecki
reviewed
Oct 28, 2026
| * \param[in] coeffs_config IPC4 micsel config with Q10 coefficients. | ||
| */ | ||
| static void process_frame_s16le_to_s24le(int32_t dst[], int dst_channels, | ||
| const int16_t src[], int src_channels, |
softwarecki
reviewed
Oct 28, 2026
| * \param[in] src_channels Number of source channels. | ||
| * \param[in] coeffs_config IPC4 micsel config with Q10 coefficients. | ||
| */ | ||
| static void process_frame_s24le_to_s16le(int16_t dst[], int dst_channels, |
Collaborator
There was a problem hiding this comment.
Same as for process_frame_s16le_to_s24le
softwarecki
reviewed
Oct 28, 2026
| struct comp_data *cd = module_get_private_data(mod); | ||
| const unsigned int src_channels = cd->config.in_channels_count; | ||
| const unsigned int dst_channels = cd->config.out_channels_count; | ||
| const unsigned int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)src_channels); |
softwarecki
reviewed
Oct 28, 2026
softwarecki
left a comment
softwarecki
left a comment
Collaborator
There was a problem hiding this comment.
Some comments below
Add bidirectional sample format conversion between S16_LE and S24_4LE. Track valid sample formats, select processing functions accordingly, and disable passthrough when source and sink formats differ.
piotrhoppeintel
force-pushed
the
error/ptl/18042325599-pcm-conversion
branch
from
October 29, 2026 09:45
8f1d833 to
6c9515f
Compare
piotrhoppeintel
marked this pull request as ready for review
October 29, 2026 09:48
piotrhoppeintel
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
tlissows
as code owners
October 29, 2026 09:48
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Active findings identify narrowing before saturation that can invert output polarity, a valid-format overwrite that can make S32 EQIIR preparation fail, and missing tests for both new conversion paths.
3 open issues
What changed in this PR
This PR addresses audio-quality issues in the selector when input and output sample formats differ, adding bidirectional S16_LE ↔ S24_4LE conversion.
Changes:
- Track container and valid sample formats for processing-function selection.
- Add conversion routines and disable passthrough when formats differ.
- Update stream parameters to reflect input/output formats and output channel counts.
| File | Description |
|---|---|
src/include/sof/audio/selector.h |
Add IPC4 format fields to selector state and dispatch entries. |
src/audio/selector/selector.c |
Track valid formats and update parameter and passthrough handling. |
src/audio/selector/selector_generic.c |
Add conversion routines and format-aware dispatch. |
Review effort: Balanced
|
|
||
| /* get source data format and period bytes */ | ||
| cd->source_format = audio_stream_get_frm_fmt(&sourceb->stream); | ||
| cd->source_valid_format = audio_stream_get_valid_fmt(&sourceb->stream); |
| accum += (int32_t)src[j] * (int32_t)coeffs_config->coeffs[i][j]; | ||
|
|
||
| /* Convert Q1.15 * Q6.10 (Q7.25) to Q1.23. */ | ||
| dst[i] = sat_int24((accum + (1 << 1)) >> 2); |
Comment on lines
+582
to
+585
| {SOF_IPC_FRAME_S16_LE, SOF_IPC_FRAME_S32_LE, | ||
| SOF_IPC_FRAME_S16_LE, SOF_IPC_FRAME_S24_4LE, 0, sel_s16le_to_s24le}, | ||
| {SOF_IPC_FRAME_S32_LE, SOF_IPC_FRAME_S16_LE, | ||
| SOF_IPC_FRAME_S24_4LE, SOF_IPC_FRAME_S16_LE, 0, sel_s24le_to_s16le}, |
tmleman
reviewed
Oct 29, 2026
tmleman
left a comment
tmleman
left a comment
Contributor
There was a problem hiding this comment.
The change looks like two separate fixes combined into one. Can this be split? Fixing the incorrect format and adding the missing conversion.
PR 11370: test resultsRun date: 2026-10-29 14:20 UTC Tested commit: 6c9515f4d4e435c92cbf8afc11532d58064ea7f0
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During testing, we encountered an issue when the input and output formats were different. In such cases, the processing function was selected incorrectly because the selection was based on the input format without taking the output format into account.
For example, this occurred for the conversion path S16_LE -> S24_4LE, where a 16-bit format was used with a 32-bit container.
For this reason we add bidirectional sample format conversion between S16_LE and S24_4LE. Track valid sample formats, select processing functions accordingly, and disable passthrough when source and sink formats differ.