audio: src: refactor src component with more common code in src source file #8161

btian1 · 2023-09-06T01:55:46Z

move out all ipc3 and ipc4 specifc code, data structure and functions.

iuliana-prodan · 2023-09-06T07:51:34Z

@btian1 why are you splitting IPC3 vs IPC4 on component level?
Do you (meaning Intel) plan of doing this for all audio components?
What is the purpose of this separation?

btian1 · 2023-09-06T08:32:39Z

@btian1 why are you splitting IPC3 vs IPC4 on component level? Do you (meaning Intel) plan of doing this for all audio components? What is the purpose of this separation?

@iuliana-prodan , you can see that current module become more and more larger, and inside, ipc3 and ipc4 code are mixed together, make code hard to read/understand/maintain.

these changes have two purpose:

move ipc specific code to separate source file.
move back header file to source directory.

currently, volume was done, module adapter is in: #8144, this is the third one.

lyakh · 2023-09-06T08:52:45Z

src/audio/src/src_uuid.h

+#if CONFIG_IPC_MAJOR_4
+
+/* e61bb28d-149a-4c1f-b709-46823ef5f5a3 */
+DECLARE_SOF_RT_UUID("src", src_uuid, 0xe61bb28d, 0x149a, 0x4c1f,


the name is confusing, this isn't a declaration, it's a definition - it actually defines objects. So, this shouldn't be in a header. Besides - this can be perfectly placed in src_ipc4.c and src_ipc3.c, they divide very well.

I also want to put it ipc3 and ipc4 separately, however, if you check DECLARE_SOF_RT_UUID inside, you will find static definition, how it can be referred by src.c?

#define DECLARE_SOF_UUID(entity_name, uuid_name,
va, vb, vc,
vd0, vd1, vd2, vd3, vd4, vd5, vd6, vd7)
__section(".static_uuids")
static const struct sof_uuid_entry uuid_name ## _ldc = {
{.a = va, .b = vb, .c = vc,
.d = {vd0, vd1, vd2, vd3, vd4, vd5, vd6, vd7}},
entity_name "\0"
}

right, that defined an object called src_uuid_ldc, right? And that one is then used in DECLARE_TR_CTX(src_tr, SOF_UUID(src_uuid), LOG_LEVEL_INFO); So I would either duplicate that single line DECLARE_TR_CTX(...); in both ipc3 and ipc4 files or leave them all in src.c with #ifs but still please don't include them in a header. By placing them in that header and only including it in src.c you effectively placing them there, so you can just do that explicitly instead.

there is another way to use src_uuid:
DECLARE_MODULE_ADAPTER(src_interface, src_uuid, src_tr);
hard to resolve this one.

sorry, what do you mean? What do you need to resolve there? Its use of src_uuid decodes as

static const struct comp_driver comp_##adapter##_module = { \ .type = SOF_COMP_MODULE_ADAPTER, \ .uid = &src_uuid, \

and src_uuid is defined by

DECLARE_SOF_RT_UUID("src", src_uuid, 0xe61bb28d, 0x149a, 0x4c1f, 0xb7, 0x09, 0x46, 0x82, 0x3e, 0xf5, 0xf5, 0xae);

as

