Major updates to the echo removal functionality in AEC3

Major updates to the echo removal functionality in AEC3

This CL adds fairly significant changes to the echo removal 
functionality, the main ones being.
-More centralized control over the echo removal.
-Updated echo suppression gain behavior.
-Significantly increased usage of the linear adaptive filter.
-New echo removal functionality when the linear filter is not usable.

This CL is chained to the CL https://codereview.webrtc.org/2784023002/


BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2782423003
Cr-Commit-Position: refs/heads/master@{#17575}
Committed: https://chromium.googlesource.com/external/webrtc/+/86afe9d6618a57e4817e6f9261042e785b7ff0c8

[peah-webrtc, 2017-03-30 06:35:50]

Description was changed from

==========
Major updates to the echo removal functionality in AEC3

BUG=
==========

to

==========
Major updates to the echo removal functionality in AEC3

This CL adds fairly significant changes to the echo removal 
functionality, the main ones being.
-More centralized control over the echo removal.
-Updated echo suppression gain behavior.
-Significantly increased usage of the linear adaptive filter.
-New echo removal functionality when the linear filter is not usable.

This CL is chained to the CL https://codereview.webrtc.org/2784023002/


BUG=webrtc:6018
==========

[peah-webrtc, 2017-03-30 12:01:45]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2017-03-30 12:01:54]

Patchset #1 (id:20001) has been deleted

[peah-webrtc, 2017-03-30 12:02:31]

Patchset #1 (id:40001) has been deleted

[peah-webrtc, 2017-03-30 12:02:39]

Patchset #1 (id:60001) has been deleted

[peah-webrtc, 2017-03-30 12:02:47]

Patchset #1 (id:80001) has been deleted

[peah-webrtc, 2017-03-30 12:04:05]

peah@webrtc.org changed reviewers:
+ aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org

[peah-webrtc, 2017-03-30 12:04:05]

Hi,
Here is the second CL for the new AEC3 functionality that is now ready for
review. There are no further CLs scheduled for now (even though there are
further upcoming changes).

PTAL

[peah-webrtc, 2017-03-30 12:04:14]

The CQ bit was checked by peah@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 12:04:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-30 12:08:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 12:08:51]

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/20367)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15621)

[ivoc, 2017-04-05 15:21:25]

