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.

Sign up for GitHub

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

Jump to bottom

Add initial Google CTC component #8689

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Add initial Google CTC component #8689

merged 1 commit into from
Aug 6, 2024

Conversation

eddy1021
Copy link

@eddy1021 eddy1021 commented Jan 3, 2024

Add CTC component

@eddy1021 eddy1021 force-pushed the ctc branch 5 times, most recently from 6cb6a6a to 2cc2bad Compare January 3, 2024 22:10
lgirdwood
lgirdwood reviewed Jan 8, 2024
View reviewed changes
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.

@eddy1021 can you update to us the module API instead of component API (now deprecated) ? Module API works with both IPC3 & 4, the changes are mainly in registration/init with some functions using changed names and args. @singalsu can give you more details if needed. Thanks !

@eddy1021 eddy1021 force-pushed the ctc branch 2 times, most recently from a0a249b to 70aa53d Compare January 10, 2024 21:24
@eddy1021 eddy1021 force-pushed the ctc branch 3 times, most recently from 92446b7 to 747468c Compare January 24, 2024 20:52
@eddy1021 eddy1021 force-pushed the ctc branch 4 times, most recently from ec965f5 to e0b55d3 Compare February 6, 2024 23:04
@eddy1021 eddy1021 changed the title [WIP] CTC Add initial Google CTC component Feb 6, 2024
@eddy1021 eddy1021 marked this pull request as ready for review February 6, 2024 23:07
andyross
andyross reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

I guess I can declare myself expert enough to start pontificating a bit. A few spots seem like they might be errors, most are nitpicks.

third_party/include/google_ctc_audio_processing.h Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
lgirdwood
lgirdwood reviewed Feb 8, 2024
View reviewed changes
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.

@eddy1021 good to see a mock as part of this PR :)
@singalsu pls review.

@lyakh lyakh dismissed their stale review July 18, 2024 06:23

issue fixed

@lyakh
Copy link
Collaborator

lyakh commented Jul 18, 2024

@eddy1021 BTW, this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

kv2019i
kv2019i approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I think this is starting to be ready. Having a version in the tree could help with maintainance and get changes like the recent UUID registry and cleanup of the xtos/zephyr makefiles, "for free". These need to be addressed, but I'm ok to do these as follow-ups.

src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 19, 2024

this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I think you mean "open-source CI". I think (and hope) there are many closed CIs we don't all know about.

@lyakh
Copy link
Collaborator

lyakh commented Jul 19, 2024

this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I think you mean "open-source CI". I think (and hope) there are many closed CIs we don't all know about.

@marc-hb I mean the SOF GitHub PR CI - the one we all see results of when submitting our PRs.

@eddy1021 eddy1021 force-pushed the ctc branch 3 times, most recently from afe706a to ab415cc Compare July 24, 2024 20:30
@eddy1021
Copy link
Author

@eddy1021 BTW, this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I turned the Mock to default on now. Thanks for pointing this out!

@eddy1021 eddy1021 force-pushed the ctc branch 2 times, most recently from f5535e7 to 341fae2 Compare July 25, 2024 23:12
@lgirdwood
Copy link
Member

@eddy1021 CI has some scheduled downtime, I'll manually rerun CI on Monday.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 26, 2024 •
edited
Loading

@eddy1021 CI has some scheduled downtime, I'll manually rerun CI on Monday.

There was a downtime but that is not the main reason Jenkins did not test this. The main reason is because I had to add the magic keywords [SKIP SOF-TEST] in the past because of rapid-fire force pushes that were saturating our scarce hardware devices. It seems more quiet now; dropping it.

EDIT: Friday's downtime was actually cancelled for complex reasons.

@marc-hb marc-hb changed the title [SKIP SOF-TEST] Add initial Google CTC component Add initial Google CTC component Jul 26, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 26, 2024

SOFCI TEST

1 similar comment
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 29, 2024

SOFCI TEST

andyross
andyross approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Sneak in and refresh my vote

cujomalainey
cujomalainey suggested changes Jul 30, 2024
View reviewed changes
src/audio/google/Kconfig Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing_ipc4.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing_mock.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
src/audio/google/google_ctc_audio_processing.c Outdated Show resolved Hide resolved
zephyr/CMakeLists.txt Show resolved Hide resolved
@eddy1021 eddy1021 force-pushed the ctc branch 2 times, most recently from 2089309 to 92ee97e Compare July 31, 2024 20:44
@lgirdwood
Copy link
Member

@cujomalainey good for you now ?

comp: Add initial Google CTC component
0f72a19 
Introduce a new component to perform crosstalk-cancellation on
the playback path.

Signed-off-by: Eddy Hsu <[email protected]>
@cujomalainey
Copy link
Contributor

I'll push submit tomorrow unless anyone else fines any issues.

@lgirdwood
Copy link
Member

I'll push submit tomorrow unless anyone else fines any issues.

I've rerun failing GH actions (now passed) and CI is showing expected results (this module not tested in CI to date). Pls merge when ready.

@cujomalainey cujomalainey merged commit b2521ea into thesofproject:main Aug 6, 2024
44 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marc-hb marc-hb marc-hb left review comments

@kv2019i kv2019i kv2019i approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@cujomalainey cujomalainey cujomalainey approved these changes

@andyross andyross andyross approved these changes

@lyakh lyakh lyakh left review comments

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@johnylin76 johnylin76 Awaiting requested review from johnylin76

@singalsu singalsu Awaiting requested review from singalsu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.11
Development

Successfully merging this pull request may close these issues.
9 participants
@eddy1021 @marc-hb @singalsu @lgirdwood @cujomalainey @lyakh @andyross @johnylin76 @kv2019i