-
Notifications
You must be signed in to change notification settings - Fork 22
Add aec_process_frame() to lib_aec #439
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
Conversation
c2a6e17 to
cefc053
Compare
beaf15b to
2d38521
Compare
e58df07 to
21e2318
Compare
61e0616 to
b71dc5a
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.
Pull request overview
This PR refactors the AEC (Acoustic Echo Cancellation) library to add a unified aec_process_frame() function that supports variable numbers of threads (1-3) through a task distribution parameter. The major changes include:
- Replacing
aec_api.hwithaec.has the main public API header - Adding
aec_process_frame()function to replace thread-specific implementations (aec_process_frame_1thread,aec_process_frame_2threads) - Refactoring the task distribution scheme to support dynamic thread counts
- Converting AEC from a static library to an interface library
- Moving global state (X_energy_recalc_bin) into shared state structure
Reviewed changes
Copilot reviewed 74 out of 75 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/shared/python/generate_task_distribution_scheme.py | Updated script to generate new task distribution format with thread_count and passes fields |
| modules/lib_aec/api/aec.h | New main API header replacing aec_api.h with simplified public interface |
| modules/lib_aec/api/aec_defines.h | Added conditional inclusion of aec_config.h and default values for AEC_MAX_Y_CHANNELS, AEC_MAX_X_CHANNELS |
| modules/lib_aec/api/aec_schedule.h | Refactored task distribution structures with renamed types and dynamic sizing |
| modules/lib_aec/api/aec_state.h | Updated state structures to use AEC_MAX_* defines, added X_energy_recalc_bin to shared state |
| modules/lib_aec/src/aec_process_frame.c | Major refactoring to support variable thread counts with PAR_THREADS_PJOBS macro |
| modules/lib_aec/src/aec_priv.h | Moved public API functions from aec_api.h, added documentation |
| test/lib_aec/*/CMakeLists.txt | Removed aec_task_distribution.h from autogenerated includes, added aec_conf_h_exists define |
| test/lib_aec/aec_unit_tests/src/*.c | Updated includes from aec_defines.h/aec_api.h to aec.h |
| examples/bare-metal//.c | Updated to use aec_process_frame() with tdist parameter instead of thread-specific functions |
Based on my review, all changes appear to be straightforward refactoring with trailing whitespace cleanup. The code maintains backward compatibility through the configuration system and properly handles the multi-threading scenarios. No critical issues were identified.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2ebc8a5 to
8000ac0
Compare
uvvpavel
left a comment
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.
Some comments, but all looks great!
| * aec_init() gets called with.*/ | ||
| unsigned num_x_channels; | ||
|
|
||
| unsigned X_energy_recalc_bin; |
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.
Does this need to be documented?
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.
done
CMakeLists.txt
Outdated
|
|
||
| ## Compile flags for C/C++ for all apps (for all platforms) | ||
| list(APPEND COMPILE_FLAGS -Os -MMD -g) | ||
| #list(APPEND COMPILE_FLAGS -Os -MMD -g) |
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.
I think this line can be removed
CHANGELOG.rst
Outdated
| * ADDED: `aec_process_frame()` to `lib_aec` | ||
| * ADDED: AEC memory pool structs (`aec_memory_pool_t` and `aec_shadow_filt_memory_pool_t`) to `lib_aec` |
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.
I think it's worth mentioning that aec_api.h has been moved to aec.h
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.
done
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.
Do we still use the static library anywhere? If not, I would suggest removing it and renaming the interface cmake target to the original name
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.
We use it for the cffi tests. It needs to link to a proper .a
| fwk_voice::agc | ||
| fwk_voice::ic | ||
| fwk_voice::example::aec2thread | ||
| fwk_voice::aec |
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.
aec is already linked here
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! fixed everywhere
| fwk_voice::agc | ||
| fwk_voice::ic | ||
| fwk_voice::example::aec1thread | ||
| fwk_voice::aec |
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.
aec is already linked here
| "-report" | ||
| "${CONFIG_XSCOPE_PATH}/config.xscope") | ||
| elseif(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin") | ||
| elseif(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "Darwin") |
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.
Didn't look too closesly but I hope you know what you're doing here, same in the next instance
| fwk_voice::test::shared::test_utils | ||
| fwk_voice::test::shared::unity | ||
| fwk_voice::example::aec1thread) | ||
| fwk_voice::aec) |
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.
aec is already linked here
| fwk_voice::aec | ||
| fwk_voice::adec | ||
| fwk_voice::example::aec1thread | ||
| fwk_voice::aec |
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.
aec is already linked here
| fwk_voice::test::shared::test_utils | ||
| fwk_voice::test::shared::unity | ||
| fwk_voice::example::aec1thread) | ||
| fwk_voice::aec) |
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.
aec is already linked here
47b75f0 to
ee8322d
Compare
- Move aec_memory_pool into lib_aec. As a result lib_aec is now an interface (and not static) library, with the AEC_MAX_Y_CHANNELS, AEC_MAX_X_CHANNELS, AEC_MAIN_FILTER_PHASES and AEC_SHADOW_FILTER_PHASES overridable by the application. - Add aec_task_distribution_t to lib_aec and extend aec_process_frame() to accept the schedule as a const aec_task_distribution_t* pointer. - Remove fwk_voice::example::aec_process_frame target - Remove linking to libm from example and test CMakeLists.txt
ee8322d to
4dc8957
Compare
Pending and will be part of future PRs -