const struct sof_uuid src_uuid = { \

i.e. that one isn't static

then have to add extern part, I added, please check again, not sure this looks better or not.

Yes, that now looks correct C. Someone might complain that macros like DECLARE_SOF_RT_UUID() and DECLARE_TR_CTX() are supposed to hide their implementation and you aren't supposed to access internals of the code, that they generate, but I'd say that isn't a major concern.

lyakh · 2023-09-06T08:53:54Z

src/audio/src/src_uuid.h

+#error "No or invalid IPC MAJOR version selected."
+#endif /* CONFIG_IPC_MAJOR_4 */
+
+DECLARE_TR_CTX(src_tr, SOF_UUID(src_uuid), LOG_LEVEL_INFO);


this also defines an object, please, keep it in .c

lyakh · 2023-09-06T08:56:40Z

src/audio/src/src_ipc3.c

+
+void src_set_sink_params(struct comp_dev *dev, struct sof_source __sparse_cache *source,
+			 struct sof_sink __sparse_cache *sink)
+{


well, this function now isn't really setting sink parameters? Please choose a different name

@singalsu any suggestions for appropriate name.

In IPC4 build the same name function sets source and sink parameters and these component data members are updated in init() where directly received from kernel.

If IPC3 function name is changed, then it could be src_get_source_sink_params(), since in IPC3 these are retrieved from params walk result for buffers.

singalsu · 2023-09-07T07:39:33Z

src/audio/src/src/src_ipc4_int32_10_21_2500_5000.h

@@ -5,7 +5,7 @@
 */

 /** \cond GENERATED_BY_TOOLS_TUNE_SRC */
-#include <sof/audio/src/src.h>
+#include "../src.h"


Why is the location changed? The coefficients are generic polyphase FIR coefficients for a fractional ratio and could be in theory used by some other SRC like component. It's not a big deal to move and move back if need but was this causing issues?

If needed please make this change also to SRC coefficients generator. It can be a separate PR. These files must be directly generated without any manual edits done.

@singalsu , intention is move all related header to one folder, you can check volume part, all volume related moved to src/audio/volume folder.

OK!

Can you please make another PR to change this similarly

sof/tools/tune/src/src_export_coef.m

Line 53 in 2c6c1b4

fprintf(fh, '#include <sof/audio/src/src.h>\n');

Try to run e.g. https://github.com/thesofproject/sof/blob/main/tools/tune/src/src_ipc4_int32.m in octave

and check that you get similar h-files as these edited.

Also I think directory name "coefficients" would be better than "src" for the moved header files.

lyakh

The actual blocking comment has been addressed - thanks. It still would be good to rename src_set_source_sink_params() and of course this needs an approval from @singalsu when all his concerns have been addressed.

lgirdwood

LGTM, name can be fixed incrementally.

lgirdwood · 2023-09-08T12:36:10Z

src/audio/src/src_ipc3.c

+
+void src_set_sink_params(struct comp_dev *dev, struct sof_source __sparse_cache *source,
+			 struct sof_sink __sparse_cache *sink)
+{


@singalsu any suggestions for appropriate name.

singalsu · 2023-09-08T12:43:35Z

src/audio/src/src/src_small_int32_table.h

-#include <sof/audio/coefficients/src/src_small_int32_20_21_4167_5000.h>
-#include <sof/audio/coefficients/src/src_small_int32_21_20_4167_5000.h>
-#include <sof/audio/src/src.h>
+#include "src_small_int32_1_2_2268_5000.h"


This change needs to be updated (other PR) here

sof/tools/tune/src/src_export_table_2s.m

Line 85 in 2c6c1b4

fprintf(fh, '#include <%ssrc_%s_%d_%d_%d_%d.h>\n', ...

singalsu · 2023-09-08T12:44:00Z

src/audio/src/src/src_small_int32_table.h

+#include "src_small_int32_8_7_4535_5000.h"
+#include "src_small_int32_20_21_4167_5000.h"
+#include "src_small_int32_21_20_4167_5000.h"
+#include "../src.h"


And this change here

sof/tools/tune/src/src_export_table_2s.m

Line 94 in 2c6c1b4

fprintf(fh, '#include <sof/audio/src/src.h>\n');

kv2019i

Looks good. #8167 will conflict though, and needs to go first due to its dependencies (if it is ready today).

kv2019i · 2023-09-11T07:32:48Z

zephyr/CMakeLists.txt

+		${SOF_AUDIO_PATH}/src/src.c
+		${SOF_AUDIO_PATH}/src/src_ipc4.c
+	)
+endif()


Nitpick: you could add the common files in one statement and then only have a if-IPC3-else-IPC4 case to select between src_ipc3.c and src_ipc4.c. No need to duplicate the lists for both IPC versions,

it include zephyr_library_sources_ifdef, I am afraid I can't handle it well, so duplicate for security.
you can see volume also use same way, we can improve this later when there is a new change.

Currently, this PR is ready for merge.

move component data out of source file to avoid ipc3 and ipc4 usage in src source file. Signed-off-by: Baofeng Tian <[email protected]>

create new files to cover ipc3 and ipc4 specific code. Signed-off-by: Baofeng Tian <[email protected]>

previously, these headers are located in include directory. Now, move it to src/audio/src/ directory, since these headers are only used by src module. Signed-off-by: Baofeng Tian <[email protected]>

rename it from src to coef to reflect the real content inside. Signed-off-by: Baofeng Tian <[email protected]>

kv2019i · 2023-09-13T09:08:43Z

D'oh, my bad, I checked @singalsu you had approved, but it seemed you had NOT. Please double-check the issues where addressed correctly. @btian1 please follow up

btian1 · 2023-09-14T01:37:34Z

D'oh, my bad, I checked @singalsu you had approved, but it seemed you had NOT. Please double-check the issues where addressed correctly. @btian1 please follow up

@kv2019i , @singalsu , I think what Seppo mentioned are addressed, only matlab file change is not yet covered, it need use another PR to continue, no worry no this.

src --> coef
function name change from src_set_source_sink_params --> src_get_source_sink_params
matlab file change will be covered by another PR.

btian1 force-pushed the src_split branch 3 times, most recently from 9dc6084 to 8531759 Compare September 6, 2023 06:53

lyakh requested changes Sep 6, 2023

View reviewed changes

btian1 marked this pull request as ready for review September 7, 2023 01:56

btian1 requested review from marc-hb, singalsu, abonislawski, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners September 7, 2023 01:56

btian1 force-pushed the src_split branch from 8531759 to bf9f62f Compare September 7, 2023 02:29

singalsu reviewed Sep 7, 2023

View reviewed changes

btian1 force-pushed the src_split branch 2 times, most recently from 9dcccc4 to 691d821 Compare September 8, 2023 03:06

lyakh approved these changes Sep 8, 2023

View reviewed changes

lgirdwood approved these changes Sep 8, 2023

View reviewed changes

singalsu reviewed Sep 8, 2023

View reviewed changes

btian1 force-pushed the src_split branch from 691d821 to b60aa6c Compare September 11, 2023 01:34

kv2019i approved these changes Sep 11, 2023

View reviewed changes

marcinszkudlinski approved these changes Sep 11, 2023

View reviewed changes

audio: src: move out src component data out from src source

b397032

move component data out of source file to avoid ipc3 and ipc4 usage in src source file. Signed-off-by: Baofeng Tian <[email protected]>

btian1 force-pushed the src_split branch from b60aa6c to 6141692 Compare September 13, 2023 03:26

btian1 added 2 commits September 13, 2023 11:32

audio: src: split src ipc3 and ipc4 specific file

2cf779e

create new files to cover ipc3 and ipc4 specific code. Signed-off-by: Baofeng Tian <[email protected]>

audio: src: move src header file to source folder

c89be33

previously, these headers are located in include directory. Now, move it to src/audio/src/ directory, since these headers are only used by src module. Signed-off-by: Baofeng Tian <[email protected]>

audio: src: rename sample rate converter coefficient folder name

6141692

rename it from src to coef to reflect the real content inside. Signed-off-by: Baofeng Tian <[email protected]>

kv2019i merged commit 88fdb10 into thesofproject:main Sep 13, 2023
37 of 41 checks passed

andrula-song mentioned this pull request Nov 24, 2023

Audio: ASRC: Code reorganization #8514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audio: src: refactor src component with more common code in src source file #8161

audio: src: refactor src component with more common code in src source file #8161

btian1 commented Sep 6, 2023

iuliana-prodan commented Sep 6, 2023

btian1 commented Sep 6, 2023

lyakh Sep 6, 2023

btian1 Sep 6, 2023

lyakh Sep 7, 2023

btian1 Sep 7, 2023

lyakh Sep 7, 2023

btian1 Sep 8, 2023

lyakh Sep 8, 2023

lyakh Sep 6, 2023

lyakh Sep 6, 2023

lgirdwood Sep 8, 2023

singalsu Sep 8, 2023

singalsu Sep 7, 2023

btian1 Sep 8, 2023

singalsu Sep 8, 2023

singalsu Sep 8, 2023

lyakh left a comment

lgirdwood left a comment

lgirdwood Sep 8, 2023

singalsu Sep 8, 2023

singalsu Sep 8, 2023

kv2019i left a comment

kv2019i Sep 11, 2023

btian1 Sep 11, 2023

kv2019i commented Sep 13, 2023

btian1 commented Sep 14, 2023 •

edited

Loading

audio: src: refactor src component with more common code in src source file #8161

audio: src: refactor src component with more common code in src source file #8161

Conversation

btian1 commented Sep 6, 2023

iuliana-prodan commented Sep 6, 2023

btian1 commented Sep 6, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

lgirdwood left a 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

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Sep 13, 2023

btian1 commented Sep 14, 2023 • edited Loading

btian1 commented Sep 14, 2023 •

edited

Loading