Audio: Convert Google RTC audio processing to module API #8258

singalsu · 2023-09-25T15:59:56Z

Fist commit is module adapter conversion

Second commit is improvement to mockup to mix and saturate all available matching channels.

Third commit adds build and load to testbench.

Related to, no more dependent of:

S32 support for AEC #8237

lgirdwood

@singalsu good stuff !
@johnylin76 @cujomalainey fyi - pls review.

singalsu · 2023-09-26T15:02:36Z

@cujomalainey This is the first version that works with created test topology in sof-nocodec style, sharing it soon as well. I think it's best to get #8237 merged first, then I'll rebase this.

singalsu · 2023-09-27T08:00:50Z

Fixed conflict with 7112246 ("google_rtc_audio_processing: remove buffer_acquire")

lyakh · 2023-09-27T10:28:41Z

src/audio/google/google_rtc_audio_processing.c

+	 * The config blob is not referenced after reconfigure() returns
+	 * so it is safe to call comp_get_data_blob here which frees the
+	 * old blob. This assumes cmd() and prepare()/copy() cannot run
+	 * concurrently which is the case when there is no preemption.


in principle .copy() can run concurrently with IPCs, in which case the timer interrupt can interrupt IPC processing, then the scheduler will choose between the IPC thread and the LL scheduler thread and will pick up the latter, since it has a higher priority. But if the component has only one sink and one source, mostly it's only the "stop" IPC that can arrive while the stream is already running, or can some configuration be sent to a running stream too?

I preserved this part as close to the original so this should not change the operation. @lkoenig What do you think of this?

Configurations can be sent when a stream is running.

@lyakh @lkoenig I think it depends on the (unknown) internals of the processing whether new config data blob during copy() is a hazard or not. The open-source mock-up version ignores the blob content totally.

The reconfigure set here should ensure that the next copy() correctly re-initializes for the blob. Please let me know if this needs to be addressed now, I could propose a fix. Or (preferably) let @thesofproject/google address this in successive PR.

The data should be whole and not altered during the call of GoogleRtcAudioProcessingReconfigure and GoogleRtcAudioProcessingParseSofConfigMessage
As of today we do not have concurrent access protection enabled in the GoogleRtcAudioProcessing.

@singalsu at least I would note in the comment, that a race is possible and should be fixed or at least further investigated

For DP processing there are more possibilities of race, especially if the module runs on secondary core. Processing data in this case is not synchronized with IPC set config in any way and may be even executed at the same time on 2 separate cores

RanderWang · 2023-09-28T05:21:05Z

@singalsu How do you test it ? it is used by chrome so it is better to sync with chrome team.

singalsu · 2023-09-28T15:24:15Z

@singalsu How do you test it ? it is used by chrome so it is better to sync with chrome team.

I tested this in UPX nocodec mode with new very simple topology in PR #8266. Testing with Chrome sounds like a good idea but I can't do it myself. I'll see if I can get help from them to test this.

yongzhi1 · 2023-09-29T01:26:20Z

Sanity checked the patch along with thesofproject/linux#4550 and #8101, #8246, #8204, no regression found with AEC mock module.

cujomalainey · 2023-09-29T18:43:08Z

Apologies for the delay here, I will look on monday, been a crazy week

lgirdwood · 2023-10-02T14:02:38Z

@singalsu still draft ?

singalsu · 2023-10-02T15:00:48Z

@singalsu still draft ?

I think it's best to wait for merge of #8237 that is a larger change.

ranj063 · 2023-10-04T18:07:37Z

src/audio/google/google_rtc_audio_processing_mock.c

+	int n, m;
+
+	channels = MIN(state->num_capture_channels, state->num_aec_reference_channels);
+	channels = MIN(state->num_output_channels, channels);


@singalsu this works for the 2ch case where the PDM 0 is configured on the board but if PDM 1 is connected, this won't work correctly right?

If reference is 2ch and mic & out 4ch then the input and reference would be mixed to first two channels. You are right that mic channels 3 and 4 would not be copied to output. I think at least those should be copied.

