Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804223002:
Adding support for handling highly reverberant echoes in AEC3 (Closed)
|
Created:
3 years, 8 months ago by peah-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding support for handling highly reverberant echoes in AEC3.
This CL adds support for handling highly reverberant echoes in
AEC3. The functionality is hardcoded to be have no effect (via
a decay factor of 0), but this CL will be followed by other CLs
for which nonzero decay factors will be allowed.
Apart from this change, this CL also refactors the residual
echo estimation code to make it shorter and more readable.
The changes introduced herein are bitexact (for a decay factor
of 0).
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2804223002
Cr-Commit-Position: refs/heads/master@{#17589}
Committed: https://chromium.googlesource.com/external/webrtc/+/89420459d1d4d02e89f36241c1046407773ed9ff
Patch Set 1 #
Total comments: 14
Patch Set 2 : Changes in response to reviewer comments #Patch Set 3 : Added limiting of the delay size #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Bitexact Bitexact Bitexact Bitexact Bitexact Bitexact Bitexact Bitexact BUG= ========== to ========== Adding support for handling highly reverberant echoes in AEC3. This CL adds support for handling highly reverberant echoes in AEC3. The functionality is hardcoded to be have no effect (via a decay factor of 0), but this CL will be followed by other CLs for which nonzero decay factors will be allowed. Apart from this change, this CL also refactors the residual echo estimation code to make it shorter and more readable. The changes introduced herein are bitexact (for a decay factor of 0). BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org
Hi, This is a new CL that adds AEC3 support for highly reverberant echo environments. PTAL
peah@webrtc.org changed reviewers: + alessiob@webrtc.org
Adding alessiob@ as a reviewer.
Adding alessiob@ as a reviewer.
lgtm Thanks for having added me as reviewer, great chance to familiarize with AEC3. I only have a few minor comments. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:69: headset_detected_cached_ = false; It's for sure already ok as it is, but double check if you need to take action (e.g., call Reset()) if headset_detected_cached_ is true. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:94: // Computes the spectral power over that blocks surrounding the delay. Either "that block" or "those blocks" - I guess the latter since you refer to an interval in render_buffer. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:97: std::min(kResidualEchoPowerRenderWindowSize - 1, delay_use + 1), &X2); I'm not sure, but maybe you want to DCHECK (kResidualEchoPowerRenderWindowSize - 1 >= delay_use + 1 || kResidualEchoPowerRenderWindowSize - 1 >= delay_use - 1) https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.h (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:42: // Estimates the residual echo power based on the erle and the linear power "erle" stands for Echo Return Loss Enhancement, right? Since it's the first time you use the acronym in this file, I would write the full name and then "(erle)" - it also helps to understand the role of the variable in the LinearEstimate signature. As an AEC newbie, that would help me to better follow the signal processing pipeline. However, if you think it should be obvious, just ignore this comment.
lgtm with small nit! https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:94: // Computes the spectral power over that blocks surrounding the delay. that -> the https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:106: delay ? *delay : kAdaptiveFilterLength, Please replace ternary op expression with 'delay.value_or(kAdaptiveFilterLength)'. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:113: R2->fill((*std::max_element(R2->begin(), R2->end())) * 100.f); Should the '100.f' also be kFixedEchoPathGain'?
LGTM with one question. https://codereview.webrtc.org/2804223002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:171: auto integer_power = [](float base, int exp) { Is this really more efficient than powf? Is exp generally a small number?
Thanks for the comments! I will address them all. Note that I accidentally uploaded a patch which belonged to another CL (patch 2). I will therefore delete that patch. That patch did, however, not affect any of the other code in this CL, so that should not affect the review. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:69: headset_detected_cached_ = false; On 2017/04/07 09:34:39, AleBzk wrote: > It's for sure already ok as it is, but double check if you need to take action > (e.g., call Reset()) if headset_detected_cached_ is true. The intention of caching this is that otherwise I would for each block need to do a Reset when a headset is detected. As the reset is only needed for the first block when a headset is detected, my feeling was that that was such waste of computations that it motivated extra functionality so that this only happens at the first block when a headset is detected.
https://codereview.webrtc.org/2804223002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:171: auto integer_power = [](float base, int exp) { On 2017/04/07 11:43:52, ivoc wrote: > Is this really more efficient than powf? Is exp generally a small number? It is a bit unclear, sources seem to state both ways. The key thing is that exp is an int, in contrast to powf where it is a float.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:94: // Computes the spectral power over that blocks surrounding the delay. On 2017/04/07 09:34:39, AleBzk wrote: > Either "that block" or "those blocks" - I guess the latter since you refer to an > interval in render_buffer. Done. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:94: // Computes the spectral power over that blocks surrounding the delay. On 2017/04/07 11:08:57, aleloi wrote: > that -> the Done. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:97: std::min(kResidualEchoPowerRenderWindowSize - 1, delay_use + 1), &X2); On 2017/04/07 09:34:39, AleBzk wrote: > I'm not sure, but maybe you want to DCHECK (kResidualEchoPowerRenderWindowSize - > 1 >= delay_use + 1 || kResidualEchoPowerRenderWindowSize - 1 >= delay_use - 1) I'll put a DCHECK on delay_use. Done. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:106: delay ? *delay : kAdaptiveFilterLength, On 2017/04/07 11:08:56, aleloi wrote: > Please replace ternary op expression with > 'delay.value_or(kAdaptiveFilterLength)'. Awesome! Done. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:113: R2->fill((*std::max_element(R2->begin(), R2->end())) * 100.f); On 2017/04/07 11:08:57, aleloi wrote: > Should the '100.f' also be kFixedEchoPathGain'? No. Not necessary at least. This is some kind of maximum factor for the spectral leakage overhead that happens during saturation. The echo path gain is, on the other hand, the maximum gain of the echo path. https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/residual_echo_estimator.h (right): https://codereview.webrtc.org/2804223002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:42: // Estimates the residual echo power based on the erle and the linear power On 2017/04/07 09:34:39, AleBzk wrote: > "erle" stands for Echo Return Loss Enhancement, right? Since it's the first time > you use the acronym in this file, I would write the full name and then "(erle)" > - it also helps to understand the role of the variable in the LinearEstimate > signature. As an AEC newbie, that would help me to better follow the signal > processing pipeline. However, if you think it should be obvious, just ignore > this comment. Good point! Will do that! Done.
peah@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from alessiob@webrtc.org, ivoc@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2804223002/#ps40001 (title: "Changes in response to reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, aleloi@webrtc.org, alessiob@webrtc.org Link to the patchset: https://codereview.webrtc.org/2804223002/#ps60001 (title: "Added limiting of the delay size")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491569582429160,
"parent_rev": "38415b2d8e7a4a5a3c4ac52e24c6ecc31c2da007", "commit_rev":
"89420459d1d4d02e89f36241c1046407773ed9ff"}
Message was sent while issue was closed.
Description was changed from ========== Adding support for handling highly reverberant echoes in AEC3. This CL adds support for handling highly reverberant echoes in AEC3. The functionality is hardcoded to be have no effect (via a decay factor of 0), but this CL will be followed by other CLs for which nonzero decay factors will be allowed. Apart from this change, this CL also refactors the residual echo estimation code to make it shorter and more readable. The changes introduced herein are bitexact (for a decay factor of 0). BUG=webrtc:6018 ========== to ========== Adding support for handling highly reverberant echoes in AEC3. This CL adds support for handling highly reverberant echoes in AEC3. The functionality is hardcoded to be have no effect (via a decay factor of 0), but this CL will be followed by other CLs for which nonzero decay factors will be allowed. Apart from this change, this CL also refactors the residual echo estimation code to make it shorter and more readable. The changes introduced herein are bitexact (for a decay factor of 0). BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2804223002 Cr-Commit-Position: refs/heads/master@{#17589} Committed: https://chromium.googlesource.com/external/webrtc/+/89420459d1d4d02e89f36241c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/89420459d1d4d02e89f36241c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