This one is looking pretty good as well! See some comments below.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:155: for (size_t j =
0; j < 0 + H.size(); ++j) {
Why is this better than the previous way to iterate over H? Also, "0 + H.size()"
looks strange.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:171:
RTC_DCHECK_GE(H.size(), 0 + H.size() - 1);
Why is it "0 + H.size()"?

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:242: for (auto H2_k
: H2_) {
auto&

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:63: // the filter.
I think it makes more sense to move this comment just before the for loop.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:64: const size_t delay =
reliable_delays_sum / num_reliable_delays;
As a sanity check I think it would be good to add a DCHECK to verify that delay
< H2.size().

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:134: const float max_sample =
fabs(*std::max_element(
A DCHECK for the size of x would be nice (max_element returns an iterator to
x.end() if x is empty)

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector_unittest.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:37:
e_ref.fill(10.f);
Isn't this already done in the init_blocks function?

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:38: a =
std::max(0.f, a - 0.3f * (kNoiseGatePower - a));
So this is equal to 1.3*a - 0.3*kNoiseGatePower, right? Where do these factors
come from?

[peah-webrtc, 2017-04-06 07:20:32]

Thanks for the comments! I have addressed them in two patches.

PTAL

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:155: for (size_t j =
0; j < 0 + H.size(); ++j) {
On 2017/04/05 15:21:25, ivoc wrote:
> Why is this better than the previous way to iterate over H? Also, "0 +
H.size()"
> looks strange.

Ah, sorry about that. I had an alternative implementation going which was needed
for an experimental feature, but decided in the end to revert to the old as that
made the code simpler.

I'll revert.
Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:171:
RTC_DCHECK_GE(H.size(), 0 + H.size() - 1);
On 2017/04/05 15:21:25, ivoc wrote:
> Why is it "0 + H.size()"?

Same thing here, a replace error. 

Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:242: for (auto H2_k
: H2_) {
On 2017/04/05 15:21:25, ivoc wrote:
> auto&

Wow! Great find!

Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:63: // the filter.
On 2017/04/05 15:21:25, ivoc wrote:
> I think it makes more sense to move this comment just before the for loop.

Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:64: const size_t delay =
reliable_delays_sum / num_reliable_delays;
On 2017/04/05 15:21:25, ivoc wrote:
> As a sanity check I think it would be good to add a DCHECK to verify that
delay
> < H2.size().

Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:134: const float max_sample =
fabs(*std::max_element(
On 2017/04/05 15:21:25, ivoc wrote:
> A DCHECK for the size of x would be nice (max_element returns an iterator to
> x.end() if x is empty)

Good point. 

Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector_unittest.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:37:
e_ref.fill(10.f);
On 2017/04/05 15:21:25, ivoc wrote:
> Isn't this already done in the init_blocks function?

True.
Done.

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right):

https://codereview.webrtc.org/2782423003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:38: a =
std::max(0.f, a - 0.3f * (kNoiseGatePower - a));
On 2017/04/05 15:21:25, ivoc wrote:
> So this is equal to 1.3*a - 0.3*kNoiseGatePower, right? Where do these factors
> come from?

Yes, but writing it like that is a bit misleading I think. 

The 0.3 is a totally ad-hoc choice of rate for transitioning from a to zero when
a is below kNoiseGatePower. The precise value 0.3 is likely not important (I
haven't really checked) but what I want to accomplish by this is that I get a
noise-gate with a knee that is not abrupt.

[ivoc, 2017-04-06 09:09:53]

LGTM with 2 more comments, nice!

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:253: for (auto H2_k
: H2_) {
This should be auto& as well.

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:135: RTC_DCHECK_GT(0,
x.size());
Shouldn't this be RTC_DCHECK_GT(x.size(), 0)?

[peah-webrtc, 2017-04-06 09:25:31]

Thanks again for the comments!!!

I've uploaded a new patch with the changes.

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:253: for (auto H2_k
: H2_) {
On 2017/04/06 09:09:53, ivoc wrote:
> This should be auto& as well.

Thanks!!! Great find!

Done.

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:135: RTC_DCHECK_GT(0,
x.size());
On 2017/04/06 09:09:53, ivoc wrote:
> Shouldn't this be RTC_DCHECK_GT(x.size(), 0)?

Of course, thanks!!!
I changed it to RTC_DCHECK_LT(0, x.size()); to get the error report correct.

Done.

[hlundin-webrtc, 2017-04-06 09:50:48]

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:135: RTC_DCHECK_GT(0,
x.size());
On 2017/04/06 09:25:31, peah-webrtc wrote:
> On 2017/04/06 09:09:53, ivoc wrote:
> > Shouldn't this be RTC_DCHECK_GT(x.size(), 0)?
> 
> Of course, thanks!!!
> I changed it to RTC_DCHECK_LT(0, x.size()); to get the error report correct.
> 
> Done.

For RTC_(D)CHECK_* the order does not matter. You get printouts like
# Check failed: 17 <= 4 (17 vs. 4)
or
# Check failed: 17 == 4 (17 vs. 4)
which don't label any of the arguments as "expected" or "actual".

For gtest, the order matters sometimes. In particular for EXPECT_EQ and
EXPECT_NE, which print out stuff like
Value of: 4
Expected: 17

However, gtest's inequality tests don't care about ordering. For instance,
EXPECT_LE prints out
Expected: (17) <= (4), actual: 17 vs 4

[peah-webrtc, 2017-04-06 10:45:54]

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:135: RTC_DCHECK_GT(0,
x.size());
On 2017/04/06 09:50:48, hlundin-webrtc wrote:
> On 2017/04/06 09:25:31, peah-webrtc wrote:
> > On 2017/04/06 09:09:53, ivoc wrote:
> > > Shouldn't this be RTC_DCHECK_GT(x.size(), 0)?
> > 
> > Of course, thanks!!!
> > I changed it to RTC_DCHECK_LT(0, x.size()); to get the error report correct.
> > 
> > Done.
> 
> For RTC_(D)CHECK_* the order does not matter. You get printouts like
> # Check failed: 17 <= 4 (17 vs. 4)
> or
> # Check failed: 17 == 4 (17 vs. 4)
> which don't label any of the arguments as "expected" or "actual".
> 
> For gtest, the order matters sometimes. In particular for EXPECT_EQ and
> EXPECT_NE, which print out stuff like
> Value of: 4
> Expected: 17
> 
> However, gtest's inequality tests don't care about ordering. For instance,
> EXPECT_LE prints out
> Expected: (17) <= (4), actual: 17 vs 4

Ah, that I did not know, I thought the order convention was due to the output
formatting. But am I right in my impression that we always strive to put the
thing compared against as the first argument, and the thing that is compared as
the second argument?

[hlundin-webrtc, 2017-04-06 11:13:26]

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2782423003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:135: RTC_DCHECK_GT(0,
x.size());
On 2017/04/06 10:45:54, peah-webrtc wrote:
> On 2017/04/06 09:50:48, hlundin-webrtc wrote:
> > On 2017/04/06 09:25:31, peah-webrtc wrote:
> > > On 2017/04/06 09:09:53, ivoc wrote:
> > > > Shouldn't this be RTC_DCHECK_GT(x.size(), 0)?
> > > 
> > > Of course, thanks!!!
> > > I changed it to RTC_DCHECK_LT(0, x.size()); to get the error report
correct.
> > > 
> > > Done.
> > 
> > For RTC_(D)CHECK_* the order does not matter. You get printouts like
> > # Check failed: 17 <= 4 (17 vs. 4)
> > or
> > # Check failed: 17 == 4 (17 vs. 4)
> > which don't label any of the arguments as "expected" or "actual".
> > 
> > For gtest, the order matters sometimes. In particular for EXPECT_EQ and
> > EXPECT_NE, which print out stuff like
> > Value of: 4
> > Expected: 17
> > 
> > However, gtest's inequality tests don't care about ordering. For instance,
> > EXPECT_LE prints out
> > Expected: (17) <= (4), actual: 17 vs 4
> 
> Ah, that I did not know, I thought the order convention was due to the output
> formatting. But am I right in my impression that we always strive to put the
> thing compared against as the first argument, and the thing that is compared
as
> the second argument?

The order convention *is* due to output formatting, but it only applies where
the output format actually makes a difference between the expected and actual
value. None of the (D)CHECKs does that, and from the gtest suite, only _EQ and
_NE tests do that.

[aleloi, 2017-04-06 14:34:22]

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:74: // the adaptive
filter.
This comment needs to be updated, since use_filter_statistics is no longer a
c-tor argument.

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right):

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:39: }
This can be simplified to something like this:

minG = std::min_element( gain_squared->begin() + 2, gain_squared->begin() +
kLimit).

[aleloi, 2017-04-06 14:41:46]

lgtm. I havn't looked super-carefully at everything, but the changes in
adaptive_fir_filter, aec_state, residual_echo_estimator, suppression_gain LG,
and the rest seems mostly minor changes.

[peah-webrtc, 2017-04-06 21:11:14]

Thanks for the comments!

I have uploaded a patch  addressing those.

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:74: // the adaptive
filter.
On 2017/04/06 14:34:22, aleloi wrote:
> This comment needs to be updated, since use_filter_statistics is no longer a
> c-tor argument.

Good find!

Done.

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right):

https://codereview.webrtc.org/2782423003/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:39: }
On 2017/04/06 14:34:22, aleloi wrote:
> This can be simplified to something like this:
> 
> minG = std::min_element( gain_squared->begin() + 2, gain_squared->begin() +
> kLimit).
> 

True. Furthermore, it turned out that minG was not used. I remove this code.

Done.

[peah-webrtc, 2017-04-06 21:11:39]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-06 21:11:39]

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/2782423003/#ps220001 (title:
"Correction for incorrect rebase")

[commit-bot: I haz the power, 2017-04-06 21:11:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-06 21:18:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 21:18:32]

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/17985)

[peah-webrtc, 2017-04-06 21:42:40]

peah@webrtc.org changed reviewers:
- henrik.lundin@webrtc.org

[peah-webrtc, 2017-04-06 21:42:51]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-06 21:42:52]

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/2782423003/#ps260001 (title:
"Corrected uninitialized value access and corrected difference between SSE2 code
and its reference")

[commit-bot: I haz the power, 2017-04-06 21:42:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-06 22:00:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 22:00:48]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/19458)

[peah-webrtc, 2017-04-06 22:20:45]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-06 22:20:45]

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/2782423003/#ps280001 (title:
"Added initialization of uninitialized vector")

