audio: crossover: crossover code refactor #8587

btian1 · 2023-12-07T05:49:59Z

This is a clean up, purpose is declutter headers, toml files, Readme.md etc per module basis, since today everything is scattered in current code base.

lyakh · 2023-12-07T09:42:19Z

src/audio/multiband_drc/user/multiband_drc.h

 #include <user/drc.h>
 #include <user/eq.h>

+#include "../../crossover/crossover_user.h"


not sure how strongly crossover and multiband DRC are related, but maybe it would be better to extract required common parts and keep it in headers under src/include/sof/audio?

only use one definition: SOF_CROSSOVER_MAX_STREAMS in line 18 and 21

we don't want to have new file headers in include path, any other good ways?
if I did not use user folder for multiband DRC, then only one layer ../

for #include <user/drc.h>

it also need referenced by: ../../drc/drc_user.h, the structure is used from drc_user.h, combined with above, if put common to one header, seems also strange, image a header with crossover definition and drc structure definition, we may consult Seppo once he is back.

src/audio/crossover/crossover.c

src/audio/crossover/CMakeLists.txt

src/audio/crossover/crossover_ipc3.c

src/audio/crossover/crossover_ipc4.c

lyakh · 2023-12-07T09:48:40Z

src/audio/crossover/crossover_ipc4.c

+
+#include "crossover_algorithm.h"
+#include "crossover_user.h"
+#include "crossover.h"


looks like too many headers. Just to expand a bit: getting headers perfectly isn't always simple, and probably we don't expect that. But please try to have them mostly right - include all the headers that are needed in this file and avoid any headers, that aren't needed here

marc-hb · 2023-12-08T08:03:13Z

Never a dull moment... this PR confuses the disassembler. For the first time since forever, it introduces a disassembly difference between Linux and Windows - with the same toolchain of course.

Note I have seen other issues before with parent #include "../...", this looks suspicious. EDIT: ed7c57b

https://github.com/thesofproject/sof/actions/runs/7129022432/job/19412552108?pr=8587

