Skip to content
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

Audio: RTNR: Convert to module API #8324

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

singalsu
Copy link
Collaborator

No description provided.

src/audio/rtnr/rtnr.c Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
@singalsu
Copy link
Collaborator Author

This is early draft version, shared to find about opens related to module conversion. Currently if fails in init() due to not having configuration blob available, that would be sent after init(), before prepare() by kernel.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean, I assume you can test using 3P pass through mode ?

tools/rimage/config/tgl-cavs.toml Outdated Show resolved Hide resolved
src/arch/host/configs/library_defconfig Outdated Show resolved Hide resolved
tools/testbench/common_test.c Outdated Show resolved Hide resolved
@singalsu
Copy link
Collaborator Author

Looks clean, I assume you can test using 3P pass through mode ?

I need to modify further the code to run it, now the problem with binary control arrival after init() prevents the init() to pass. I think it's too early to attempt to initialize the algorithm there. Also, it does not impact mockup, but there's an issue in controlling two internal features with same binary control. It would be simplest to have two binary controls unless I've understood this wrong.

@singalsu singalsu force-pushed the rtnr_module_convert branch from 33ac228 to 5491cb3 Compare November 2, 2023 18:20
src/audio/rtnr/rtnr.c Outdated Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
@singalsu singalsu force-pushed the rtnr_module_convert branch from 5491cb3 to ed63435 Compare November 6, 2023 15:11
src/audio/rtnr/rtnr.c Outdated Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
src/audio/rtnr/rtnr_stub.c Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
@lgirdwood lgirdwood added this to the v2.8 milestone Nov 13, 2023
@kv2019i kv2019i modified the milestones: v2.8, v2.9 Nov 24, 2023
src/audio/rtnr/rtnr.c Outdated Show resolved Hide resolved
@lgirdwood
Copy link
Member

@singalsu still draft ?

src/audio/rtnr/rtnr.c Show resolved Hide resolved
src/audio/rtnr/rtnr.c Show resolved Hide resolved
src/audio/rtnr/rtnr_stub.c Show resolved Hide resolved
@singalsu
Copy link
Collaborator Author

singalsu commented Jan 4, 2024

@singalsu still draft ?

There's some regression, the stub library tries to process 384 channels per frame. Function audio_stream_get_channels() returns it. The function is correct but the stream format gets messed up somewhere.

@singalsu singalsu force-pushed the rtnr_module_convert branch from 7756bfd to 3c6febe Compare January 4, 2024 14:31
/* Allocate something, to avoid return NULL and cause error
* in check of success of this.
*/
return rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, 42);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is 42 == sizeof(audio_stream_rtnr)? if so, better use sizeof, or if number are random, you can use 4, 42 is not easy to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it a macro. We don't know the real size used by the closed library, and the run fails if this is NULL. Since I had to choose something, number 42 a reference to plot of https://en.wikipedia.org/wiki/The_Hitchhiker%27s_Guide_to_the_Galaxy . :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may need move 42 definition from:
https://github.com/thesofproject/sof/pull/8301/files#diff-f09311b76861c69cd17f987de389037705c0964aaf78d0d72e0e6da39bcdb2ffR12

to a more common place and reuse it between rtnr and igo_nr module.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be done later, now, I am ok go for merge.

@lgirdwood
Copy link
Member

@singalsu any ETA on non draft ?

@singalsu singalsu force-pushed the rtnr_module_convert branch from 3c6febe to e65a043 Compare January 12, 2024 13:21
@singalsu
Copy link
Collaborator Author

Changing to ready for review!

The component now works with the stub as ALSA control disable and enable, no more stuck to xrun. I didn't change the rtnr_copy_to/from_sof_stream() from functions where the issue was seen. Apparently there was some regression in SOF that has got fixed.

@singalsu singalsu marked this pull request as ready for review January 12, 2024 13:27
This patch updates the RTNR component to module adapter API
and IPC4.

Signed-off-by: Seppo Ingalsuo <[email protected]>
The "sof," as local sources list is not correct. It causes
build fail with x86 testbench when CONFIG_COMP_RTNR_STUB
is set.

Signed-off-by: Seppo Ingalsuo <[email protected]>
This patch enables RTNR (with stubs) to be built and loaded
to testbench.

Signed-off-by: Seppo Ingalsuo <[email protected]>
The init code checks for NULL and errors, so need to return
from RTKMA_API_Context_Create() a valid allocated address. The
free is added to RTKMA_API_Context_Free().

Signed-off-by: Seppo Ingalsuo <[email protected]>
This change avoids with stub library version annoying playback
sound from non-updated sink buffer when the processing ALSA
switch control is enabled. The audio is passed through from
source to sink.

Signed-off-by: Seppo Ingalsuo <[email protected]>
This patch allows load of RTNR component with these platforms.

Signed-off-by: Seppo Ingalsuo <[email protected]>
@singalsu singalsu force-pushed the rtnr_module_convert branch from e65a043 to 210899d Compare January 12, 2024 13:35
@lgirdwood lgirdwood merged commit 14237ed into thesofproject:main Jan 17, 2024
43 of 46 checks passed
@singalsu singalsu deleted the rtnr_module_convert branch January 17, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants