iadk: Fix GetConfiguration API function #10102
Conversation
softwarecki
requested review from
abonislawski,
Copilot,
jxstelter,
kv2019i,
lyakh,
marcinszkudlinski,
serhiy-katsyuba-intel and
tmleman
softwarecki
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the definition and usage of the GetConfiguration API for the IADK module adapter by switching the size parameters to be passed by reference/pointer.
- Update C++ adapter signature to take
data_offset_sizeandfragment_sizeby reference. - Adjust wrapper and caller sites to match the new parameter passing convention for
data_offset_size.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h | Changed data_offset_size and fragment_size to references in the adapter API. |
| src/audio/module_adapter/module/modules.c | Updated call to iadk_wrapper_get_configuration to pass data_offset_size pointer. |
| src/audio/module_adapter/iadk/iadk_module_adapter.cpp | Modified wrapper signature to accept data_offset_size pointer. |
| uint8_t *fragment, size_t fragment_size) | ||
| { | ||
| struct IadkModuleAdapter *mod_adp = (struct IadkModuleAdapter *) md; | ||
| return mod_adp->IadkModuleAdapter_GetConfiguration(config_id, pos, | ||
| data_offset_size, | ||
| *data_offset_size, | ||
| fragment, | ||
| fragment_size); |
There was a problem hiding this comment.
The wrapper iadk_wrapper_get_configuration still takes fragment_size by value, so updates from IadkModuleAdapter_GetConfiguration will be lost. Change the signature to size_t *fragment_size, dereference it in the call, and assign back the updated size.
| return iadk_wrapper_get_configuration(module_get_private_data(mod), config_id, | ||
| MODULE_CFG_FRAGMENT_SINGLE, *data_offset_size, | ||
| MODULE_CFG_FRAGMENT_SINGLE, data_offset_size, | ||
| fragment, fragment_size); |
There was a problem hiding this comment.
After updating the wrapper to accept size_t *fragment_size, this call should pass &fragment_size instead of fragment_size, so the new fragment size is propagated correctly.
| fragment, fragment_size); | |
| fragment, &fragment_size); |
There was a problem hiding this comment.
What is the reference API doing ?
tmleman
left a comment
tmleman
left a comment
There was a problem hiding this comment.
Could you add const for input-only parameters and document if data_offset_size/fragment_size are [in], [out], or [in,out]?
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
need to fix fragment_size
The GetConfiguration function was incorrectly defined for IADK module adapter. This patch fixes the definition. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com> Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
softwarecki
force-pushed
the
iadk-getconfig
branch
from
d847de3 to
2547365
Compare
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
Looks correct now. I guess the previous version would fail an IADK compilation test, do we have such tests in GH, Jenkins or internal? Or can we add some?
| * \param[in] pos - position of the fragment in the large message | ||
| * \param[in] data_offset_size: size of the whole configuration if it is the first fragment or the | ||
| * only fragment. Otherwise, it is the offset of the fragment in the whole configuration. | ||
| * \param[in] fragment: configuration fragment buffer |
There was a problem hiding this comment.
Here https://github.com/thesofproject/sof/blob/main/src/include/sof/audio/module_adapter/iadk/processing_module_interface.h#L338-L367 fragment is described as output.
There was a problem hiding this comment.
ok, looks like we are good to go once the API buffer is aligned,
6 participants
The GetConfiguration function was incorrectly defined for IADK module adapter. This patch fixes the definition.