Would we wrap reference channels to first if there's less than in output, or just copy mic to output for non-matching channels?

in an ideal world, we would reduce our channels to only the ones we care about so we don't need these silly configs and only have real channels in the data.

I changed it to version where every output channel has a mix of mic and reference. Hope it's good for testing now.

src/audio/google/google_rtc_audio_processing.c

btian1 · 2023-10-09T08:51:15Z

src/audio/google/google_rtc_audio_processing_mock.c


 	memset(dest, 0, sizeof(int16_t) * state->num_output_channels * state->num_frames);
 	for (n = 0; n < state->num_frames; ++n) {
-		*out = *mic + *ref;
+		for (m = 0; m < channels; m++)


is it a bug? since previous code does not have channels loop.

The original code mixed only to first channel, with this loop we can mix to other output channels. But according to Ranjani's comment we should decide what to do with non-matching channels counts.

cujomalainey · 2023-10-09T14:39:01Z

@singalsu still draft ?

I think it's best to wait for merge of #8237 that is a larger change.

Let's go with this first since mine won't work without yours unless I update the mock

cujomalainey · 2023-10-09T19:14:13Z

src/arch/host/configs/library_defconfig

@@ -16,6 +17,7 @@ CONFIG_COMP_VOLUME=y
 CONFIG_COMP_VOLUME_LINEAR_RAMP=y
 CONFIG_COMP_VOLUME_WINDOWS_FADE=y
 CONFIG_DEBUG_MEMORY_USAGE_SCAN=n
+CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK=y


you can just enable the stub config

Thanks, helps with two other conversions also, done!

marcinszkudlinski · 2023-10-10T06:35:38Z

src/audio/google/google_rtc_audio_processing.c

 }

