src_lite: fix 32kHz to 16kHz conversion issue #9348
Conversation
Fix issue where 32kHz to 16kHz conversion would give incorrect frequency signal. src_polyphase_init function was using wrong coefficient matrix while checking for equal frequencies resulting in no conversion. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
src/audio/src/src.c
Outdated
| n_stages = (src->stage2->filter_length == 1) ? 1 : 2; | ||
| if (src_in_fs[p->idx_in] == src_out_fs[p->idx_out]) | ||
| if (p->in_fs[p->idx_in] == p->out_fs[p->idx_out]) |
There was a problem hiding this comment.
This might be an issue in setup tables for src_lite since there some differences to normal. As far as I know they were hand-edited from some old version of normal SRC, while tables for normal SRC are all script generated. There's multiple ways to check for 1:1 conversion and this probably works for both src and src_lite. I'll test with normal SRC in testbench if this is OK, so it could be approved. I doubt anyone wants to do another hand edit round for src-lite. So please wait until I can check this. There's is no SOF CI coverage for this.
There was a problem hiding this comment.
To be more specific. There should be a script like this for src-lite to be able to re-generate them if we would change anything into core functions: https://github.com/thesofproject/sof/blob/main/src/audio/src/tune/sof_src_ipc4_int32.m . Remove the rates combinations not needed and run the generator in Matlab or Octave.
Could you @tobonex please create a similar script and see if it would fix this issue? Without such script existing maintenance of src-lite is hard.
There was a problem hiding this comment.
also to remind, that src_lite was actually unused until 7ee322f
There was a problem hiding this comment.
@tobonex Maybe not familiar with Matlab? If you can make e.g. with spreadsheet a matrix of what in/our rates should be supported by src_lite, I can generate the new coefficients for you to test. I do see that possible in rates are 32, 44.1, and 48 kHz and possible out rates are 16 and 48 kHz. But are all in-out combinations supported?
There was a problem hiding this comment.
@singalsu I'm familliar with matlab. Not so familliar with SRC though , so I feel like I'm lacking some context.
The thing I want to change here is still an issue I think : using hardcoded src table even when src_lite is used. Still, got octave running, I can try to create the script and check.
There was a problem hiding this comment.
@singalsu Added the generator for src_lite, and regenerated coef files. This by itself does not fix the issue, so the change in the if statement is still needed. However I'm guessing we still want the new files anyway so I added new commits.
| const int32_t fir_one = 1073741824; | ||
| const struct src_stage src_int32_1_1_0_0 = { 0, 0, 1, 1, 1, 1, 1, 0, -1, &fir_one }; | ||
| const struct src_stage src_int32_0_0_0_0 = { 0, 0, 0, 0, 0, 0, 0, 0, 0, &fir_one }; | ||
| const int src_in_fs[3] = { 32000, 44100, 48000}; | ||
| const int src_out_fs[2] = { 16000, 48000}; | ||
| const struct src_stage * const src_table1[2][3] = { | ||
| { &src_int32_1_2_4535_5000, &src_int32_10_21_3455_5000, | ||
| &src_int32_1_3_4535_5000}, | ||
| { &src_int32_3_2_4535_5000, | ||
| &src_int32_8_7_4535_5000, &src_int32_1_1_0_0 | ||
| } | ||
| }; | ||
|
|
||
| const struct src_stage * const src_table2[2][3] = { | ||
| { &src_int32_1_1_0_0, &src_int32_16_21_4535_5000, | ||
| &src_int32_1_1_0_0}, | ||
| { &src_int32_1_1_0_0, | ||
| &src_int32_20_21_4167_5000, &src_int32_1_1_0_0 | ||
| } | ||
| }; |
There was a problem hiding this comment.
Should these be in SRC lite C flle ?
tobonex
force-pushed
the
src_lite_conversion_fix
branch
from
81a47f3
to
c32fdf7
Compare
tobonex
force-pushed
the
src_lite_conversion_fix
branch
from
c32fdf7
to
3d608c9
Compare
|
What are the targets for SRC-lite? Should SRC and SRC-lite be mutually exclusive in build? If yes, then there should be C pre-processor macros to error such Kconfig. If like it seems with TGL that both are enabled there is a conflict with global configuration tables. The SRC and SRC-lite sources should be better separated to avoid random "win" of the configuration tables with same name to happen. Currently the conversions may use totally wrong filters coefficients. At least it's demonstrated that in and out rates table is corrupt. SRC is breaking SRC-lite, and I'm pretty sure the opposite may have also happened. |
Add a coef generator for src_lite. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Add new src_lite coef files generated by the new .m generator. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Renaming src.c, as this filename will have a different purpose. File src_common.c will contain common functions for all src types. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
tobonex
force-pushed
the
src_lite_conversion_fix
branch
2 times, most recently
from
7d05073
to
fefb68d
Compare
Defines for normal src and src_lite were not separated correctly. Added new variables so those values can be different depending on the type of src being used. Those are assigned in prepare function in a new src.c file, similarly to src_lite.c Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
tobonex
force-pushed
the
src_lite_conversion_fix
branch
from
fefb68d
to
ac220c4
Compare
Fix issue where 32kHz to 16kHz conversion would give incorrect frequency signal. src_polyphase_init function was using wrong coefficient matrix while checking for equal frequencies resulting in no conversion.
In more detail:
Conversion matrices are saved in src_param struct in src_prepare function. But src_polyphase_init used hard-coded matrix for normal src (src_lite uses a different one). src_polyphase_init used incorrect matrix with indices for correct matrix (idx_in = 0, idx_out = 0), and those in the normal src matrix translate to frequencies 8k and 8k (instead of 32k and 16k), and so the if statement checking for equal frequencies was incorrectly true.
Regression started at 7ee322f