Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2718993002:
Added post-adjustment of the suppression gains (Closed)
|
Created:
3 years, 9 months ago by peah-webrtc Modified:
3 years, 9 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, 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. |
DescriptionAdded post-adjustment of the suppression gains
This CL adds correction of the echo suppressor gain
It contains 2 changes:
-Bounds the upper value for the echo suppression gain
for bin 1 to avoid that the high-pass filter causes the
gain to be high for bin 1, which in turn may impact the
realization of any lower gains for the neighboring bins.
-Bounds the upper values for the echo suppression gains
for the higher bins to avoid any impact of the
external anti-aliasing filters.
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2718993002
Cr-Commit-Position: refs/heads/master@{#16854}
Committed: https://chromium.googlesource.com/external/webrtc/+/a2376e7db6b6c7b8fbe070fe7e277e476361d3ff
Patch Set 1 : Added post-adjustment of the suppression gains #
Total comments: 14
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 3 : Added DHECK #
Messages
Total messages: 16 (9 generated)
Description was changed from ========== Added post-adjustment of the suppression gains BUG= ========== to ========== Added post-adjustment of the suppression gains This CL adds correction of the echo suppressor gain It contains 2 changes: -Bounds the upper value for the echo suppression gain for bin 1 to avoid that the high-pass filter causes the gain to be high for bin 1, which in turn may impact the realization of any lower gains for the neighboring bins. -Bounds the upper values for the echo suppression gains for the higher bins to avoid any impact of the external anti-aliasing filters. BUG=webrtc:6018 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:20002) has been deleted
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Here is a CL containing a new tuning for the AEC3. PTAL https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:148: 0.4f * std::max(1.f - high_bands_gain, 0.f); The former computation of high_bands_noise_scaling is a leftover from AEC2. There is no reason why the square should be there, so I removed it as that may cause the comfort noise to be too high. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (left): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:122: These two lines are now moved into GainPostProcessing(... https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:234: (*gain_squared)[0] = (*gain_squared)[1]; These two lines are now moved into GainPostProcessing(...
https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:142: float high_bands_gain = std::accumulate( Iiuc, this is not an accumulation at all. You are picking out the minimum element in the range, right? If so, why not just use std::min_element? https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:148: 0.4f * std::max(1.f - high_bands_gain, 0.f); On 2017/02/27 07:55:22, peah-webrtc wrote: > The former computation of high_bands_noise_scaling is a leftover from AEC2. > There is no reason why the square should be there, so I removed it as that may > cause the comfort noise to be too high. Acknowledged. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:31: // filter filter on the upper-freqyency gains influencing the overall achieved filter filter https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:31: // filter filter on the upper-freqyency gains influencing the overall achieved freqyency https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:34: int kAntiAlisingImpactLimit = 64 * 0.7f; Alising -> Aliasing https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:34: int kAntiAlisingImpactLimit = 64 * 0.7f; constexpr size_t
Thanks for the comments! I've addressed them in a new patch. PTAL https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:142: float high_bands_gain = std::accumulate( On 2017/02/27 08:12:52, hlundin-webrtc wrote: > Iiuc, this is not an accumulation at all. You are picking out the minimum > element in the range, right? If so, why not just use std::min_element? Great suggestion! Thanks! Done. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:31: // filter filter on the upper-freqyency gains influencing the overall achieved On 2017/02/27 08:12:52, hlundin-webrtc wrote: > filter filter Done. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:31: // filter filter on the upper-freqyency gains influencing the overall achieved On 2017/02/27 08:12:52, hlundin-webrtc wrote: > filter filter Done. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:34: int kAntiAlisingImpactLimit = 64 * 0.7f; On 2017/02/27 08:12:52, hlundin-webrtc wrote: > Alising -> Aliasing Done. https://codereview.webrtc.org/2718993002/diff/50001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:34: int kAntiAlisingImpactLimit = 64 * 0.7f; On 2017/02/27 08:12:52, hlundin-webrtc wrote: > constexpr size_t Done.
LGTM with one suggestion. https://codereview.webrtc.org/2718993002/diff/70001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right): https://codereview.webrtc.org/2718993002/diff/70001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:142: *std::min_element(suppression_gain.begin() + 3, suppression_gain.end()); In theory, min_element will return suppression_gain.end() if the given range is empty. The value of kFftLengthBy2Plus1 will of course prevent that from happening, but I suggest you add a simple RTC_DCHECK_GT(suppression_gain.size(), 3);
Thanks for the review!!! I added the suggested DCHECK. https://codereview.webrtc.org/2718993002/diff/70001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right): https://codereview.webrtc.org/2718993002/diff/70001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_filter.cc:142: *std::min_element(suppression_gain.begin() + 3, suppression_gain.end()); On 2017/02/27 08:32:58, hlundin-webrtc wrote: > In theory, min_element will return suppression_gain.end() if the given range is > empty. The value of kFftLengthBy2Plus1 will of course prevent that from > happening, but I suggest you add a simple > RTC_DCHECK_GT(suppression_gain.size(), 3); Good one! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2718993002/#ps90001 (title: "Added DHECK")
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": 90001, "attempt_start_ts": 1488185208585540,
"parent_rev": "fa5fdce1009619d4ada99d74d066834fd66d3f40", "commit_rev":
"a2376e7db6b6c7b8fbe070fe7e277e476361d3ff"}
Message was sent while issue was closed.
Description was changed from ========== Added post-adjustment of the suppression gains This CL adds correction of the echo suppressor gain It contains 2 changes: -Bounds the upper value for the echo suppression gain for bin 1 to avoid that the high-pass filter causes the gain to be high for bin 1, which in turn may impact the realization of any lower gains for the neighboring bins. -Bounds the upper values for the echo suppression gains for the higher bins to avoid any impact of the external anti-aliasing filters. BUG=webrtc:6018 ========== to ========== Added post-adjustment of the suppression gains This CL adds correction of the echo suppressor gain It contains 2 changes: -Bounds the upper value for the echo suppression gain for bin 1 to avoid that the high-pass filter causes the gain to be high for bin 1, which in turn may impact the realization of any lower gains for the neighboring bins. -Bounds the upper values for the echo suppression gains for the higher bins to avoid any impact of the external anti-aliasing filters. BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2718993002 Cr-Commit-Position: refs/heads/master@{#16854} Committed: https://chromium.googlesource.com/external/webrtc/+/a2376e7db6b6c7b8fbe070fe7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:90001) as https://chromium.googlesource.com/external/webrtc/+/a2376e7db6b6c7b8fbe070fe7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
