Module api conversion of math functions and mfcc module, and cmocka fixes #10293
Conversation
jsarha
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
singalsu
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR converts math functions and the MFCC module to use a new module API for memory management, while also fixing memory leaks in cmocka tests to enable Valgrind compatibility.
- Converts math functions (FFT, DCT, matrix, auditory) from direct memory allocation to module-based allocation
- Updates MFCC module to use module API for memory management
- Fixes memory leaks in cmocka tests by adding proper cleanup and using module allocation functions
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/cmocka/src/math/matrix/matrix.c | Updates matrix tests to use module allocation and adds proper cleanup |
| test/cmocka/src/math/fft/fft.c | Updates FFT tests to use module allocation and adds buffer cleanup |
| test/cmocka/src/math/dct/dct.c | Updates DCT tests to use module allocation and adds cleanup |
| test/cmocka/src/math/auditory/auditory.c | Updates auditory tests to use module allocation and adds cleanup |
| test/cmocka/src/common_mocks.c | Adds mock implementations for module allocation functions |
| test/cmocka/src/audio/volume/volume_process.c | Fixes memory leak by freeing allocated parameters |
| test/cmocka/src/audio/pipeline/pipeline_new.c | Adds cleanup for pipeline test data |
| test/cmocka/src/audio/pipeline/pipeline_free.c | Adds proper teardown for pipeline objects |
| test/cmocka/src/audio/pipeline/pipeline_connection_mocks.h | Adds declaration for cleanup function |
| test/cmocka/src/audio/pipeline/pipeline_connection_mocks.c | Implements cleanup function and fixes memory allocation |
| test/cmocka/src/audio/pipeline/pipeline_connect_upstream.c | Adds proper teardown for pipeline objects |
| test/cmocka/src/audio/mux/mux_copy.c | Fixes memory leaks in mux tests |
| test/cmocka/src/audio/mux/demux_copy.c | Fixes memory leaks in demux tests |
| test/cmocka/src/audio/module_adapter_test.c | Fixes incorrect function call |
| src/math/fft/fft_common.c | Adds module-based allocation variants of FFT functions |
| src/math/dct.c | Adds module-based allocation variants of DCT functions |
| src/math/auditory/auditory.c | Adds module-based allocation variants of auditory functions |
| src/include/sof/math/matrix.h | Adds module-based matrix allocation function |
| src/include/sof/math/fft.h | Adds declarations for module-based FFT functions |
| src/include/sof/math/dct.h | Adds declarations for module-based DCT functions |
| src/include/sof/math/auditory.h | Adds declarations for module-based auditory functions |
| src/include/sof/audio/mfcc/mfcc_comp.h | Updates function signature for module-based cleanup |
| src/audio/mfcc/mfcc_setup.c | Converts MFCC setup to use module allocation and simplifies error handling |
| src/audio/mfcc/mfcc.c | Updates MFCC module to use module allocation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| free(ptr); | ||
| } | ||
|
|
||
| void WEAK *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment) |
There was a problem hiding this comment.
what is the purpose of WEAK?
There was a problem hiding this comment.
If the module adapter code is linked to the test, then the orginal implementation prevails, and there is no duplicate symbol error.
Make mod_alloc() versions of all heap using functions. The new functions are simple copy-paste versions of the original, just using mod_alloc() and friend instead of rmalloc() friends. The idea is to get rid off the original versions once all the modules are converted to module API heap usage and the cmocka math tests have been fixed to work with mod_alloc() versions. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
module_api_convert_math_mfcc_and_cmocka_fixes
branch
from
8f596aa to
1f4ceef
Compare
| mod_free(mod, cd->state.buffers); | ||
| mod_free(mod, cd->state.melfb.data); | ||
| mod_free(mod, cd->state.dct.matrix); | ||
| mod_free(mod, cd->state.lifter.matrix); |
There was a problem hiding this comment.
mfcc_free_buffers() is only called from mfcc_free(). So we don't need explicit mod_free() calls? Also mod_fft_plan_free() only calls mod_free() internally
There was a problem hiding this comment.
It indeed appears so. But now that I am restoring all the clean up code anyway, this code is again needed.
| fb.scratch_length1 = fft_size / sizeof(int16_t); | ||
| fb.scratch_length2 = fft_size / sizeof(int16_t); | ||
| ret = psy_get_mel_filterbank(&fb); | ||
| ret = mod_psy_get_mel_filterbank(&dummy, &fb); |
There was a problem hiding this comment.
can we make those functions accept NULL to avoid having to use a dummy object? But if dummies are only needed in cmocka, maybe better indeed keep this here
There was a problem hiding this comment.
I did not want to implement the devm/mod style code for cmocka tests too (pulling in the module adapter code looked like too big a task too), so I implemented a function prototype level compatibility to mod_allcoc and friends only. The whole point of this exercise was to make cmocka Valgrind run to pass again, so making another another layer of memory leak tests seemed redundant.
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
would it be possible to modify these functions in place without first adding new versions and then deleting original ones? E.g. in patch 1 modify function prototypes and add wrappers:
-void x(int y)
+void x_new(struct mod *p, int y)
{
...
- m = rmalloc(...);
+ if (p)
+ m = mod_alloc(p, ...);
+ else
+ m = rmalloc(...);
...
}
+
+void x(int y)
+{
+ return x_new(NULL, y);
+}
then move all users one by one to use x_new() and then remove the original x() and (optionally) rename x_new() back to x()?
There was a problem hiding this comment.
Maybe, but I am not volunteering for the task. What whoud be the point?
There was a problem hiding this comment.
Maybe, but I am not volunteering for the task. What whoud be the point?
@jsarha the point would be not to duplicate code even intermittently, keeping all commits small and easily reviewable
There was a problem hiding this comment.
I grant you the point obout being easier to review and to be sure the functions are still in essence the same. But I think our git repository can easily take the duplicated code.
This was a result of my conversion method. Making mod api versions of all that I needed for mfcc and then remove the obsolte functions and see what breaks. I really hope you do not put to redo also this part of this PR series. Good thing of course is that by now I know almost by heart all the allocs and frees in all the module code.
| return 0; | ||
| } | ||
|
|
||
| int mod_psy_free_mel_filterbank(struct processing_module *mod, struct psy_mel_filterbank *mel_fb) |
There was a problem hiding this comment.
is this ever used outside of the module destruction path?
There was a problem hiding this comment.
As I mentioned earlier, the cmocka tests to not implement the resource tracking and everything has to be freed manually in order to make Valgrind happy. And of course due to recent turn of events, the resource tracking has taken a step back in any case.
There was a problem hiding this comment.
Of course there probably is one or two places in mfcc where this function should be called, but that is another story. Once I get my per module resource tracking ready, the mfcc is one module that should take it into use among the first ones.
Allocate all memory, blob handlers, and fast_get() buffers through module API mod_alloc() and friends. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add mocks for mod_balloc_align() and mod_alloc_align(). These dummy versions do not keep track of the allocated memory or provide real control over alignment. They just forward the calls to regular malloc() and free(). Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Convert auditory, dct, fft, and matrix math tests to use module heap API functions instead of the old version using rmalloc() and friends directly. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Both unit tests and modules should now use the module API heap allocation functions, so we can now remove the directly heap using versions. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add mod_psy_free_mel_filterbank() for freeing the allocated memory. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix memory leaks from auditory cmocka test. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from fft test when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from matrix test when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add mod_dct_free_16() to free memory allocated by mod_dct_initialize_16(). Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix memory leaks found by running the tests with Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix memory freeing error from module_adapter_test_free(). Use free_test_source() for sources, not free_test_sink(). Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from volume test when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from mux_copy test when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from demux_copy test when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix all memory leaks found from all pipeline tests when tests were run under Valgrind. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
module_api_convert_math_mfcc_and_cmocka_fixes
branch
from
1f4ceef to
fd4f807
Compare
|
These new versions of module conversions use module API to allocate the resources, but the code still has all the resource freeing code in place. |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
nothing critical. If desired, some of my comments can be addressed in follow-up PRs
| while (lim < size) { | ||
| lim <<= 1; | ||
| len++; | ||
| } |
There was a problem hiding this comment.
this can be done without a loop with clz() and possibly checks for 0 and a power of 2
There was a problem hiding this comment.
A good proposal, but the issue has nothing to do with this PR.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
The duplicated versions in the series is a bit funky, but given the duplicated versions won't live longer in parallel, I'm ok with this.
4 participants
Module api conversion of math functions and mfcc module, and cmocka fixes. The cmocka fixes include the changes required by math function changes, and then on top of them fixes for all memorory leaks in cmocka tests. After this PR the cmocka tests can be run with Valgrind.