-
Notifications
You must be signed in to change notification settings - Fork 314
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
src_lite: fix 32kHz to 16kHz conversion issue #9348
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also to remind, that src_lite was actually unused until 7ee322f
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be in SRC lite C flle ?
81a47f3
to
c32fdf7
Compare
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>
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>
fefb68d
to
ac220c4
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.
Thanks @tobonex ! This version works well for real device (tested with sof-hda-benchmark-src32.tplg and sof-hda-benchmark-src_lite32.tplg) and src_test.m with testbench.
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