Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886733002:
Transparency increasing tuning for AEC3 (Closed)
|
Created:
3 years, 7 months ago by peah-webrtc Modified:
3 years, 7 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. |
DescriptionTransparency increasing tuning for AEC3.
This CL increases the transparency of the AEC3 via tuning.
The major changes are
1) Limiting the suppression gain to the 16 bit sample floor.
2) Controlling the rate of the suppression gain increase
according to the signal characteristics.
Apart from these tunings, the code for the suppression gain
was refactored to increase/maintain the code quality after
the above changes.
BUG=webrtc:7519, webrtc:7528, chromium:715893
Review-Url: https://codereview.webrtc.org/2886733002
Cr-Commit-Position: refs/heads/master@{#18229}
Committed: https://chromium.googlesource.com/external/webrtc/+/1d68089f4b68be04aaef0fdf0013412a01fab270
Patch Set 1 #
Total comments: 22
Patch Set 2 : Corrected unit test parameter list #Patch Set 3 : Fixing failing unittests #Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Fixed sign in the DCHECK comparison #Patch Set 6 : Changes in response to reviewer comments #Patch Set 7 : Fixed uninitialized value #Patch Set 8 : Rebase #Patch Set 9 : Fixed memory issue #
Messages
Total messages: 61 (44 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== AEC3 transparency increasing BUG= ========== to ========== Transparency increasing tuning for AEC3. This CL increases the transparency of the AEC3 by 1) Limiting the suppression gain to the 16 bit sample floor. 2) Controlling the rate of the suppression gain increase according to the signal characteristics. Apart from these tunings, the code for the suppression gain was refactored to increase the code quality. BUG=webrtc:7519,webrtc:7528, chromium:715893 ==========
Description was changed from ========== Transparency increasing tuning for AEC3. This CL increases the transparency of the AEC3 by 1) Limiting the suppression gain to the 16 bit sample floor. 2) Controlling the rate of the suppression gain increase according to the signal characteristics. Apart from these tunings, the code for the suppression gain was refactored to increase the code quality. BUG=webrtc:7519,webrtc:7528, chromium:715893 ========== to ========== Transparency increasing tuning for AEC3. This CL increases the transparency of the AEC3 via tuning. The major changes are 1) Limiting the suppression gain to the 16 bit sample floor. 2) Controlling the rate of the suppression gain increase according to the signal characteristics. Apart from these tunings, the code for the suppression gain was refactored to increase/maintain the code quality after the above changes. BUG=webrtc:7519,webrtc:7528, chromium:715893 ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, ivoc@webrtc.org
Hi, This CL contains further tuning of the AEC3. Could you please take a look?
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21666)
Sorry, but I needed to upload another patch with a one-liner change. I had missed building for debug mode and there was a build error for that.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/26226)
lgtm, I only have some minor comments and a few questions. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/erle_estimator.cc:45: size_t band_limit = 32; I think it would be nice to initialize this to to kFftLengthBy2 / 2. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:28: // Adjust the gains according to the presense of known external filters. I think it should be "presence". https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:51: if (render.size() == 1) { Should there be a DHECK for render.size() > 0? https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:56: *std::min_element(low_band_gain.begin() + 32, low_band_gain.end()); I think the constant (32) should be derived from kFftLengthBy2, either by introducing kFftLengthBy4, or by using kFftLengthBy2 / 2. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:121: min_decreasing = 2.f; Just checking, is this 2.f correct or a typo? (it's the only value that breaks the pattern) https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:159: low_noise_render ? 0.01f : kEchoMaskingMargin; This seems to have no effect, kEchoMaskingMargin is 0.01f, so it gets the same value regardless of the value of low_noise_render. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:186: for (size_t k = 1; k < gain.size() - 1; ++k) { It's possible to merge this loop with the previous one, which I expect to be a bit faster, but I'm not sure if this code is called often enough for it to be worth it, so I'll leave that up to you. You could do it like this (using a lambda would be useful if you decide to do this): Handle k == 0 for (k = 1; k<gain.size() - 1; ++k) { ... } Handle k == gain.size() - 1 https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:207: [](float a) { return a > 0.f ? 1.f / a : 1.f; }); Why is the default value 1.f? Doesn't that introduce a discontinuity? i.e. if a == 0.f: one_by_echo will be 1.f, but if a == 0.001f, one_by_echo will be 1000.f.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/12430) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
lgtm! A few small comments https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/erle_estimator.cc:54: erle_[k] = std::max(kMinErle, std::min(erle_[k], max_erle)); I think the code gets simpler to follow without the changing band_limit/max_erle at the end. How about keeping the old structure, but changing kMaxErle --> (k < KfftLenghtBy2 / 2 ? kMaxLfErle : kMaxHfErle) Or perhaps for (k = 1 .. kfftby2) { const bool is_low_frequency = < KfftLenghtBy2 / 2; // ... ... = max(..., min(..., is_low_frequency ? kMaxLfErle : kMaxHfErle)); } https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:64: auto sum_of_squares = [](float a, float b) { return a + b * b; }; const auto https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:207: [](float a) { return a > 0.f ? 1.f / a : 1.f; }); On 2017/05/22 14:58:00, ivoc wrote: > Why is the default value 1.f? Doesn't that introduce a discontinuity? i.e. if a > == 0.f: one_by_echo will be 1.f, but if a == 0.001f, one_by_echo will be 1000.f. Another related issue: perhaps we should avoid dividing by echo when the echo is < a_small_threshold_number, and not when it's == 0. To avoid very large numbers and discontinuities as Ivo pointed out.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: 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/19126)
Thanks for the reviews and the comments! https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/erle_estimator.cc:45: size_t band_limit = 32; On 2017/05/22 14:57:59, ivoc wrote: > I think it would be nice to initialize this to to kFftLengthBy2 / 2. Good point! I did not think of them being the same. Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/erle_estimator.cc:54: erle_[k] = std::max(kMinErle, std::min(erle_[k], max_erle)); On 2017/05/22 20:41:45, aleloi wrote: > I think the code gets simpler to follow without the changing band_limit/max_erle > at the end. How about keeping the old structure, but changing > > kMaxErle --> (k < KfftLenghtBy2 / 2 ? kMaxLfErle : kMaxHfErle) > > Or perhaps > > for (k = 1 .. kfftby2) { > const bool is_low_frequency = < KfftLenghtBy2 / 2; > // ... > ... = max(..., min(..., is_low_frequency ? kMaxLfErle : kMaxHfErle)); > } I agree that that would produce nicer code, but I don't think it makes sense to put in an if-statement of known outcome in a loop. But I think I found a way to rewrite the code so that it looks better using a lambda. I will, however, do that in a separate CL. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right): https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:28: // Adjust the gains according to the presense of known external filters. On 2017/05/22 14:58:00, ivoc wrote: > I think it should be "presence". Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:51: if (render.size() == 1) { On 2017/05/22 14:58:00, ivoc wrote: > Should there be a DHECK for render.size() > 0? Good point! Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:56: *std::min_element(low_band_gain.begin() + 32, low_band_gain.end()); On 2017/05/22 14:58:00, ivoc wrote: > I think the constant (32) should be derived from kFftLengthBy2, either by > introducing kFftLengthBy4, or by using kFftLengthBy2 / 2. Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:64: auto sum_of_squares = [](float a, float b) { return a + b * b; }; On 2017/05/22 20:41:45, aleloi wrote: > const auto Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:121: min_decreasing = 2.f; On 2017/05/22 14:58:00, ivoc wrote: > Just checking, is this 2.f correct or a typo? (it's the only value that breaks > the pattern) No, it should be correct. The idea when there is saturation is that the minimum should be lower if the echo is increasing. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:159: low_noise_render ? 0.01f : kEchoMaskingMargin; On 2017/05/22 14:58:00, ivoc wrote: > This seems to have no effect, kEchoMaskingMargin is 0.01f, so it gets the same > value regardless of the value of low_noise_render. Oups! Good find! Removed that. Done. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:186: for (size_t k = 1; k < gain.size() - 1; ++k) { On 2017/05/22 14:58:00, ivoc wrote: > It's possible to merge this loop with the previous one, which I expect to be a > bit faster, but I'm not sure if this code is called often enough for it to be > worth it, so I'll leave that up to you. You could do it like this (using a > lambda would be useful if you decide to do this): > > Handle k == 0 > for (k = 1; k<gain.size() - 1; ++k) { > ... > } > Handle k == gain.size() - 1 I considered that, but it makes the code somewhat longer so I decided to put the loops as this. I think they should all fit into the cache line as they are fairly short. https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:207: [](float a) { return a > 0.f ? 1.f / a : 1.f; }); On 2017/05/22 14:58:00, ivoc wrote: > Why is the default value 1.f? Doesn't that introduce a discontinuity? i.e. if a > == 0.f: one_by_echo will be 1.f, but if a == 0.001f, one_by_echo will be 1000.f. To avoid that we should have one_by_echo=inf if a == 0. However, in case the denominator is zero, the value is anyway never used. The construct is only there to ensure that something is returned instead of dividing by zero. I thought of DCHECKing for ensuring that it is really never used, but that is not straightforward to do. I'll add a comment instead https://codereview.webrtc.org/2886733002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/suppression_gain.cc:207: [](float a) { return a > 0.f ? 1.f / a : 1.f; }); On 2017/05/22 20:41:45, aleloi wrote: > On 2017/05/22 14:58:00, ivoc wrote: > > Why is the default value 1.f? Doesn't that introduce a discontinuity? i.e. if > a > > == 0.f: one_by_echo will be 1.f, but if a == 0.001f, one_by_echo will be > 1000.f. > > Another related issue: perhaps we should avoid dividing by echo when the echo is > < a_small_threshold_number, and not when it's == 0. To avoid very large numbers > and discontinuities as Ivo pointed out. This is not as big a problem as it may seem. If 1/echo is large, the resulting gain will exceed 1 which means it then will be corrected to 1. It is a bit unfortunate to have to perform the division far from where it is used, as it then would be clear that this is a no-issue.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: 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 peah@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 ivoc@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2886733002/#ps180001 (title: "Fixed uninitialized value")
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/19132)
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19137)
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/2886733002/#ps200001 (title: "Rebase")
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/19138)
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19142)
Patchset #9 (id:220001) has been deleted
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
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/2886733002/#ps280001 (title: "Fixed memory 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: linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
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": 280001, "attempt_start_ts": 1495535507773300,
"parent_rev": "5e171752a21a90c4ee186e5e2fffe8a0a6675e99", "commit_rev":
"1d68089f4b68be04aaef0fdf0013412a01fab270"}
Message was sent while issue was closed.
Description was changed from ========== Transparency increasing tuning for AEC3. This CL increases the transparency of the AEC3 via tuning. The major changes are 1) Limiting the suppression gain to the 16 bit sample floor. 2) Controlling the rate of the suppression gain increase according to the signal characteristics. Apart from these tunings, the code for the suppression gain was refactored to increase/maintain the code quality after the above changes. BUG=webrtc:7519,webrtc:7528, chromium:715893 ========== to ========== Transparency increasing tuning for AEC3. This CL increases the transparency of the AEC3 via tuning. The major changes are 1) Limiting the suppression gain to the 16 bit sample floor. 2) Controlling the rate of the suppression gain increase according to the signal characteristics. Apart from these tunings, the code for the suppression gain was refactored to increase/maintain the code quality after the above changes. BUG=webrtc:7519,webrtc:7528, chromium:715893 Review-Url: https://codereview.webrtc.org/2886733002 Cr-Commit-Position: refs/heads/master@{#18229} Committed: https://chromium.googlesource.com/external/webrtc/+/1d68089f4b68be04aaef0fdf0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001) as https://chromium.googlesource.com/external/webrtc/+/1d68089f4b68be04aaef0fdf0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
