Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808733002:
Added forced zero AEC output after call startup and echo path changes (Closed)
|
Created:
3 years, 8 months ago by peah-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, 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. |
DescriptionAdded forced zero AEC output after call startup and echo path changes
During the first few capture frames, there is no way for the AEC
to tell whether there is echo in the capture signal as the echo
removal functionality in the AEC has not yet seen any render
signal. To avoid initial echo bursts due to this, this CL adds
functionality for forcing the echo suppression gain to zero during
the first 50 blocks (200 ms) after call start and after a reported
echo path change.
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2808733002
Cr-Commit-Position: refs/heads/master@{#17624}
Committed: https://chromium.googlesource.com/external/webrtc/+/6d822adac4439258b0a07c5458620fcf2314874f
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changes in response to reviewer comments #Patch Set 3 : Fixed typo breaking unittest builds #Patch Set 4 : Fixed memory access issue #
Total comments: 2
Patch Set 5 : Fixed memory access issue again #
Messages
Total messages: 38 (20 generated)
Description was changed from ========== Added initial forcing of zero suppression gain after call startup and echo path changes During the first few capture frames, there is no way for the AEC to tell whether there is echo in the capture signal as the echo removal functionality in the AEC has not yet seen any render signal. To avoid initial echo bursts due to this, this CL adds functionality for forcing the echo suppression gain to zero during the first 50 blocks (200 ms) after call start and after a reported echo path change. BUG= ========== to ========== Added forced zero AEC output after call startup and echo path changes During the first few capture frames, there is no way for the AEC to tell whether there is echo in the capture signal as the echo removal functionality in the AEC has not yet seen any render signal. To avoid initial echo bursts due to this, this CL adds functionality for forcing the echo suppression gain to zero during the first 50 blocks (200 ms) after call start and after a reported echo path change. BUG=webrtc:6018 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, ivoc@webrtc.org
Hi, here comes another (fairly short) AEC3 CL. PTAL
LG, and a few comments. https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec_state.cc (right): https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec_state.cc:129: force_zero_gain_ = (++force_zero_gain_counter_) < kZeroGainBlocksAfterChange; The zero gain counter is a size_t. Isn't there a risk it overflows? https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:368: *high_bands_gain = 0.f; I think this function will be a bit more simple if it is rearranged as if (force_zero_gain) { // handle special zero gain case return; } // handle main case: ...
Thanks for the comments. I've addressed them in a new patch. PTAL https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec_state.cc (right): https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec_state.cc:129: force_zero_gain_ = (++force_zero_gain_counter_) < kZeroGainBlocksAfterChange; On 2017/04/10 09:35:51, aleloi wrote: > The zero gain counter is a size_t. Isn't there a risk it overflows? Good point! I thought about the effect of that. The counter is increased by 250 each second. On an 32bit platform, that corresponds to an overflow after 6 months. As discussed offline, together with the nonintrusive effect of this occuring, that should be fine. In particular as any handling of that is not testable. WDYT? https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:368: *high_bands_gain = 0.f; On 2017/04/10 09:35:51, aleloi wrote: > I think this function will be a bit more simple if it is rearranged as > > if (force_zero_gain) { > // handle special zero gain case > return; > } > // handle main case: > ... That is indeed a much better way to write this. Great! Done!
lgtm.
lgtm https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec_state.cc (right): https://codereview.webrtc.org/2808733002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec_state.cc:129: force_zero_gain_ = (++force_zero_gain_counter_) < kZeroGainBlocksAfterChange; On 2017/04/10 09:57:24, peah-webrtc wrote: > On 2017/04/10 09:35:51, aleloi wrote: > > The zero gain counter is a size_t. Isn't there a risk it overflows? > > Good point! I thought about the effect of that. The counter is increased by 250 > each second. On an 32bit platform, that corresponds to an overflow after 6 > months. As discussed offline, together with the nonintrusive effect of this > occuring, that should be fine. In particular as any handling of that is not > testable. > > WDYT? Acknowledged.
The CQ bit was checked by peah@webrtc.org
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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24701)
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 Link to the patchset: https://codereview.webrtc.org/2808733002/#ps120001 (title: "Fixed typo breaking unittest builds")
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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23856) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/25082)
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 Link to the patchset: https://codereview.webrtc.org/2808733002/#ps140001 (title: "Fixed memory access issue")
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/18077)
https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/suppression_gain.cc:337: previous_masker_.begin()); I think it should be previous_masker_->begin()
https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/suppression_gain.cc:337: previous_masker_.begin()); On 2017/04/10 12:19:13, aleloi wrote: > I think it should be previous_masker_->begin() Sorry, disregard that. comfort_noise_power is two floats larger than previuos_masker_, not one.
On 2017/04/10 12:21:56, aleloi wrote: > https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): > > https://codereview.webrtc.org/2808733002/diff/140001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec3/suppression_gain.cc:337: > previous_masker_.begin()); > On 2017/04/10 12:19:13, aleloi wrote: > > I think it should be previous_masker_->begin() > > Sorry, disregard that. comfort_noise_power is two floats larger than > previuos_masker_, not one. Awesome! Thanks! That explains it, I thought it was 1 float larger. Will update.
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 Link to the patchset: https://codereview.webrtc.org/2808733002/#ps160001 (title: "Fixed memory access issue again")
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: win_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by peah@webrtc.org
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": 160001, "attempt_start_ts": 1491857336720770,
"parent_rev": "ca31f175e1e14d14f5036e4a9d4ef13796c9b799", "commit_rev":
"6d822adac4439258b0a07c5458620fcf2314874f"}
Message was sent while issue was closed.
Description was changed from ========== Added forced zero AEC output after call startup and echo path changes During the first few capture frames, there is no way for the AEC to tell whether there is echo in the capture signal as the echo removal functionality in the AEC has not yet seen any render signal. To avoid initial echo bursts due to this, this CL adds functionality for forcing the echo suppression gain to zero during the first 50 blocks (200 ms) after call start and after a reported echo path change. BUG=webrtc:6018 ========== to ========== Added forced zero AEC output after call startup and echo path changes During the first few capture frames, there is no way for the AEC to tell whether there is echo in the capture signal as the echo removal functionality in the AEC has not yet seen any render signal. To avoid initial echo bursts due to this, this CL adds functionality for forcing the echo suppression gain to zero during the first 50 blocks (200 ms) after call start and after a reported echo path change. BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2808733002 Cr-Commit-Position: refs/heads/master@{#17624} Committed: https://chromium.googlesource.com/external/webrtc/+/6d822adac4439258b0a07c545... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/6d822adac4439258b0a07c545... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
