-
Notifications
You must be signed in to change notification settings - Fork 349
comp: Add initial Google AEC #5149
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
comp: Add initial Google AEC #5149
Conversation
Introduce a new component to perform acoustic echo cancellation on capture path using a buffer from the playback as a reference. 1. Put the google_rtc_audio_processing at $SOF_REPO/third_party_libraries - libgoogle_rtc_audio_processing.a - libgoogle_rtc_audio_processing_tuning.a - libc++.a (Corresponding stdlibc++) - libc++abi.a 2. Put the header in $SOF_REPO/third_party_includes - google_rtc_audio_processing.h 3. Build firmware and tool with xcc 4. To verify it works: - aplay some speech - At the same time arecord the mic which uses AEC The mic signal should not exhibit any echo from the playback. Signed-off-by: Lionel Koenig <lionelk@google.com>
This adds a mock to be able to build with the google_rtc_audio_processing component. Signed-off-by: Lionel Koenig <lionelk@google.com>
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment on what is different in this PR to the original PR (just so we reviewers can follow the changes). Thanks.
| s->num_output_channels = 1; | ||
| s->num_frames = GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ * 10 / 1000; | ||
| s->aec_reference = rballoc(0, | ||
| SOF_MEM_CAPS_RAM, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation looks wrong here, could be github though ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I haven't found a way to get this one right. I am open to any suggestion.
Ignore - just noticed this is for a different branch. |
The differences with the original PR are:
|
|
@mwasko I dont think all the CI works on the production branches, but feel free to merge when you are ready. |
|
@lkoenig we need to hit adl-003 as well with this series |
|
@cujomalainey This is in the pipe. |
| cd->raw_mic_buffer[cd->raw_mic_buffer_index] = *src; | ||
| ++cd->raw_mic_buffer_index; | ||
|
|
||
| dst = audio_stream_read_frag_s16(&cd->output->stream, output_buff_frag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lkoenig Hi, is this intentional or mistake that sink buffer is written with reference to r_ptr with the use function? Normally sink write is done with audio_stream_write_frag_s16() that references to w_ptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is an error. Thanks for catching it. I'll fix it straight away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am working on a patch for this component to deprecate read/write frag from SOF but it's good to have a correct reference, so please make the fix. Thanks!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5576 should correct it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you want me to test the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singalsu Let me know if you need help testing the read/write change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lkoenig Nice, thank you! I'm able to run the mockup version in testbench and in a HDA generic device with a custom topology. But I really need your check and approve before it can be merged. I'll share in PR the MCPS saving that I see in my TGL-H.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lkoenig There's similar issue in Crossover. @andrula-song may be working on a fix. We are addressing rest of read/write frag together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whelp, that is two I let slip through in my reviews, thanks @andrula-song and @singalsu !
This backports Google Initial AEC to latest tigerlake branch
Original PR: #5090