-static int google_rtc_audio_processing_prepare(struct comp_dev *dev)
+static int google_rtc_audio_processing_prepare(struct processing_module *mod,
+					       struct sof_source __sparse_cache **sources,


__sparse_cache have been depreciated and removed

Good catch, thanks!

marcinszkudlinski · 2023-10-10T06:40:04Z

src/audio/google/google_rtc_audio_processing.c

+	 * The config blob is not referenced after reconfigure() returns
+	 * so it is safe to call comp_get_data_blob here which frees the
+	 * old blob. This assumes cmd() and prepare()/copy() cannot run
+	 * concurrently which is the case when there is no preemption.


For DP processing there are more possibilities of race, especially if the module runs on secondary core. Processing data in this case is not synchronized with IPC set config in any way and may be even executed at the same time on 2 separate cores

src/audio/google/google_rtc_audio_processing.c

cujomalainey · 2023-10-10T22:55:53Z

src/audio/google/google_rtc_audio_processing.c

 #if CONFIG_IPC_MAJOR_4
 		if (IPC4_SINK_QUEUE_ID(source->id) == SOF_AEC_FEEDBACK_QUEUE_ID) {
 #else
 		if (source->source->pipeline->pipeline_id != dev->pipeline->pipeline_id) {
 #endif
-			cd->aec_reference = source;


I am curious why you are opting for the index over the pointer

It's because module adapter provides the input and ouput streams as arrays of pointers to process(). I use the index to access the right stream. This also eases the transition to source/sink api in pipeline2.0 with better abstraction where process() receives array of sof_source and sof_sink pointers. It let's us optimize further the pipeline buffers. I'm not sure keeping the old way would prevent that but I felt this is cleaner approach.

What do you think @marcinszkudlinski , was this the right thing to do?

cujomalainey · 2023-10-10T22:57:05Z

src/audio/google/google_rtc_audio_processing.c

 	}
-	comp_update_buffer_consume(buffer, num_aec_reference_bytes);
+	input_buffers[cd->aec_reference_source].consumed = num_aec_reference_bytes;


is this a hack for the same side effect as module_update_buffer_position in which case i think we should fix the api

Yes, not very clean API -- frames as input and bytes as output. But as far as I understand the design is from reference firmware. Did you mean a similar helper function but for this multiple sources/sinks case? This component will be converted to source/sink API after this PR for DP mode so we could review and possibly make improvements to api there.

FYI @marcinszkudlinski

Yes, i think breaking into this struct is probably not the safest thing for API purposes on the buffer, we should fix the api or make a new call.

src/arch/host/configs/library_defconfig

singalsu · 2023-10-11T09:55:35Z

Marked this as DNM - I'm not sure if this PR step is necessary in converting this to DP. But please continue reviewing and sharing your thoughts so the next step can be better.

lgirdwood · 2023-10-16T15:41:08Z

Marked this as DNM - I'm not sure if this PR step is necessary in converting this to DP. But please continue reviewing and sharing your thoughts so the next step can be better.

All modules should use the module API as component API is deprecated and will gradually be removed/refactored.i.e. we only have 1 API for modules.

marcinszkudlinski · 2023-10-20T06:44:09Z

Please proceed with merging
I'm finishing with changes to sink/src interface and DP processing of the module, all my changes have been based on top of this PR
So pls proceed and merge it once ready

singalsu · 2023-10-20T08:01:47Z

@cujomalainey Is this version good for merge? It will be a short time version only. Work by @marcinszkudlinski for DP schduling is now done and needs this patch as base.

lkoenig

The change looks good: I haven't look into too much details.
Did you run it with the mock an d got good audio?

lkoenig

Nice change so we can some at least build coverage.

src/arch/host/configs/library_defconfig

lkoenig · 2023-10-20T09:56:17Z

src/audio/google/google_rtc_audio_processing_mock.c

+		ir = 0;
+		for (io = 0; io < state->num_output_channels; io++) {
+			out[io] = sat_int16((int32_t)mic[im++] + ref[ir++]);
+			if (im == state->num_capture_channels)


You might mix several time same channel like is output is 2 and input is one channel you'll get 2 times the first channel.

out[io] = sat_int16( (im < state->num_capture_channels ? (int32_t)mic[im++] : 0) + (ir < state->num_reference_channles ? ref[ir++] : 0) );

Might be more appropriate (sorry for the vey poor codestyle).

We don't really know what's best for testing with mock-up. Sure, I can update it to be like this.

@lkoenig Thanks, I included your suggestion as such, it works well!

singalsu · 2023-10-20T10:13:04Z

The change looks good: I haven't look into too much details. Did you run it with the mock an d got good audio?

Yep, I've listened without glitches heard a good bit of output music from mic and reference inputs feed. with and without mixing, and test topology suitable for UPX board to use this. I have not been able to test bytes control with mock other than it is received.

I've changed from menu config the channels to 2 to hear both stereo channels from mockup mix.

cujomalainey · 2023-10-21T19:52:25Z

Sorry been busy internally, I trust @lkoenig 's review

This patch converts the component to module API. The changes are: - google_rtc_audio_processing_params() is changed to update sources and sink params from IPC4 init IPC. It's not used for IPC3 where module adapter handles params(). - The commands handling is fixed to handle via module adapter the IPC3 and IPC4 commands. - The google_rtc_audio_processing_init() is updated to simpler module adapter client way. - The google_rtc_audio_processing_prepare() is updated to set buffers parameters for IPC4 and find sources buffers for microphone and reference by index instead of buffer pointer. That simplifies the processing function with module adapter. - The google_rtc_audio_processing_process() is updated from copy() to module adapter client way. - google_rtc_audio_processing_get_attribute() is removed - As overall change dev handle is changed to mod handle in nearly all functions. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

With this change e.g. in all 2ch case all output channels are mixed with matching microphone and reference channels. It helps with testing to hear and see that the mockup works. The mix is done for matching microphone, reference and output channels indices. With e.g. less reference channels, the remaining output channels are passed directly from microphone. The mixed samples are saturated to avoid nasty sounding overflows. Signed-off-by: Lionel Koenig <lionelk@google.com> Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch allows to load and run it in testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

lgirdwood reviewed Sep 26, 2023

View reviewed changes

singalsu force-pushed the grtc_module_convert branch from 054d298 to 55f587d Compare September 26, 2023 14:58

singalsu requested a review from RanderWang September 27, 2023 07:32

singalsu force-pushed the grtc_module_convert branch from 55f587d to 752aae6 Compare September 27, 2023 07:58

lyakh reviewed Sep 27, 2023

View reviewed changes

RanderWang requested a review from cujomalainey September 28, 2023 05:08

lgirdwood approved these changes Sep 29, 2023

View reviewed changes

ranj063 reviewed Oct 4, 2023

View reviewed changes

btian1 reviewed Oct 9, 2023

View reviewed changes

cujomalainey mentioned this pull request Oct 9, 2023

S32 support for AEC #8237

Closed

singalsu marked this pull request as ready for review October 9, 2023 14:50

singalsu requested review from a team, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners October 9, 2023 14:50

cujomalainey reviewed Oct 9, 2023

View reviewed changes

marcinszkudlinski reviewed Oct 10, 2023

View reviewed changes

singalsu force-pushed the grtc_module_convert branch from 752aae6 to 90e67ac Compare October 10, 2023 10:10

singalsu requested a review from lkoenig October 10, 2023 10:11

singalsu requested review from cujomalainey, ranj063, btian1 and marcinszkudlinski October 10, 2023 10:11

cujomalainey reviewed Oct 10, 2023

View reviewed changes

btian1 reviewed Oct 11, 2023

View reviewed changes

src/arch/host/configs/library_defconfig Show resolved Hide resolved

singalsu changed the title ~~Audio: Convert Google RTC audio processing to module API~~ [DNM] Audio: Convert Google RTC audio processing to module API Oct 11, 2023

singalsu changed the title ~~[DNM] Audio: Convert Google RTC audio processing to module API~~ Audio: Convert Google RTC audio processing to module API Oct 20, 2023

singalsu requested review from cujomalainey and btian1 October 20, 2023 07:58

lkoenig approved these changes Oct 20, 2023

View reviewed changes

src/arch/host/configs/library_defconfig Show resolved Hide resolved

lkoenig requested changes Oct 20, 2023

View reviewed changes

singalsu force-pushed the grtc_module_convert branch from 90e67ac to 858614c Compare October 20, 2023 10:46

singalsu requested a review from lkoenig October 20, 2023 10:48

lkoenig approved these changes Oct 20, 2023

View reviewed changes

singalsu added 3 commits October 23, 2023 10:38

Tools: Testbench: Add Google RTC with mockup to testbench

30e323a

This patch allows to load and run it in testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the grtc_module_convert branch from 858614c to 30e323a Compare October 23, 2023 07:39

lyakh approved these changes Oct 23, 2023

View reviewed changes

lgirdwood approved these changes Oct 23, 2023

View reviewed changes

cujomalainey approved these changes Oct 23, 2023

View reviewed changes

cujomalainey merged commit 3d040c7 into thesofproject:main Oct 23, 2023
40 of 42 checks passed

Audio: Convert Google RTC audio processing to module API #8258

Audio: Convert Google RTC audio processing to module API #8258

Conversation

singalsu commented Sep 25, 2023 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

singalsu commented Sep 26, 2023

singalsu commented Sep 27, 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

RanderWang commented Sep 28, 2023

singalsu commented Sep 28, 2023 • edited Loading

yongzhi1 commented Sep 29, 2023

cujomalainey commented Sep 29, 2023

lgirdwood commented Oct 2, 2023

singalsu commented Oct 2, 2023

ranj063 Oct 4, 2023 • 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

Choose a reason for hiding this comment

cujomalainey commented Oct 9, 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

singalsu commented Oct 11, 2023

lgirdwood commented Oct 16, 2023

marcinszkudlinski commented Oct 20, 2023

singalsu commented Oct 20, 2023

lkoenig left a comment

Choose a reason for hiding this comment

lkoenig 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

singalsu commented Oct 20, 2023 • edited Loading

cujomalainey commented Oct 21, 2023

singalsu commented Sep 25, 2023 •

edited

Loading

singalsu commented Sep 28, 2023 •

edited

Loading

ranj063 Oct 4, 2023 •

edited

Loading

singalsu commented Oct 20, 2023 •

edited

Loading