--- lin/build-sof-staging/sof-info/mtl/zephyr.lst	2023-12-07 13:46:40
+++ win/build-sof-staging/sof-info/mtl/zephyr.lst	2023-12-07 13:49:28
@@ -138437,58 +138437,47 @@
 	return 0;
 a006c11c:	020c                          	movi.n	a2, 0
 }
 a006c11e:	f01d                          	retw.n
 
 a006c120 <multiband_drc_reset_state>:
 {
 a006c120:	004136                      	entry	a1, 32
 	for (i = 0; i < PLATFORM_MAX_CHANNELS; i++)
 a006c123:	88a052                      	movi	a5, 136
 a006c126:	328b                          	addi.n	a3, a2, 8
 a006c128:	525a                          	add.n	a5, a2, a5
-static inline void crossover_reset_state_lr4(struct iir_state_df2t *lr4)
-{
-	rfree(lr4->coef);
-	rfree(lr4->delay);
-
 	lr4->coef = NULL;
 a006c12a:	040c                          	movi.n	a4, 0
 	rfree(lr4->coef);
 a006c12c:	0023a2                      	l32i	a10, a3, 0
 a006c12f:	e1a425                      	call8	a004db70 <rfree>
 	rfree(lr4->delay);
 a006c132:	0123a2                      	l32i	a10, a3, 4
 a006c135:	e1a3a5                      	call8	a004db70 <rfree>
 	lr4->coef = NULL;
 a006c138:	0349                          	s32i.n	a4, a3, 0
 	lr4->delay = NULL;
 a006c13a:	1349                          	s32i.n	a4, a3, 4
 a006c13c:	10c332                      	addi	a3, a3, 16
 a006c13f:	e99357                      	bne	a3, a5, a006c12c <multiband_drc_reset_state+0xc>
 a006c142:	30a362                      	movi	a6, 0x330
 a006c145:	30c252                      	addi	a5, a2, 48
 a006c148:	626a                          	add.n	a6, a2, a6
 	lr4->coef = NULL;
 a006c14a:	040c                          	movi.n	a4, 0
 a006c14c:	000c06                      	j	a006c180 <multiband_drc_reset_state+0x60>
 a006c14f:	00                               	.byte 00
 	rfree(lr4->coef);
 a006c150:	2223a2                      	l32i	a10, a3, 136
- */
-static inline void crossover_reset_state_ch(struct crossover_state *ch_state)
-{
-	int i;
-
-	for (i = 0; i < CROSSOVER_MAX_LR4; i++) {
 a006c153:	10c332                      	addi	a3, a3, 16
 a006c156:	e1a1a5                      	call8	a004db70 <rfree>
 	rfree(lr4->delay);
 a006c159:	1f23a2                      	l32i	a10, a3, 124
 a006c15c:	e1a125                      	call8	a004db70 <rfree>
 	rfree(lr4->coef);
 a006c15f:	2a23a2                      	l32i	a10, a3, 168
 	lr4->coef = NULL;
 a006c162:	1e6342                      	s32i	a4, a3, 120
 	lr4->delay = NULL;
 a006c165:	1f6342                      	s32i	a4, a3, 124
 	rfree(lr4->coef);

wc -l ?in/build-sof-staging/sof-info/mtl/zephyr.lst
  179630 lin/build-sof-staging/sof-info/mtl/zephyr.lst
  179619 win/build-sof-staging/sof-info/mtl/zephyr.lst

marc-hb · 2023-12-08T09:24:02Z

src/audio/multiband_drc/multiband_drc.h

 #include <sof/audio/drc/drc.h>
 #include <sof/math/iir_df2t.h>
 #include <sof/audio/component.h>
 #include <sof/audio/data_blob.h>
 #include <sof/platform.h>
 #include <stdint.h>
 #include "user/multiband_drc.h"
+#include "../crossover/crossover.h"


Don't use .. in #include, no one does that. Keep .h files in -I locations.

@marc-hb , how about add one more path to zephyr/CMakeList with below diff:

--- a/zephyr/CMakeLists.txt +++ b/zephyr/CMakeLists.txt @@ -90,6 +90,7 @@ ExternalProject_Add(sof_logger_ep # default SOF includes target_include_directories(SOF INTERFACE ${RIMAGE_TOP}/src/include) target_include_directories(SOF INTERFACE ${SOF_SRC_PATH}/include) +target_include_directories(SOF INTERFACE ${SOF_SRC_PATH}/audio) target_include_directories(SOF INTERFACE ${SOF_SRC_PATH}/arch/${ARCH}/include) target_include_directories(SOF INTERFACE ${sof_top_dir}/third_party/include)

then all these can be changed with <crossover/crossover.h> ?

@marc-hb we do! I think ".." is better in this case to make it very clear this module is reusing definitions of a peer module (see my comments to #8593 ).

how about add one more path to zephyr/CMakeList with below diff:

(I edited your diff, edit it again to see how)

Yes please, something like that but probably with some CMakeLists.txt:if(CONFIG_...). It would be interesting to check whether that solves the disassembly difference between Linux and Windows. That can be done with test draft PR (no reviewers / no github spam)

No no such disassembly difference in https://github.com/thesofproject/sof/actions/runs/7137933378/job/19438902490?pr=8593 even though it has one #include "../"

to make it very clear this module is reusing definitions of a peer module

There is no requirement for all target_include_directories() to be in the top-level makefile.

@marc-hb @btian1 @singalsu To me, adding a non-include folder into target_include_directories is opening a can of worms. Then the build is searching for headers in the implementation parts of the code base and this can lead to surprises. I personally prefer the explicit "../../foo.h" includes, to make it very explicit something unusual is happening here, and only specific foo.h is included. This really only affects modules that are directly reusing portions of other peer modules.

Thanks for thoughts @kv2019i ! Makes sense. I hope we can soon clean up this dependency into maths library. It's been causing difficulty with testbench build (shared libraries time).

Thanks, I am fully agree with you, @kv2019i @singalsu
let's do it later with another PR for move those to math directory.

I think ".." is better in this case to make it very clear this module is reusing definitions of a peer module

I tested this and it Just Works:

zephyr_library_include_directories_ifdef(CONFIG_COMP_MULTIBAND_DRC ${SOF_AUDIO_PATH}/crossover )

This expresses the multiband->crossover dependency (or any other similar dependency) very clearly and in a regular, "CMake-native" way.

To me, adding a non-include folder into target_include_directories is opening a can of worms.
Then the build is searching for headers in the implementation parts of the code base and this can lead to surprises.

I don't see what could go wrong: there are very few .h files in the .c directories after all.

In fact the "real" question is: why is the .h file of an external module API located in the same directory as some .c files? This here is really the root-cause/of all evil.

If a "peer module" wants to be re-usable then I'm sure it can afford some include/ subdirectory, that's really not vey demanding and it clarifies what .h is external versus internal. Then:

zephyr_library_include_directories_ifdef(CONFIG_COMP_MULTIBAND_DRC ${SOF_AUDIO_PATH}/crossover/include )

and job done.

there is no similar usage for this, also Kai don't want add new path for include, even with ifdef, this path are added for full sof build, I simply move user header one more step, this should resolve the error.

btian1 · 2023-12-11T02:08:04Z

SOFCI TEST

lyakh

headers look roughly reasonable now, thanks!

kv2019i

Code looks good, but we need to discuss and reach a consensus how we handle cross-module dependencies (and their headers). See inline and my comments to DRC refactor PR.

kv2019i · 2023-12-11T12:37:32Z

src/audio/multiband_drc/multiband_drc.h

 #include <sof/audio/drc/drc.h>
 #include <sof/math/iir_df2t.h>
 #include <sof/audio/component.h>
 #include <sof/audio/data_blob.h>
 #include <sof/platform.h>
 #include <stdint.h>
 #include "user/multiband_drc.h"
+#include "../crossover/crossover.h"


@marc-hb we do! I think ".." is better in this case to make it very clear this module is reusing definitions of a peer module (see my comments to #8593 ).

src/audio/crossover/crossover.c

marc-hb

Blocking merge until the build is fixed.

Also, a Googler's name is all over the git blame yet no one from Google has reviewed yet. According to @cujomalainey in #8593, @johnylin76 should be back soon.

but we need to discuss and reach a consensus how we handle cross-module dependencies (and their headers). See inline and my comments to DRC refactor PR.

I don't have a strong opinion about #include but I recommended a couple, simple solutions that don't require any #include "../foo.h" and that look like a more "normal" dependency in CMake.

I do have a much stronger opinion on partially breaking the disassembly on Windows, see example above. Reminder in case anyone didn't notice: this PR "breaks the build"!

I have a very strong suspicion that something somewhere in the toolchain when it runs on Windows does not like the unusual #include "../foo.h", probably because of some backslash-forward slash issue. Something similar already happened in ed7c57b [*]. Indeed, if you look at some Windows build log you will find a mix of forward slashes and backslashes: https://github.com/thesofproject/sof/actions/runs/7176152352/job/19540597509?pr=8587

[*]

zephyr/CMakeLists.txt: remove '..' in include paths #7428

marc-hb · 2023-12-13T07:22:39Z

src/audio/multiband_drc/multiband_drc.h

 #include <sof/audio/drc/drc.h>
 #include <sof/math/iir_df2t.h>
 #include <sof/audio/component.h>
 #include <sof/audio/data_blob.h>
 #include <sof/platform.h>
 #include <stdint.h>
 #include "user/multiband_drc.h"
+#include "../crossover/crossover.h"


I think ".." is better in this case to make it very clear this module is reusing definitions of a peer module

I tested this and it Just Works:

zephyr_library_include_directories_ifdef(CONFIG_COMP_MULTIBAND_DRC ${SOF_AUDIO_PATH}/crossover )

This expresses the multiband->crossover dependency (or any other similar dependency) very clearly and in a regular, "CMake-native" way.

To me, adding a non-include folder into target_include_directories is opening a can of worms.
Then the build is searching for headers in the implementation parts of the code base and this can lead to surprises.

I don't see what could go wrong: there are very few .h files in the .c directories after all.

In fact the "real" question is: why is the .h file of an external module API located in the same directory as some .c files? This here is really the root-cause/of all evil.

If a "peer module" wants to be re-usable then I'm sure it can afford some include/ subdirectory, that's really not vey demanding and it clarifies what .h is external versus internal. Then:

zephyr_library_include_directories_ifdef(CONFIG_COMP_MULTIBAND_DRC ${SOF_AUDIO_PATH}/crossover/include )

and job done.

btian1 · 2023-12-13T08:01:41Z

Blocking merge until the build is fixed.

Also, a Googler's name is all over the git blame yet no one from Google has reviewed yet. According to @cujomalainey in #8593, @johnylin76 should be back soon.

but we need to discuss and reach a consensus how we handle cross-module dependencies (and their headers). See inline and my comments to DRC refactor PR.

I don't have a strong opinion about #include but I recommended a couple, simple solutions that don't require any #include "../foo.h" and that look like a more "normal" dependency in CMake.

I do have a much stronger opinion on partially breaking the disassembly on Windows, see example above. Reminder in case anyone didn't notice: this PR "breaks the build"!

I have a very strong suspicion that something somewhere in the toolchain when it runs on Windows does not like the unusual #include "../foo.h", probably because of some backslash-forward slash issue. Something similar already happened in ed7c57b. Indeed, if you look at some Windows build log you will find a mix of forward slashes and backslashes: https://github.com/thesofproject/sof/actions/runs/7176152352/job/19540597509?pr=8587

@marc-hb ， I also ahve concern on this error difference, @kv2019i , I can add one more patch: move out header file from user folder, and rename it with _user.h, then include will only have "../" instead of "../../", this is a intermediate solution, do you agree on this?

singalsu · 2023-12-13T08:31:16Z

@marc-hb The dependency from multiband-DRC to crossover will be cleaned up by moving the common parts to maths library (IIR filterbank). But it can't be done in this PR.

keqiaozhang · 2023-12-14T05:37:36Z

SOFCI TEST

cujomalainey · 2023-12-14T16:40:24Z

src/audio/crossover/crossover.c

 #include <user/eq.h>
 #include <errno.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <limits.h>

+#include "crossover_algorithm.h"
+#include "crossover_user.h"


is there a doc/comment/anything explaining the new expected layout of the headers?

@cujomalainey I was looking for same here -> please see #8557 (comment) explanation from @lgirdwood

Let me file an enhancement retrospectively -- these cross-module usages require alignment for sure.

Ok what about how modules that depend on each other, is the plan to just to reach into their private files?

@cujomalainey ，this is the GH link: #8630
we may need discuss how to implement modules that depend on each other.

marc-hb · 2023-12-15T04:55:57Z

The disassembly is still different in https://github.com/thesofproject/sof/actions/runs/7207650079/job/19635312145?pr=8587

btian1 · 2023-12-15T06:40:59Z

The disassembly is still different in https://github.com/thesofproject/sof/actions/runs/7207650079/job/19635312145?pr=8587

yes, I think we should first resolve the module dependency before review and merge this and DRC PR.
@singalsu @kv2019i

I will submit patch next week try to resolve the module dependency.

Since there is only one file for user folder, move it out and delete this folder directly. Signed-off-by: Baofeng Tian <[email protected]>

marc-hb · 2023-12-19T06:56:46Z

Not sure what happened but Windows and Linux are now the same in https://github.com/thesofproject/sof/actions/runs/7258301081/job/19773565761?pr=8587

Windows linux difference fixed

lgirdwood · 2023-12-19T19:57:04Z

@btian1 can you fix the conflicts and make non draft. thanks.

btian1 · 2023-12-20T08:29:06Z

closing this, due to there are much conflicts than expected, I create new PR for continue review:
#8652

btian1 changed the title ~~audio: crossover: move header file to module folder~~ audio: crossover: crossover code refactor Dec 7, 2023

btian1 force-pushed the crossover_refactor branch from 569abb2 to 9f79336 Compare December 7, 2023 07:45

btian1 marked this pull request as ready for review December 7, 2023 07:46

btian1 requested review from marc-hb, a team, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners December 7, 2023 07:46

lyakh reviewed Dec 7, 2023

View reviewed changes

btian1 force-pushed the crossover_refactor branch 2 times, most recently from 7ba8018 to 5c67299 Compare December 7, 2023 13:41

marc-hb reviewed Dec 8, 2023

View reviewed changes

cujomalainey requested a review from johnylin76 December 8, 2023 14:54

lgirdwood approved these changes Dec 8, 2023

View reviewed changes

lyakh approved these changes Dec 11, 2023

View reviewed changes

kv2019i reviewed Dec 11, 2023

View reviewed changes

btian1 force-pushed the crossover_refactor branch 2 times, most recently from 1fc1e50 to 662e7ee Compare December 12, 2023 02:32

marc-hb mentioned this pull request Dec 13, 2023

DRC code refactor #8593

Merged

marc-hb previously requested changes Dec 13, 2023

View reviewed changes

btian1 force-pushed the crossover_refactor branch from 662e7ee to b2da484 Compare December 14, 2023 02:37

btian1 force-pushed the crossover_refactor branch from b2da484 to 56079b8 Compare December 14, 2023 03:00

btian1 force-pushed the crossover_refactor branch 2 times, most recently from a8cbf37 to 612a400 Compare December 14, 2023 10:33

cujomalainey reviewed Dec 14, 2023

View reviewed changes

kv2019i mentioned this pull request Dec 14, 2023

[FEATURE] organize audio module source code to per-module directories #8630

Closed

btian1 marked this pull request as draft December 15, 2023 06:41

btian1 force-pushed the crossover_refactor branch from 612a400 to 4a494c1 Compare December 19, 2023 06:39

audio: multiband_drc: move out multiband drc user header file

4a494c1

Since there is only one file for user folder, move it out and delete this folder directly. Signed-off-by: Baofeng Tian <[email protected]>

btian1 closed this Dec 20, 2023

This was referenced Apr 11, 2024

meta issue for reproducible builds (was: tests/determinism.sh) zephyrproject-rtos/zephyr#14593

Closed

nxp: imx8/imx8x: switch to Zephyr logging #9021

Closed

Windows/Linux disassembly differences when using #include ../something.h? #9034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audio: crossover: crossover code refactor #8587

audio: crossover: crossover code refactor #8587

btian1 commented Dec 7, 2023 •

edited

Loading

lyakh Dec 7, 2023

btian1 Dec 7, 2023

btian1 Dec 8, 2023

lyakh Dec 7, 2023

marc-hb commented Dec 8, 2023 •

edited

Loading

marc-hb Dec 8, 2023

btian1 Dec 11, 2023 •

edited by marc-hb

Loading

kv2019i Dec 11, 2023

marc-hb Dec 11, 2023

marc-hb Dec 11, 2023

kv2019i Dec 12, 2023

singalsu Dec 12, 2023

btian1 Dec 12, 2023

marc-hb Dec 13, 2023

btian1 Dec 14, 2023

btian1 commented Dec 11, 2023

lyakh left a comment

kv2019i left a comment

kv2019i Dec 11, 2023

marc-hb left a comment •

edited

Loading

marc-hb Dec 13, 2023

btian1 commented Dec 13, 2023

singalsu commented Dec 13, 2023

keqiaozhang commented Dec 14, 2023

cujomalainey Dec 14, 2023

kv2019i Dec 14, 2023

cujomalainey Dec 14, 2023

btian1 Dec 19, 2023

marc-hb commented Dec 15, 2023

btian1 commented Dec 15, 2023

marc-hb commented Dec 19, 2023

lgirdwood commented Dec 19, 2023

btian1 commented Dec 20, 2023

audio: crossover: crossover code refactor #8587

audio: crossover: crossover code refactor #8587

Conversation

btian1 commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

btian1 Dec 11, 2023 • edited by marc-hb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 commented Dec 11, 2023

lyakh left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 commented Dec 13, 2023

singalsu commented Dec 13, 2023

keqiaozhang commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Dec 15, 2023

btian1 commented Dec 15, 2023

marc-hb commented Dec 19, 2023

lgirdwood commented Dec 19, 2023

btian1 commented Dec 20, 2023

btian1 commented Dec 7, 2023 •

edited

Loading

marc-hb commented Dec 8, 2023 •

edited

Loading

btian1 Dec 11, 2023 •

edited by marc-hb

Loading

marc-hb left a comment •

edited

Loading