Audio: Fix the kconfig error of mixin_mixout. #8791
Conversation
andrula-song
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
src/audio/mixin_mixout/Kconfig
Outdated
| Applied gain is in range <1, 2 power attenuation> | ||
| Currently ARIA introduces gain transition and algorithmic | ||
| latency equal to 1 ms. | ||
| config COMP_MIXER |
There was a problem hiding this comment.
Hmm, it doesn't seem correct to have two COMP_MIXER definitions with different semantics. This works now as Zephyr and XTOS builds use different Kconfigs (and this is getting slowly fixed in #8606 ), but this seems very confusing. Maybe better to just fix the extra ARIA defintiion and leave COMP_MIXER intact.
There was a problem hiding this comment.
actually there shouldn't be any info about ARIA here, it is just an error we didn't realize before. I also don't think it would be a good choice to use 2 COMP_MIXER definitions, but don't know which solution is better.
There was a problem hiding this comment.
First, add a Fixes commit abcdef ("...") line in the commit message so we know for how long this didn't work.
I also don't think it would be a good choice to use 2 COMP_MIXER definitions,
Why do we need two different directories?
-
Either the IPC3 "mixer" and the IPC4 "mixin_mixout" are significantly different features that need a different name. Then don't add a new IPC4 COMP_MIXER. Instead a new, IPC4
COMP_MIXIN_MIXOUToption. Keep the single IPC3 COMP_MIXER. -
Or, the IPC3 "mixer" and the IPC4 "mixin_mixout" are the same feature. Then merge the two directories with a
git mv mixin_mixout/* mixer/. Single COMP_MIXER for both in the same directory. You can preserve the filenames, they're not part of the Kconfig user interface.
but don't know which solution is better.
I don't have any opinion as long as the Kconfig names are consistent with the directory names.
How was this done for other components?
BTW it's not obvious what "that" is in this sentence. I suspect this contributed to Kai's confusion.
Please use a basic grammar checker, it helps too. There seems to be one embedded in everything nowadays. So I think you meant this:
The mixer seems like a pretty important component. So what have been the effects of this error for the last 2 months? The consequences (big or small) must be described briefly in the commit message. Or even described in a Github issue if the problem is too complex for a commit message (hopefully not needed this time). |
Fixes the commit f4d0437 ("Audio: Move components related config to subfolder"). Due to a copy/paste error, the content of mixin_mixout/Kconfig has been a copy of aria/Kconfig. This patch fixes the copy/paste error, and adds IPC version dependence for COMP_MIXER and COMP_MIXIN_MIXOUT. Signed-off-by: Andrula Song <[email protected]>
andrula-song
force-pushed
the
mixinout_fix
branch
from
aceef1a to
9633be7
Compare
There was a problem hiding this comment.
Sorry I missed your force-pushed. Also, you didn't update the description which didn't help.
So aceef1a was initially was a minor Kconfig fix without a bug description.
Now there is a commit message in 9633be7 that described the Kconfig change well but:
-
Still no explanation of how this was broken for 2 months without anyone noticing. Is anyone using this C code?
-
Much more importantly, commit 9633be7 now makes a new, BIG logical change: the CMake conditionals totally new and totally different from
src/audio/CMakeLists.txt- without a single word of CMake explanation.
What is going on?
| zephyr_library_sources_ifdef(CONFIG_COMP_MIXIN_MIXOUT | ||
| ${SOF_AUDIO_PATH}/mixin_mixout/mixin_mixout.c | ||
| ${SOF_AUDIO_PATH}/mixin_mixout/mixin_mixout_generic.c | ||
| ${SOF_AUDIO_PATH}/mixin_mixout/mixin_mixout_hifi3.c |
There was a problem hiding this comment.
This makes the conditionals totally different from src/audio/CMakeLists.txt ?!?
There was a problem hiding this comment.
we use mmixin_mixout code to achieve the mixing function for IPC4 version and mixer code to achieve the mixing function for IPC3, but the infrastructure and implementation and even ability are very different for those 2 components.
Either the IPC3 "mixer" and the IPC4 "mixin_mixout" are significantly different features that need a different name. Then don't add a new IPC4 COMP_MIXER. Instead a new, IPC4 COMP_MIXIN_MIXOUT option. Keep the single IPC3 COMP_MIXER.
so I think we should better use 2 different kconfig name with the dependence of IPC version for them, and since I move the IPC version dependence into kconfig, we have to change the src/audio/CMakeLists.txt to fit the kconfig modification.
There was a problem hiding this comment.
Great explanations, thank you! They really deserve to be in the commit message.
and since I move the IPC version dependence into kconfig, we have to change the src/audio/CMakeLists.txt to fit the kconfig modification.
... and as usual in these situations, the problem is: how are you going to TEST all these CMake changes? This requires compiling in many different ways.
Always remember: break it before you fix it. It's very common to edit some of our CMake files while accidentally NOT testing the changes because they're in a different use case or configuration than the current one. If you break it and nothing happens (compilation still works fine), then you're not testing it.
There was a problem hiding this comment.
Here's another very useful test trick in C files:
#warning this volume Hifi7 file is being compiled#error works too.
There was a problem hiding this comment.
actually even with the copy/past error, we won't found this mistake because the COMP_MIXER is default yes in mixer folder, we will always compile the mixin_mixout or mixer code based on IPC version（all most all the CI DUTs have pipelines with mixin_mixout now）. I noticed this because I want to change mixin_mixout to new hifi config style for hifi5 implementation.
So once again: how come no one noticed this Kconfig mistake for 2 months? How did you notice it now? EDIT: answered above in #8791 (comment) |
|
@andrula-song @marc-hb this looks fine to me. I cant see any blockers here. |
|
Last week was all firefighting and Monday was off. I promise I'll look at this again Tuesday first thing. |
There was a problem hiding this comment.
- The code change looks good but it is incomplete: please align
src/audio/CMakeLists.txtnot to ignore your newCONFIG_COMP_*. As already requested by @kv2019i and myself 3 weeks ago, don't leavesrc/audio/CMakeLists.txtinconsistent withzephyr/CMakeLists.txt
You can easily test src/audio/CMakeLists.txt and Kconfig with commands like these (just an example)
./scripts/docker-run.sh ./scripts/xtensa-build-all.sh rmb
grep MIX build_rmb_gcc/generated/.config
./scripts/docker-run.sh make -C build_rmb_gcc/ menuconfig
-
The commit title is misleading, this is not just "fixing a kconfig error", this PR is significantly more than that! It's introducing a brand new
COMP_MIXIN_MIXOUTthat never existed before instead of relying directly on the IPC level. The error that is getting fixed does not even matter much because the fixed code was never used. So that really threw me off for a long time. -
The commit message is much better now but still too short. Just summarize the answers to the most important questions already discussed. in this PR and add that to the commit message.
-
As requested earlier, Github does not automatically sync the PR title and PR message with the commit title and PR message. It would speed reviews a bit if you could do this manually.
-
To make overbusy reviewers less confused and future reviews much faster, anticipate reviewers questions and answer them in advance in the commit message. This is the purpose of the commit message. https://wiki.openstack.org/wiki/GitCommitMessages
Anticipating reviewers' questions is especially faster when reviews happen across timezones with no possibility of interactive, real-time discussions. Ask teammates in your timezone for a quick review and answer their interactive questions in the commit message Use a metaphorical rubber duck for code reviews: https://en.wikipedia.org/wiki/Rubber_duck_debugging It's not just for debugging.
Lets fix this incrementally as the next step, it better to get this fix in rather than waiting longer. |
The original content of mixin_mixout kconfig is incorrect, that is for aria. This commit fix that error.