DRC code refactor #8593
DRC code refactor #8593
Conversation
btian1
requested review from
a team,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
| #include <stddef.h> | ||
| #include <stdint.h> | ||
|
|
||
| #include "../drc/drc_algorithm.h" |
There was a problem hiding this comment.
as @marc-hb also has pointed out elsewhere[*], such includes aren't very pretty, would be better to keep shared between modules headers in an "include" directory
[*]
There was a problem hiding this comment.
This isn't a fully clearcut case either. Here multiband_drc is directly building on top of drc (and also uses crossover).
I think we should also ensure this type of cross-module resuse is easy to do and code stays maintainable.
@lgirdwood Any input, should we keep such reused headers (like drc_algorithm.h) in common code?
I think I'd personally still code with the approach in this PR. The modules are deeply dependent on each other, so having a "../drc/drc_algorithm.h" makes sense. Having alone drc_algorithm.h in sof/audio/ seems harder to understand ("why is this header here but not others?").
There was a problem hiding this comment.
I don't have a huge problem with this approach, just looking for the best one. As alternatives we could either
- have an include directory under src/audio/include for such shared headers or
- add src/audio to the list of include paths
I think I'd find (2) preferable
There was a problem hiding this comment.
Seppo suggested move the common part to math folder in a separate PR, I will do it once those PRs get approval and merged.
There was a problem hiding this comment.
The modules are deeply dependent on each other, so having a "../drc/drc_algorithm.h" makes sense.
I measured how frequent are these "dot-dot" includes across all the default zephyr/west.yml repos. It's about 1.5%. So it's not "exotic" but it's definitely not "mainstream" either. As I just mentioned in #8630, there is zephyr_library_include_directories() and friends.
$ west grep '^[[:blank:]]*#[[:blank:]]*include[[:blank:]]*[<"]\.\.' | wc -l
2914
$ west grep '^[[:blank:]]*#[[:blank:]]*include[[:blank:]]*[<"][[:alnum:]]' | wc -l
228292
There was a problem hiding this comment.
how about non-zephyr cases? i.e: CONFIG_ZEPHYR_NATIVE_DRIVERS is not defined, will other build also support zephyr syntax?
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
The shared headers questions warrant a bit more discussion, see inline. Feedback from @johnylin76 as module owner also very welcome.
So the background is to make module implementation more standalone, with the module directory having all essential bits (code, headers, docs, Kconfig and possible meta data like a toml file in the future). As DRC is used as a basic for Multiband-DRC, this doesn't fit the goal as nicely -- but I think sharing module code is even more important, so we shouldn't go overboard trying to fit everything to the same one-directory model.
| #include <stddef.h> | ||
| #include <stdint.h> | ||
|
|
||
| #include "../drc/drc_algorithm.h" |
There was a problem hiding this comment.
This isn't a fully clearcut case either. Here multiband_drc is directly building on top of drc (and also uses crossover).
I think we should also ensure this type of cross-module resuse is easy to do and code stays maintainable.
@lgirdwood Any input, should we keep such reused headers (like drc_algorithm.h) in common code?
I think I'd personally still code with the approach in this PR. The modules are deeply dependent on each other, so having a "../drc/drc_algorithm.h" makes sense. Having alone drc_algorithm.h in sof/audio/ seems harder to understand ("why is this header here but not others?").
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. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>
These three functions are called by external modules and its prototype is not defined with inline, also these functions are not performance critical function, no need inline. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>
|
Please wait till @johnylin76 is back to review this as he is the original author of this code. |
johnylin76
left a comment
johnylin76
left a comment
There was a problem hiding this comment.
LGTM, thanks.
Multiband-DRC is a compound arithmetic model that consists of DRC, EQ, and CROSSOVER. (EQ is referenced from src/math/ which shouldn't bet a problem.)
I have some follow-up questions. Any advice will be appreciated.
- Do we plan the same code refactoring as DRC for CROSSOVER?
- What would be the good practice for such compound model for future reference?
- Is it proper to move the shared function, i.e. (drc|crossover)_algorithm, to src/math?
- I used to consider to assemble them in topology level, but soon after then I changed my mind due to the technical difficulties for structure in topology
|
|
7 participants
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.