[commit-bot: I haz the power, 2017-04-06 22:20:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-06 22:45:34]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1491517245147930,
"parent_rev": "f51517a64fc234d3efb5aa93772d81063af08a08", "commit_rev":
"86afe9d6618a57e4817e6f9261042e785b7ff0c8"}

[commit-bot: I haz the power, 2017-04-06 22:45:38]

Description was changed from

==========
Major updates to the echo removal functionality in AEC3

This CL adds fairly significant changes to the echo removal 
functionality, the main ones being.
-More centralized control over the echo removal.
-Updated echo suppression gain behavior.
-Significantly increased usage of the linear adaptive filter.
-New echo removal functionality when the linear filter is not usable.

This CL is chained to the CL https://codereview.webrtc.org/2784023002/


BUG=webrtc:6018
==========

to

==========
Major updates to the echo removal functionality in AEC3

This CL adds fairly significant changes to the echo removal 
functionality, the main ones being.
-More centralized control over the echo removal.
-Updated echo suppression gain behavior.
-Significantly increased usage of the linear adaptive filter.
-New echo removal functionality when the linear filter is not usable.

This CL is chained to the CL https://codereview.webrtc.org/2784023002/


BUG=webrtc:6018

Review-Url: https://codereview.webrtc.org/2782423003
Cr-Commit-Position: refs/heads/master@{#17575}
Committed:
https://chromium.googlesource.com/external/webrtc/+/86afe9d6618a57e4817e6f926...
==========

[commit-bot: I haz the power, 2017-04-06 22:45:39]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/external/webrtc/+/86afe9d6618a57e4817e6f926...