Google aec sink src dp #8722

marcinszkudlinski · 2024-01-10T10:59:23Z

this is a replacement of #8469
all review changes from it have been included

This PR modifies google AEC shimm to use sink/src interface and to be run as DP module in 10ms period

This is upstream of AEC changes from 007 branch and:

Legacy implementation kept as google_rtc_audio_processing_ll.c, compiled when DP processing is disabled
changes required after merging Better handling of module base_config_ext data #8685 change

please ignore checkpatch warnings in google_rtc_audio_processing_ll.c, this file is - and should stay - 1:1 copy of legacy google_rtc_audio_processing.c.

lgirdwood · 2024-01-10T14:52:54Z

src/audio/google/CMakeLists.txt

@@ -13,7 +13,7 @@ if((NOT CONFIG_LIBRARY) OR CONFIG_LIBRARY_STATIC)
 	if(CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING)
 	  target_include_directories(sof PRIVATE ${CMAKE_SOURCE_DIR}/third_party/include)
 	  add_local_sources(sof
-		google_rtc_audio_processing.c
+		google_rtc_audio_processing_ll.c


Thinking aloud as this was done for other modules with IPC3/4 split, is there any common code where we can have a google_rtc_audio_processing_common.c ?

Actually there's really very little divergence needed. In the, heh, "other" patch, IPC3/4 deltas are limited to:

The set_config() method is handled differently for IPC4, and in a way that seems a little like needless boilerplate (this has never worked in main, so it's not clear to me why the changes happened). I haven't touched this yet but absolutely will clean it up.

A single workaround line remains to set the reference stream format (topology isn't getting to the pipeline correctly).

The single line "is this the reference or capture buffer?" predicate works differently (IPC3 checks pipeline IDs, IPC4 looks at the pin index instead, really those too are equivalent and I don't know why they diverged).

app/boards/intel_adsp_ace15_mtpm.conf

andyross

No objection in principle to merging this first, as long as there's a sincere commitment from Intel to consume and validate the other version too. If the only reason for merging this is to cut another "stable drop" branch that is cut off from SOF main, that seems like a bad trade. We need to agree on a single implementation and that takes work from both sides.

As far as code: it would be cleaner to avoid all the code motion (i.e. leave it all in one file, avoid the IPC3 removals), which really isn't needed to support the unified AEC and will only confuse the git history. (Edit: but that's not a -1, it's no harder to squash a big change than a small one, the only impact is to the pour soul trying to figure out what happened from the commit log)

andyross · 2024-01-10T15:09:30Z

src/audio/google/google_rtc_audio_processing.c

@@ -53,9 +53,7 @@ DECLARE_TR_CTX(google_rtc_audio_processing_tr, SOF_UUID(google_rtc_audio_process
 			   LOG_LEVEL_INFO);

 struct google_rtc_audio_processing_comp_data {
-#if CONFIG_IPC_MAJOR_4


Can we remove this "remove IPC3 support" patch, or just squash it? IPC3 support is going to be added right back in as soon as this merges, so it seems a little silly to go through the motions of turning it off.

Can we split this file into google_rtc_audio_processing_ipc3.c and google_rtc_audio_processing_ipc4.c like smart_amp ?

agree, I did not touch goole RTC part for ipc3/ipc4 split, because last quarter, a lot of patches are changing this module, if you need me to do this, we can do it later, this module indeed need a separation.

Let's do the ipc3/ipc4 split later as we have now so many PR in flight touching this code.

andyross · 2024-01-10T15:21:42Z

src/audio/google/CMakeLists.txt

@@ -13,7 +13,7 @@ if((NOT CONFIG_LIBRARY) OR CONFIG_LIBRARY_STATIC)
 	if(CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING)
 	  target_include_directories(sof PRIVATE ${CMAKE_SOURCE_DIR}/third_party/include)
 	  add_local_sources(sof
-		google_rtc_audio_processing.c
+		google_rtc_audio_processing_ll.c


Actually there's really very little divergence needed. In the, heh, "other" patch, IPC3/4 deltas are limited to:

The set_config() method is handled differently for IPC4, and in a way that seems a little like needless boilerplate (this has never worked in main, so it's not clear to me why the changes happened). I haven't touched this yet but absolutely will clean it up.

A single workaround line remains to set the reference stream format (topology isn't getting to the pipeline correctly).

The single line "is this the reference or capture buffer?" predicate works differently (IPC3 checks pipeline IDs, IPC4 looks at the pin index instead, really those too are equivalent and I don't know why they diverged).

andyross

If the only reason for merging this is to cut another "stable drop" branch that is cut off from SOF main, that seems like a bad trade. We need to agree on a single implementation and that takes work from both sides.

And... it turns out from sideband communication that this is exactly what is happening.
I think this has to be a -1 from me. I've been following the rules in #8571, addressing review comments, splitting out changes, chasing down workarounds and external/framework issues instead of just working around them inline. It's neither fair nor good engineering practice for Intel to ram this through just to be able to cut a release without having to work with me.

Can you guys please actually try the code in #8571 first before merging this without review? It works as-is, though I need to push up a respin without the separately-submitted changes. I'll get this out this afternoon. If it doesn't work, then report problems and let's work together on fixing them.

RanderWang · 2024-01-11T01:51:41Z

src/audio/google/google_rtc_audio_processing.c

@@ -53,9 +53,7 @@ DECLARE_TR_CTX(google_rtc_audio_processing_tr, SOF_UUID(google_rtc_audio_process
 			   LOG_LEVEL_INFO);

 struct google_rtc_audio_processing_comp_data {
-#if CONFIG_IPC_MAJOR_4


Can we split this file into google_rtc_audio_processing_ipc3.c and google_rtc_audio_processing_ipc4.c like smart_amp ?

RanderWang · 2024-01-11T02:06:14Z

src/audio/google/google_rtc_audio_processing.c

-		src = audio_stream_wrap(mic_stream, src);
-		dst = audio_stream_wrap(out_stream, dst);
+#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API
+	GoogleRtcAudioProcessingAnalyzeRender_float32(


we use different proc function for different format and select proc function based on module config
.e.g.

const struct drc_proc_fnmap drc_proc_fnmap[] = { /* { SOURCE_FORMAT , PROCESSING FUNCTION } */ #if CONFIG_FORMAT_S16LE { SOF_IPC_FRAME_S16_LE, drc_s16_default }, #endif /* CONFIG_FORMAT_S16LE */ #if CONFIG_FORMAT_S24LE { SOF_IPC_FRAME_S24_4LE, drc_s24_default }, #endif /* CONFIG_FORMAT_S24LE */ #if CONFIG_FORMAT_S32LE { SOF_IPC_FRAME_S32_LE, drc_s32_default }, #endif /* CONFIG_FORMAT_S32LE */ };

this is a separate option - just use google's API 16 or 32. It is independent of out CONFIG_FORMAT_xxxx flags.
In fact, AEC 16 and 32 bit API differ a bit more than just number of bits - they also accept diffrent set of parameters:

int GoogleRtcAudioProcessingAnalyzeRender_float32( GoogleRtcAudioProcessingState *const state, const float *const *src); int GoogleRtcAudioProcessingAnalyzeRender_int16( GoogleRtcAudioProcessingState *const state, const int16_t *const src);

const float *const *src vs const float *const src - a pointer to a table of pointers to channels or just a pointer to first channel

FWIW, this is basically the architecture in #8571 (though it's selected with a branch and isn't table driven), there are a bunch of function pointers for doing the optimized sink/source de/interleave+convert+copy for each format variant, without any cut/paste of algorithm code. See https://github.com/andyross/sof/blob/mt8195-aec/src/audio/google/google_rtc_audio_processing.c#L112

marcinszkudlinski · 2024-01-11T07:57:54Z

why _LL ?

because IPC4 != DP_scheduling

this PR does:

LL version for IPC3 and IPC4
DP version (IPC4 only - no choice here)

mwasko · 2024-01-11T08:55:32Z

No objection in principle to merging this first, as long as there's a sincere commitment from Intel to consume and validate the other version too.

@andyross the purpose of this change is to enable AEC module with DP Scheduler using IPC4. Since Intel has already transition to IPC4 then we have no plan to enable and validate this feature with IPC3. However, there are no objections to follow up with IPC3 support as a next step, but the validation and enabling will have to be done on your end.

As far as code: it would be cleaner to avoid all the code motion (i.e. leave it all in one file, avoid the IPC3 removals), which really isn't needed to support the unified AEC and will only confuse the git history. (Edit: but that's not a -1, it's no harder to squash a big change than a small one, the only impact is to the pour soul trying to figure out what happened from the commit log)

I talked with @marcinszkudlinski and "Remove IPC3 support" is unfortunate naming and should be squashed, because the intention here is not to remove any IPC3 functionality, but rather limit the new feature to IPC4 (at least in this PR).

andyross · 2024-01-11T15:16:01Z

Since Intel has already transition to IPC4 then we have no plan to enable and validate this feature with IPC3

This isn't an Intel project and SOF isn't an Intel deliverable though. I understand you don't want to support IPC3 anymore (I don't either, FWIW), but we still have devices shipping with it and they need to run with current code still. Which is why #8571 exists: it has all the features this PR does, plus IPC3 support, plus dynamic stream format handling, it's smaller, faster, and it does it all in a single source file without having to copy and paste all the future changes between this and a "legacy" version. If there are features you want to see there, just add review comments and let's work together on fixing them.

I absolutely understand there are business worries about what software which entity is willing to support. And that may have to be dealt with out of band. I'll remove the -1 here since it's not really code-related and replace it with a proper review trying to point out the collisions and how it compares with the competing PR.

andyross · 2024-01-11T15:25:16Z

Can we split this file into google_rtc_audio_processing_ipc3.c and google_rtc_audio_processing_ipc4.c like smart_amp ?

To repeat, there's really just not that much divergence. The only major code block that changes is that the control handling has a rather different API in IPC4 (this was pre-existing, and absolutely can be cleaned up a ton). Beyond that, there's some sink/source validation code that obviously doesn't apply to IPC3 and a 1-line workaround for the broken reference channel count (which is a protocol bug really, will submit a proposed fix once we get things merged).

Basically see here and scan for the use of IPC_MAJOR: https://github.com/andyross/sof/blob/mt8195-aec/src/audio/google/google_rtc_audio_processing.c

marcinszkudlinski · 2024-01-11T19:25:14Z

...so here's a merge IPC3+IPC4
IPC4 works with DP scheduling only

mwasko · 2024-01-12T09:06:02Z

This isn't an Intel project and SOF isn't an Intel deliverable though. I understand you don't want to support IPC3 anymore (I don't either, FWIW), but we still have devices shipping with it and they need to run with current code still.

@andyross you are completely right and because of that you cannot expect Intel to enable and validate all possible configurations that are supported in SOF. This change introduce DP scheduling support for IPC4 because this is the configuration we can validate and guarantee the quality of it.

andyross · 2024-01-12T16:02:04Z

you cannot expect Intel to enable and validate all possible configurations that are supported in SOF.

Of course not. I just want you to review and approve a PR. If it doesn't work on your supported platform, provide comments and test results or whatever and we'll work together to fix it. I'll handle the other stuff.

marcinszkudlinski · 2024-01-24T09:27:59Z

@andyross @lkoenig @marc-hb all requests addressed, pls proceed

mwasko · 2024-01-29T09:06:33Z

@lkoenig, @andyross your change requests has been addressed, please update your review so we can move forward with this PR.

lkoenig · 2024-01-29T16:40:15Z

Sorry I was on vacation. Will have a look tomorrow.

src/audio/google/google_rtc_audio_processing.c

lkoenig · 2024-02-02T07:57:59Z

I think we are getting there. Any update ?

lkoenig · 2024-02-02T07:58:38Z

Sorry I clicked the wrong button.

This commit changes AEC to use sink/src interface and makes it to be a DP module it also enables optional usage of 32bit AEC API Signed-off-by: Marcin Szkudlinski <[email protected]>

Signed-off-by: Marcin Szkudlinski <[email protected]>

marcinszkudlinski · 2024-02-05T13:25:20Z

I was on medical leave, please proceed

marcinszkudlinski · 2024-02-07T16:23:06Z

@andyross ?

andyross

Sorry, thought I was already +1. Fine with me, though not looking forward to having to piecewise-rework the other PR.

lgirdwood · 2024-02-08T09:52:35Z

Sorry, thought I was already +1. Fine with me, though not looking forward to having to piecewise-rework the other PR.

Thanks @andyross - we can plan this better next time, a lot of moving parts this time which increased effort/complexity.

lgirdwood · 2024-02-08T09:54:15Z

SOFCI TEST

lgirdwood · 2024-02-08T09:54:52Z

Re run the CI build check. CI went down on Monday so manually restart.

Andy has replied that his comments are now addressed and gives a +1 now..

marc-hb · 2024-08-23T22:34:15Z

app/boards/intel_adsp_ace15_mtpm.conf

@@ -92,3 +92,6 @@ CONFIG_DEBUG_COREDUMP_MEMORY_DUMP_MIN=y
 CONFIG_PROBE=y
 CONFIG_PROBE_DMA_MAX=2
 CONFIG_LOG_TIMESTAMP_64BIT=y
+
+CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING=y
+CONFIG_COMP_STUBS=y


?

From src/audio/Kconfig
"CONFIG_STUBS: This should only be used in testing environments like fuzzers or CI."

Good example of #9386

cc: @cujomalainey

Now being reverted in:

app: boards: intel_adsp_ace15_mtpm: disable mock components #9410

#9410 results and discussion seem to show a test seems to depend on this!

CONFIG_COMP_STUBS=y was enabled in thesofproject#8722 / commit 8e34109 ("AEC: Enable Google AEC with Mock compliation"). CONFIG_COMP_STUBS indirectly enables CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK which was the desired effect. However it also automatically and silently "mocks" all other 3rd party modules which is not desirable. So, replace it with the more focused `CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK`. `src/audio/Kconfig` says "CONFIG_STUBS: This should only be used in testing environments like fuzzers or CI." It's not clear why official sof-bin releases should include `google_rtc_audio_processing_mock.c` but that's another topic for another day. build-mtl/zephyr.strip is identical before versus after this commit. Signed-off-by: Marc Herbert <[email protected]>

CONFIG_COMP_STUBS=y was enabled in thesofproject#8722 / commit 8e34109 ("AEC: Enable Google AEC with Mock compliation"). CONFIG_COMP_STUBS indirectly enables CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK which was the desired effect. However it also automatically and silently "mocks" all other 3rd party modules which is not desirable. So, replace it with the more focused `CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK`. `src/audio/Kconfig` says "CONFIG_STUBS: This should only be used in testing environments like fuzzers or CI." Official sof-bin releases include `google_rtc_audio_processing_mock.c` because the CI that uses it can't use extra CONFIGS. That's another topic for another day, see thesofproject#9410. build-mtl/zephyr.strip is identical before versus after this commit. Signed-off-by: Marc Herbert <[email protected]>

CONFIG_COMP_STUBS=y was enabled in #8722 / commit 8e34109 ("AEC: Enable Google AEC with Mock compliation"). CONFIG_COMP_STUBS indirectly enables CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK which was the desired effect. However it also automatically and silently "mocks" all other 3rd party modules which is not desirable. So, replace it with the more focused `CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK`. `src/audio/Kconfig` says "CONFIG_STUBS: This should only be used in testing environments like fuzzers or CI." Official sof-bin releases include `google_rtc_audio_processing_mock.c` because the CI that uses it can't use extra CONFIGS. That's another topic for another day, see #9410. build-mtl/zephyr.strip is identical before versus after this commit. Signed-off-by: Marc Herbert <[email protected]>

marcinszkudlinski force-pushed the aec-on-main branch 3 times, most recently from 422946e to 878a4da Compare January 10, 2024 12:03

marcinszkudlinski marked this pull request as ready for review January 10, 2024 12:03

marcinszkudlinski requested review from marc-hb, a team, RanderWang, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners January 10, 2024 12:03

marcinszkudlinski requested a review from andyross January 10, 2024 12:12

lgirdwood reviewed Jan 10, 2024

View reviewed changes

andyross reviewed Jan 10, 2024

View reviewed changes

andyross previously requested changes Jan 10, 2024

View reviewed changes

andyross mentioned this pull request Jan 10, 2024

Google AEC rework: unbreak/resurrect, mt8195, DP integration #8571

Closed

RanderWang reviewed Jan 11, 2024

View reviewed changes

andyross self-requested a review January 11, 2024 15:16

marcinszkudlinski force-pushed the aec-on-main branch from 878a4da to c803c46 Compare January 11, 2024 19:24

marc-hb mentioned this pull request Jan 11, 2024

[BUG] <wrn> runtime_suspend: Unbalanced suspend; <err> host_trigger(): dma stop failed: -120 in pause-resume test #8729

Closed

marcinszkudlinski force-pushed the aec-on-main branch from 900b4f5 to f060280 Compare January 24, 2024 07:36

abonislawski approved these changes Jan 29, 2024

View reviewed changes

lkoenig reviewed Jan 29, 2024

View reviewed changes

lkoenig closed this Feb 2, 2024

lkoenig reopened this Feb 2, 2024

marcinszkudlinski added 2 commits February 5, 2024 13:48

AEC: convert aec to use sink/src interface

f199670

This commit changes AEC to use sink/src interface and makes it to be a DP module it also enables optional usage of 32bit AEC API Signed-off-by: Marcin Szkudlinski <[email protected]>

AEC: Enable Google AEC with Mock compliation

37c5a26

Signed-off-by: Marcin Szkudlinski <[email protected]>

marcinszkudlinski force-pushed the aec-on-main branch from f060280 to 37c5a26 Compare February 5, 2024 12:48

lkoenig approved these changes Feb 5, 2024

View reviewed changes

andyross reviewed Feb 7, 2024

View reviewed changes

lgirdwood approved these changes Feb 8, 2024

View reviewed changes

lgirdwood merged commit 8e34109 into thesofproject:main Feb 8, 2024
36 of 44 checks passed

marcinszkudlinski deleted the aec-on-main branch February 8, 2024 11:50

marc-hb reviewed Aug 23, 2024

View reviewed changes

This was referenced Aug 24, 2024

intel_adsp_ace15_mtpm.conf: replace blanket _STUBS with focused RTC MOCK #9404

Merged

app: boards: intel_adsp_ace15_mtpm: disable mock components #9410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google aec sink src dp #8722

Google aec sink src dp #8722

marcinszkudlinski commented Jan 10, 2024 •

edited

Loading

lgirdwood Jan 10, 2024

andyross Jan 10, 2024

andyross left a comment •

edited

Loading

andyross Jan 10, 2024 •

edited

Loading

RanderWang Jan 11, 2024

btian1 Jan 15, 2024

kv2019i Jan 15, 2024

andyross Jan 10, 2024

andyross left a comment

RanderWang Jan 11, 2024

RanderWang Jan 11, 2024

marcinszkudlinski Jan 11, 2024 •

edited

Loading

andyross Jan 11, 2024

marcinszkudlinski commented Jan 11, 2024

mwasko commented Jan 11, 2024

andyross commented Jan 11, 2024

andyross commented Jan 11, 2024

marcinszkudlinski commented Jan 11, 2024

mwasko commented Jan 12, 2024

andyross commented Jan 12, 2024

marcinszkudlinski commented Jan 24, 2024 •

edited

Loading

mwasko commented Jan 29, 2024

lkoenig commented Jan 29, 2024

lkoenig commented Feb 2, 2024

lkoenig commented Feb 2, 2024

marcinszkudlinski commented Feb 5, 2024

marcinszkudlinski commented Feb 7, 2024

andyross left a comment

lgirdwood commented Feb 8, 2024

lgirdwood commented Feb 8, 2024

lgirdwood commented Feb 8, 2024

marc-hb Aug 23, 2024

marc-hb Aug 27, 2024

marc-hb Sep 2, 2024

Google aec sink src dp #8722

Google aec sink src dp #8722

Conversation

marcinszkudlinski commented Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyross left a comment • edited Loading

Choose a reason for hiding this comment

andyross Jan 10, 2024 • edited Loading

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

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski commented Jan 11, 2024

mwasko commented Jan 11, 2024

andyross commented Jan 11, 2024

andyross commented Jan 11, 2024

marcinszkudlinski commented Jan 11, 2024

mwasko commented Jan 12, 2024

andyross commented Jan 12, 2024

marcinszkudlinski commented Jan 24, 2024 • edited Loading

mwasko commented Jan 29, 2024

lkoenig commented Jan 29, 2024

lkoenig commented Feb 2, 2024

lkoenig commented Feb 2, 2024

marcinszkudlinski commented Feb 5, 2024

marcinszkudlinski commented Feb 7, 2024

andyross left a comment

Choose a reason for hiding this comment

lgirdwood commented Feb 8, 2024

lgirdwood commented Feb 8, 2024

lgirdwood commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski commented Jan 10, 2024 •

edited

Loading

andyross left a comment •

edited

Loading

andyross Jan 10, 2024 •

edited

Loading

marcinszkudlinski Jan 11, 2024 •

edited

Loading

marcinszkudlinski commented Jan 24, 2024 •

edited

Loading