Finalization of the first version of EchoCanceller 3

Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

TBR=aleloi@webrtc.org
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2678423005
Cr-Commit-Position: refs/heads/master@{#16801}
Committed: https://chromium.googlesource.com/external/webrtc/+/522d71bf3605e99f4a2b5fd253d5ccd171575e7d

[peah-webrtc, 2017-02-08 13:30:54]

Description was changed from

==========
Finalization of the first version of the EchoCanceller 3 solution

This CL adds the remaining code for the first version of EchoCanceller3.

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

to

==========
Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

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

[peah-webrtc, 2017-02-08 14:02:14]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2017-02-08 14:02:25]

Patchset #1 (id:20001) has been deleted

[peah-webrtc, 2017-02-08 14:02:36]

Patchset #1 (id:30001) has been deleted

[peah-webrtc, 2017-02-08 14:52:54]

Patchset #1 (id:50001) has been deleted

[peah-webrtc, 2017-02-08 15:04:40]

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

[peah-webrtc, 2017-02-08 15:04:41]

Hi,
Here is the final of the CLs for landing the first version of AEC3. 

PTAL

[aleloi, 2017-02-09 17:02:59]

The first few comments of a probably very long sequence.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:172: [](float a, float b)
-> float { return a + b * b; });
Maybe use std::inner_product? Less lambdas is simpler.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:170:
TEST(EchoRemover, BasicEchoRemoval) {
In this test, the near-end signal is comprised entirely of echo. Is it possible
to write similar test/tests for echo + noise?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:180:
ProduceDebugText(rate, delay_samples);
This doesn't do anything: ProduceDebugText is a function that returns a string
and has no side effects. It's used the same way in decimator_by_4_unittest.
Should it be logged with SCOPED_TRACE?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:195: }
The 'silence' updating construct seems a little strange. Only frames 100-109,
200-209, 300-309, 400-409 will have nonzero samples. Why do we start with 100
blocks of silence?

Also, I think it's rather hard to understand what happens here. Can you change
it to

for k = 0..:
   const bool silence = <t/f condition calculated from 'k'>

?

To keep it working as is, it would be 

 'const bool silence = k < 100 || (k % 100 >= 10)' 

which I think is much better (but still a bit complex).

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:213:
echo_path_variability, false, x, &y);
The remover only sees the last 250 of the samples. That seems wrong. I think the
remover should process all of the samples, but that energy levels should only be
counted starting from kNumBlocksToProcess/2.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/fft_data.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/fft_data.h:43: // Copy the data from an
interlieved array.
interlieved -> interleaved (here and in some other places)

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_output.h:27: #include
"webrtc/modules/audio_processing/utility/ooura_fft.h"
I don't think this file needs all these includes.

[ivoc, 2017-02-10 13:52:35]

First comments about the scary set pt.1, it was surprisingly a lot less scary
than I imagined :-)

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:29: const float kX2Min =
44015068.0f / 1000.f;
A comment about where this value comes from would be helpful.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:34: } else {
this can be "else if" to avoid the nested if. One thing that worries me a bit in
this loop is the possibility for elements in |bands_with_reliable_filter| to not
be changed. if the "if" and the "else if" don't hold, the old value will remain.
Is that the intended behavior?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:19: #include
"webrtc/base/optional.h"
I don't think Optional is used in this header.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:26: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Is this needed?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:38: const
std::array<float, kFftLengthBy2Plus1>& Y2 = capture_spectrum;
I would prefer const auto&

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:45: [](float a,
float b) { return a + 0.1f * (b - a); });
Out of curiousity, what kind of filter is this?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:60: // Compute
initial N2 from N2.
This is a bit confusing. I would rephrase as "Compute N2_initial from N2."

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:79: [](float a,
float b) { return a + b; }) /
The last argument (the lambda) is not needed, accumulate sums things by default.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:89: const float
kScale = 6.28318530717959f * 1.f / 32768.0f;
The "* 1.f" can be removed, right? Also, this can be constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:109: //
TODO(peah): Remove when landing.
Is that now?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:14: #include
<stdio.h>
Is this needed? For the uin32_t you could use stdint.h

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:16: #include
<vector>
Can be removed

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:18: #include
"webrtc/base/array_view.h"
Looks like ArrayView is not used in this header, but perhaps it should be used
on the interfaces (instead of array), for more flexibility. WDYT?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:22: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Is this used?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:33: void
Compute(const std::array<float, kFftLengthBy2Plus1>& capture_spectrum,
#include <array> is missing.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:24: const
std::vector<std::array<float, kFftLengthBy2Plus1>>& H2 =
I would prefer const auto& here.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:30: for (size_t k = 1; k <
kFftLengthBy2 - 5; ++k) {
Why is the "- 5" here?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:38: if (5 * H2[H2.size() -
1][k] < H2[peak][k]) {
Please define the 5 as a constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:44: return
num_reliable_delays > 5
This 5 as well.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:2: *  Copyright (c) 2016
The WebRTC project authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:18: #include
"webrtc/base/array_view.h"
It looks like this is not used in this header...

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:37: void UpdateDelays(const
std::vector<std::array<float, kFftLengthBy2Plus1>>&
... But maybe it should be used here instead of std::vector. It would make it
easier to call this function with other data than vectors (and vectors
automatically convert to ArrayView).

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:39: const
rtc::Optional<size_t>& external_delay_samples);
Since this is just a single number, I think the name should be
external_delay_sample (without the s).

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler_unittest.cc:16:
TEST(DelayHandler, CorrectOperation) {
I think it makes sense to split this test up into a few smaller tests, WDYT?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_path_variability.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_path_variability.cc:2: *  Copyright
(c) 2016 The WebRTC project authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:34: const
std::array<float, kFftLengthBy2Plus1>& X2 = render_spectrum;
I would prefer const auto&.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:37: const float kX2Min =
44015068.0f;
Where does this number come from? A comment would be helpful. Also constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:2: *  Copyright (c) 2016
The WebRTC project authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:16: #include
"webrtc/base/array_view.h"
Again, no ArrayViews in this file, but maybe there should be, WDYT?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:18: #include
"webrtc/base/optional.h"
Can be removed.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator_unittest.cc:30: {
These lines reoccur a few times, you can consider turning them into a small
function.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:27: const
std::array<float, kFftLengthBy2Plus1>& X2 = render_spectrum;
I would prefer const auto&.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:31: const float kX2Min =
44015068.0f;
Where does this value come from? A comment would be helpful. Also constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:56:
ErleEstimator::ErleEstimator() {
Move the constructor and destructor to the top of the file please.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.h:16: #include
"webrtc/base/array_view.h"
See other remarks about ArrayView.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator_unittest.cc:31: {
These lines reoccur a few times, it might be a good idea to turn them into a
small function.

[peah-webrtc, 2017-02-11 22:20:02]

Patchset #2 (id:90001) has been deleted

[aleloi, 2017-02-13 16:44:51]

And yet another portion of comments. Getting there!

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:18: #include
"webrtc/base/array_view.h"
Include order. Also, array_view is not used.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:34: #include
"webrtc/modules/audio_processing/include/audio_processing.h"
Is audio_buffer and audio_processing used?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:35: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Include order

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:118:
std::vector<std::vector<float>>* y = capture;
Is it infeasible to change the inner vectors to array<float, kBlockSize>? That
would affect everything starting from FrameBlocker, right?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:119: 
A few general comments:

I think it would improve readability somewhat to declare some of the arrays
closer to point of use.

Also, perhaps consider inner scopes. Both to limit what parts of the program an
array is visible in, and to structure the control flow a bit. E.g. 'G' can be
declared, computed and used in its own scope.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.cc:56: 
Please make y2_sum, e2_main_sum, subtractor_output const

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.cc:73: }
I think the control flow can be a little simplified here. WDYT about this
change:

if (is_best != sub_output) counter++
if (counter > 15) {
   sub_output=...;
   Transition(...);
   counter=0;
} else if (sub_output) copy(...);

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:14: #include <algorithm>
Not used in this header.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:18: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Also not used here.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:30: void
FormLinearOutput(rtc::ArrayView<const float> linear_aec_output,
linear_aec_output is also called subtractor_output below. Maybe choose one name
for both to make it simpler to read in isolation?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:36: auto
form_e_and_y = [&](bool y_equals_weaker) {
const auto

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc:15: #include
"webrtc/base/array_view.h"
Not used here.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:14: #include
<stdio.h>
Not used, I think.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:16: #include
<vector>
Replace with std::array.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:18: #include
"webrtc/base/constructormagic.h"
Not used. But perhaps it should? Why would we copy/assign a
RenderSignalAnalyzer?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:26: return v;
return {kMainFilterLength, kShadowFilterLength};

should do the same thing. Same for the else clause below.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:77: [&](float a, float b) {
return a - b * kScale; });
Beginner question: why the scaling? Is it because it's missing in Ifft and FFT
composed with Ifft is not identity?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:93: 
I suggest putting the filtering & subtraction in a function to avoid having to
almost identical blocks.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:18: 
stdio, memory not needed, <array> missing.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:48: std::vector<size_t>
NumBlocksInRenderSums() const;
This could probably be a constexpr function if the return type is changed from
std::vector and the filter length constants are constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:52: return
std::max(kMainFilterLength, kShadowFilterLength);
Can be a constexpr function if the constants are costexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:65: const size_t
kShadowFilterLength = 12;
constexpr.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:25: void
RunSubtractorTest(int num_blocks_to_process,
I can't find a reason for why echo_to_nearend_power shouldn't be a return value.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:73: float y_power =
std::inner_product(y.begin(), y.end(), y.begin(), 0.f);
const float *_power

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:129: //
blocks_with_echo_path_changes.push_back(99);
remove comment

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:143: //
blocks_with_echo_path_changes.push_back(99);
remove comment

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:151:
EXPECT_GT(1.05f, echo_to_nearend_power);
I think EXPECT_NEAR(a, b, 0.05f) can be used here.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:166: EXPECT_EQ(1.f,
echo_to_nearend_power);
Does EXPECT_EQ use exact comparison for floats? Maybe use EXPECT_NEAR to not
attract attention of the vengeful floating point deity?

[hlundin-webrtc, 2017-02-13 21:37:01]

First round of comments on my batch of files. I still haven't looked at
hadow_filter_update_gain_unittest.cc.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:23: // Constrains an
adaptive filter partition.
Comment on what this does mathematically.
IFFT of H -> h
Zero out upper part(?) of h
FFT h -> H

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:29: [&](float& a) {
a *= kScale; });
Is the default capture [&] necessary for the lambda to pick up kScale?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:34: // Adapts the
filter partitions.
What is the relationship between X, G, and H?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:34: // Adapts the
filter partitions.
And what is the math?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:43: H_j.re[k] +=
X.re[k] * G.re[k] + X.im[k] * G.im[k];
Please, comment on why the signs are this way. Normally, one would expect
x * y = (xr + i*xi) * (yr + i*yi) = (xr*yr - xi*yi) + i*(xr*yi + xi*yr)

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:81: // Computes and
stores the echo return loss of the filter.
Show me the math.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:120:
++instance_index_;
Atomic operation?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:137:
ApplyFilter(X_buffer, H_, S);
Why the extra function? You can implement it here.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Forward declare.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:28: // Provides an
frequency domain adaptive filter functionality.
an -> a

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:35: // Produces the
output of the filter.
I wouldn't mind a blank line after each method declaration.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:37: // Adapts the
filter
.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:39: // Handles echo
path change.
Can you say something more?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:43: // Returns the
filter based echo return loss
.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:44: const
std::array<float, kFftLengthBy2Plus1>& Erl() const {
Oh, so this method and the next both go down in flames if use_filter_statistics
was false in the ctor. That should probably be documented.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:45: RTC_DCHECK(erl_);
Improve the death here by supplying an explanation:
RTC_DCHECK(erl_) << "Died because something....";

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:51: RTC_DCHECK(H2_);
Explain.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:44: //
Verifies that the check for non-null data dumper works.
Copy-paste error in comment.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:80: //
Verifies that the filter size if correctly repported.,
.,

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:83: for
(size_t k = 1; k < 30; ++k) {
Rename k -> filter_size?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:83: for
(size_t k = 1; k < 30; ++k) {
Why test so many sizes? And, is there something magic about 30?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:90: //
adapt its coefficients1.
Sure, you are checking coefficients1, but what about coefficients2? :)

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:143:
EXPECT_TRUE(delay_handler.FilterDelay());
ASSERT_TRUE

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:22: constexpr
float kHerrorInitial = 10000.f;
kHErrorInitial

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:44: const
FftBuffer& X_buffer = render_buffer;
You may want to comment something like "introduce shorter notation for better
math readability".

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:63: // Do not
update the filter if the render is not sufficiently persistently
"sufficiently persistently" sounds weird.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:96: // H_error =
H_error + K * erl.
What is K?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.h:21: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Not used?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:19:
#include "webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h"
Order of includes.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:77:
bool saturation =
const

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:82: if
(use_silent_render_in_second_half && k == num_blocks_to_process / 2) {
Should this be k>=num_blocks_to_process/2?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:218: //
error estimates causes the gain to increase after a period of saturation.
causes -> cause

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:23: for (size_t j = 0;
j < 20; ++j) {
Why 20?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:47: a.value =
kHInitial;
Can't you just as well say
a = CountedFloat(kHInitial, kUpdateCounterInitial);

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:67: :
delay_handler.ExternalDelay()
Put () around the "else" result, for readability.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:73: // Compute the R2
.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:75: if (!delay) {
If no delay, then calculate RecentMaximum. Care to explain why?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:87:
std::transform(X2_active.begin(), X2_active.end(), Y2.begin(),
Math comment, please.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:91: auto H2_updater =
[&](float a, CountedFloat b) {
What math does this lambda implement?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:93: b.counter = a >
b.value ? max_update_counter_value : b.counter - 1;
You're making this branch again just below. You may consider incorporating
updating of b.counter in the if-else below.

if (a > b.value) {
  b.value = a;
  b.counter = max_update_counter_value;
} else {
  --b.counter;
  if (b.counter <= 0) {  // I find it odd that the counter can be negative.
    b.value = std::max(b.value * 0.9f, 1.f);
  }
}

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:107:
std::transform(H2_.begin(), H2_.end(), X2_active.begin(), S2->begin(),
Math comment

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:110: std::array<float,
kFftLengthBy2Plus1> H2_tmp;
Why do you calculate H2_tmp? It is not used.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model_unittest.cc:104: // Verify
that stronger echo paths are detected immediately
.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc:59: (*v)[k - 2] =
(*v)[k - 1] = (*v)[k] = (*v)[k + 2] = (*v)[k + 1] = 0.f;
Nit: For a more natural readability I would suggest swapping k+1 and k+2.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:31: // Updates the
render signal analysis with the most recent render signal.
What does delay_partitions mean?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:31: void
ProduceSinusoidalSignal(int sample_rate_hz,
Shorten the name to ProduceSinusoid.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:78:
std::for_each(mask.begin(), mask.end(), [](float a) { EXPECT_EQ(1.f, a); });
Consider:
EXPECT_TRUE(std::all_of(mask.begin(), mask.end(), [](float a) { a == 1.f; });

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:94: //
Verify that a sinusiod signal is detected as narrow bands.
This seems like a new TEST.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:103: //
Verify that a no bands are detected as narrow when the delay is unknown.
Delete 'a'

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:103: //
Verify that a no bands are detected as narrow when the delay is unknown.
Seems like yet another TEST.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:110: 
You are only testing with an input mask with all ones. But if I understand the
implementation correctly, it should leave non-masked bins untouched. So, I'd
like to see a test where a mask set to !=1 is verified to be untouched outside
of the masked bins.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:34: // Do not
update the filter if the render is not sufficiently persistently
"sufficiently persistently"?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:47: const
std::array<float, kFftLengthBy2Plus1>& X2 =
const auto& ?

[peah-webrtc, 2017-02-20 07:37:19]

Thanks for the great comments! Here is the next update to the CL.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:23: // Constrains an
adaptive filter partition.
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Comment on what this does mathematically.
> IFFT of H -> h
> Zero out upper part(?) of h
> FFT h -> H

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:29: [&](float& a) {
a *= kScale; });
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Is the default capture [&] necessary for the lambda to pick up kScale?

No, it was not it turned out.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:34: // Adapts the
filter partitions.
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> And what is the math?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:34: // Adapts the
filter partitions.
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> What is the relationship between X, G, and H?

I included the math.

PTAL if you think that is sufficient!

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:43: H_j.re[k] +=
X.re[k] * G.re[k] + X.im[k] * G.im[k];
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Please, comment on why the signs are this way. Normally, one would expect
> x * y = (xr + i*xi) * (yr + i*yi) = (xr*yr - xi*yi) + i*(xr*yi + xi*yr)

It is the conjugate, I think. I added that to the equation.

PTAL

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:81: // Computes and
stores the echo return loss of the filter.
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Show me the math.

I added a comment describing what it is. PTAL

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:120:
++instance_index_;
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Atomic operation?

It could be removed. 

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:137:
ApplyFilter(X_buffer, H_, S);
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Why the extra function? You can implement it here.

The reason was that I was planning for it to be optimized. That optimization has
now been added.

PTAL.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Forward declare.

That does not work, as the data dumper functionality is used in the methods in
the header file.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:28: // Provides an
frequency domain adaptive filter functionality.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> an -> a

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:35: // Produces the
output of the filter.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> I wouldn't mind a blank line after each method declaration.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:37: // Adapts the
filter
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:39: // Handles echo
path change.
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Can you say something more?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:43: // Returns the
filter based echo return loss
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:44: const
std::array<float, kFftLengthBy2Plus1>& Erl() const {
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Oh, so this method and the next both go down in flames if
use_filter_statistics
> was false in the ctor. That should probably be documented.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:45: RTC_DCHECK(erl_);
On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> Improve the death here by supplying an explanation:
> RTC_DCHECK(erl_) << "Died because something....";

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:51: RTC_DCHECK(H2_);
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Explain.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:44: //
Verifies that the check for non-null data dumper works.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Copy-paste error in comment.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:80: //
Verifies that the filter size if correctly repported.,
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> .,

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:83: for
(size_t k = 1; k < 30; ++k) {
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Rename k -> filter_size?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:83: for
(size_t k = 1; k < 30; ++k) {
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Why test so many sizes? And, is there something magic about 30?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:90: //
adapt its coefficients1.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Sure, you are checking coefficients1, but what about coefficients2? :)

:-D

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:143:
EXPECT_TRUE(delay_handler.FilterDelay());
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> ASSERT_TRUE

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:29: const float kX2Min =
44015068.0f / 1000.f;
On 2017/02/10 13:52:33, ivoc wrote:
> A comment about where this value comes from would be helpful.

Good point! It has now been removed though.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:34: } else {
On 2017/02/10 13:52:33, ivoc wrote:
> this can be "else if" to avoid the nested if. One thing that worries me a bit
in
> this loop is the possibility for elements in |bands_with_reliable_filter| to
not
> be changed. if the "if" and the "else if" don't hold, the old value will
remain.
> Is that the intended behavior?

You definitely have a point. And in the tuning, this did not perform how I
wanted. Therefore I have removed it.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:19: #include
"webrtc/base/optional.h"
On 2017/02/10 13:52:33, ivoc wrote:
> I don't think Optional is used in this header.

You are right. But I have now changed it, so it is again used.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:26: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/10 13:52:33, ivoc wrote:
> Is this needed?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:38: const
std::array<float, kFftLengthBy2Plus1>& Y2 = capture_spectrum;
On 2017/02/10 13:52:33, ivoc wrote:
> I would prefer const auto&

Nice!
Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:45: [](float a,
float b) { return a + 0.1f * (b - a); });
On 2017/02/10 13:52:33, ivoc wrote:
> Out of curiousity, what kind of filter is this?

This is a low-pass filter: Y2_smoothed(t)=0.9f*Y2_smoothed(t) + 0.1*Y2;

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:60: // Compute
initial N2 from N2.
On 2017/02/10 13:52:33, ivoc wrote:
> This is a bit confusing. I would rephrase as "Compute N2_initial from N2."

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:79: [](float a,
float b) { return a + b; }) /
On 2017/02/10 13:52:33, ivoc wrote:
> The last argument (the lambda) is not needed, accumulate sums things by
default.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:89: const float
kScale = 6.28318530717959f * 1.f / 32768.0f;
On 2017/02/10 13:52:33, ivoc wrote:
> The "* 1.f" can be removed, right? Also, this can be constexpr.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:109: //
TODO(peah): Remove when landing.
On 2017/02/10 13:52:33, ivoc wrote:
> Is that now?

Should be :-) Thanks!
Done-

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:14: #include
<stdio.h>
On 2017/02/10 13:52:33, ivoc wrote:
> Is this needed? For the uin32_t you could use stdint.h

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:16: #include
<vector>
On 2017/02/10 13:52:34, ivoc wrote:
> Can be removed

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:18: #include
"webrtc/base/array_view.h"
On 2017/02/10 13:52:34, ivoc wrote:
> Looks like ArrayView is not used in this header, but perhaps it should be used
> on the interfaces (instead of array), for more flexibility. WDYT?

I prefer array as it will give a compile-time warning about the size which means
that DCHECKS on the size is not needed. Everything is anyway so tightly coupled
with the sizes so I don't the flexibility achieved by array-view could be used
anyway.

WDYT?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:22: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/10 13:52:34, ivoc wrote:
> Is this used?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:33: void
Compute(const std::array<float, kFftLengthBy2Plus1>& capture_spectrum,
On 2017/02/10 13:52:34, ivoc wrote:
> #include <array> is missing.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:24: const
std::vector<std::array<float, kFftLengthBy2Plus1>>& H2 =
On 2017/02/10 13:52:34, ivoc wrote:
> I would prefer const auto& here.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:30: for (size_t k = 1; k <
kFftLengthBy2 - 5; ++k) {
On 2017/02/10 13:52:34, ivoc wrote:
> Why is the "- 5" here?

The intention is that there is no point computing this for upper frequencies due
to the anti-aliasing filter. I changed it into a constant.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:38: if (5 * H2[H2.size() -
1][k] < H2[peak][k]) {
On 2017/02/10 13:52:34, ivoc wrote:
> Please define the 5 as a constexpr.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.cc:44: return
num_reliable_delays > 5
On 2017/02/10 13:52:34, ivoc wrote:
> This 5 as well.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:2: *  Copyright (c) 2016
The WebRTC project authors. All Rights Reserved.
On 2017/02/10 13:52:34, ivoc wrote:
> 2017

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:11: #ifndef
WEBRTC_MODULES_AUDIO_PROCESSING_AEC3_DELAY_HANDLER_H_
The functionality in DelayHandler is now moved into AecState.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:18: #include
"webrtc/base/array_view.h"
On 2017/02/10 13:52:34, ivoc wrote:
> It looks like this is not used in this header...

Thanks!
I removed this file now.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:37: void UpdateDelays(const
std::vector<std::array<float, kFftLengthBy2Plus1>>&
On 2017/02/10 13:52:34, ivoc wrote:
> ... But maybe it should be used here instead of std::vector. It would make it
> easier to call this function with other data than vectors (and vectors
> automatically convert to ArrayView).

True. The advantage of doing it like this though is that it is (slightly)
cheaper. And since this is so specialized code anyway I don't think we need to
make this generic.

WDYT

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler.h:39: const
rtc::Optional<size_t>& external_delay_samples);
On 2017/02/10 13:52:34, ivoc wrote:
> Since this is just a single number, I think the name should be
> external_delay_sample (without the s).

samples is the unit. But I see what you mean, this is very ambiguous. What I
intended was external_delay measured in samples, not sample of the external
delay. Do you have a good suggestion for an alternative name?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/delay_handler_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/delay_handler_unittest.cc:16:
TEST(DelayHandler, CorrectOperation) {
On 2017/02/10 13:52:34, ivoc wrote:
> I think it makes sense to split this test up into a few smaller tests, WDYT?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_path_variability.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_path_variability.cc:2: *  Copyright
(c) 2016 The WebRTC project authors. All Rights Reserved.
On 2017/02/10 13:52:34, ivoc wrote:
> 2017

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:18: #include
"webrtc/base/array_view.h"
On 2017/02/13 16:44:50, aleloi wrote:
> Include order. Also, array_view is not used.

Reordered the includes. I did, however, keep the include as it is now again
used.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:34: #include
"webrtc/modules/audio_processing/include/audio_processing.h"
On 2017/02/13 16:44:50, aleloi wrote:
> Is audio_buffer and audio_processing used?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:35: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/13 16:44:50, aleloi wrote:
> Include order

But this one is correctly ordered, right?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:118:
std::vector<std::vector<float>>* y = capture;
On 2017/02/13 16:44:50, aleloi wrote:
> Is it infeasible to change the inner vectors to array<float, kBlockSize>? That
> would affect everything starting from FrameBlocker, right?

We should definitely do that. But as you point out, that needs to start from the
top. Ok if we take that in another CL?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:119: 
On 2017/02/13 16:44:50, aleloi wrote:
> A few general comments:
> 
> I think it would improve readability somewhat to declare some of the arrays
> closer to point of use.
> 
> Also, perhaps consider inner scopes. Both to limit what parts of the program
an
> array is visible in, and to structure the control flow a bit. E.g. 'G' can be
> declared, computed and used in its own scope.

I definitely think you have a point. However, doing that will increase the size
of the core of the functionality. As it is now, having the variables declared on
top. The actual functionality of the method is visible on one single page on
screen, which I really like as it increases readability.

WDYT? Is it ok to have it as it is?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:172: [](float a, float b)
-> float { return a + b * b; });
On 2017/02/09 17:02:59, aleloi wrote:
> Maybe use std::inner_product? Less lambdas is simpler.

Inner product looks perfect for this. I have some concerns, however, that the
inner product might be more expensive as it uses two iterators.

I broke this out into a separate method for now that we definitely should have
an optimized version for later on. I kept the accumulate for now though.

WDYT, can inner_product be more expensive than accumulate when it operates on
one single vector?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:170:
TEST(EchoRemover, BasicEchoRemoval) {
On 2017/02/09 17:02:59, aleloi wrote:
> In this test, the near-end signal is comprised entirely of echo. Is it
possible
> to write similar test/tests for echo + noise?

Absolutely, but I'm not sure if we should have that here/yet as the AEC
functionality is still being developed. I prefer to stay with having  this
sanity check test for now and instead consider adding more refined tests later.
Is that ok?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:180:
ProduceDebugText(rate, delay_samples);
On 2017/02/09 17:02:59, aleloi wrote:
> This doesn't do anything: ProduceDebugText is a function that returns a string
> and has no side effects. It's used the same way in decimator_by_4_unittest.
> Should it be logged with SCOPED_TRACE?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:195: }
On 2017/02/09 17:02:59, aleloi wrote:
> The 'silence' updating construct seems a little strange. Only frames 100-109,
> 200-209, 300-309, 400-409 will have nonzero samples. Why do we start with 100
> blocks of silence?
> 
> Also, I think it's rather hard to understand what happens here. Can you change
> it to
> 
> for k = 0..:
>    const bool silence = <t/f condition calculated from 'k'>
> 
> ?
> 
> To keep it working as is, it would be 
> 
>  'const bool silence = k < 100 || (k % 100 >= 10)' 
> 
> which I think is much better (but still a bit complex).

I changed to your proposed one-linear.

The purpose of the silence and speech parts is to ensure that the comfort noise
generator has an estimated noise level that is lower than the non-silence parts,
as that will then allow us to detect the removal of those in the AEC output.

The initial silence of 100 frames is not important, it could as well  have been
the other way around.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:213:
echo_path_variability, false, x, &y);
On 2017/02/09 17:02:59, aleloi wrote:
> The remover only sees the last 250 of the samples. That seems wrong. I think
the
> remover should process all of the samples, but that energy levels should only
be
> counted starting from kNumBlocksToProcess/2.

Good find!

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:34: const
std::array<float, kFftLengthBy2Plus1>& X2 = render_spectrum;
On 2017/02/10 13:52:34, ivoc wrote:
> I would prefer const auto&.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:37: const float kX2Min =
44015068.0f;
On 2017/02/10 13:52:34, ivoc wrote:
> Where does this number come from? A comment would be helpful. Also constexpr.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:2: *  Copyright (c) 2016
The WebRTC project authors. All Rights Reserved.
On 2017/02/10 13:52:34, ivoc wrote:
> 2017

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:16: #include
"webrtc/base/array_view.h"
On 2017/02/10 13:52:34, ivoc wrote:
> Again, no ArrayViews in this file, but maybe there should be, WDYT?

Not in this case, since we know what the incoming sizes will be and not using
ArrayViews will allow us to rely on the compiler for verifying the correctness
of the input.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:18: #include
"webrtc/base/optional.h"
On 2017/02/10 13:52:34, ivoc wrote:
> Can be removed.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator_unittest.cc:30: {
On 2017/02/10 13:52:34, ivoc wrote:
> These lines reoccur a few times, you can consider turning them into a small
> function.

Good suggestion!
Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:27: const
std::array<float, kFftLengthBy2Plus1>& X2 = render_spectrum;
On 2017/02/10 13:52:35, ivoc wrote:
> I would prefer const auto&.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:31: const float kX2Min =
44015068.0f;
On 2017/02/10 13:52:34, ivoc wrote:
> Where does this value come from? A comment would be helpful. Also constexpr.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:56:
ErleEstimator::ErleEstimator() {
On 2017/02/10 13:52:34, ivoc wrote:
> Move the constructor and destructor to the top of the file please.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator.h:16: #include
"webrtc/base/array_view.h"
On 2017/02/10 13:52:35, ivoc wrote:
> See other remarks about ArrayView.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erle_estimator_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erle_estimator_unittest.cc:31: {
On 2017/02/10 13:52:35, ivoc wrote:
> These lines reoccur a few times, it might be a good idea to turn them into a
> small function.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/fft_data.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/fft_data.h:43: // Copy the data from an
interlieved array.
On 2017/02/09 17:02:59, aleloi wrote:
> interlieved -> interleaved (here and in some other places)

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:22: constexpr
float kHerrorInitial = 10000.f;
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> kHErrorInitial

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:44: const
FftBuffer& X_buffer = render_buffer;
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> You may want to comment something like "introduce shorter notation for better
> math readability".

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:63: // Do not
update the filter if the render is not sufficiently persistently
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> "sufficiently persistently" sounds weird.

I rephrased it. PTAL

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:96: // H_error =
H_error + K * erl.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> What is K?

PTAL

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.h:21: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Not used?

It is now, but I replaced it with a forward.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:19:
#include "webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h"
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Order of includes.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:77:
bool saturation =
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:82: if
(use_silent_render_in_second_half && k == num_blocks_to_process / 2) {
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Should this be k>=num_blocks_to_process/2?

Yes, good find!

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:218: //
error estimates causes the gain to increase after a period of saturation.
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> causes -> cause

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.cc:56: 
On 2017/02/13 16:44:50, aleloi wrote:
> Please make y2_sum, e2_main_sum, subtractor_output const

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.cc:73: }
On 2017/02/13 16:44:50, aleloi wrote:
> I think the control flow can be a little simplified here. WDYT about this
> change:
> 
> if (is_best != sub_output) counter++
> if (counter > 15) {
>    sub_output=...;
>    Transition(...);
>    counter=0;
> } else if (sub_output) copy(...);

Good point!

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:14: #include <algorithm>
On 2017/02/13 16:44:50, aleloi wrote:
> Not used in this header.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:18: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/13 16:44:50, aleloi wrote:
> Also not used here.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector.h:30: void
FormLinearOutput(rtc::ArrayView<const float> linear_aec_output,
On 2017/02/13 16:44:50, aleloi wrote:
> linear_aec_output is also called subtractor_output below. Maybe choose one
name
> for both to make it simpler to read in isolation?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/output_selector_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:36: auto
form_e_and_y = [&](bool y_equals_weaker) {
On 2017/02/13 16:44:51, aleloi wrote:
> const auto

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:23: for (size_t j = 0;
j < 20; ++j) {
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Why 20?

Tuning parameter. Ok?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:47: a.value =
kHInitial;
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Can't you just as well say
> a = CountedFloat(kHInitial, kUpdateCounterInitial);

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:67: :
delay_handler.ExternalDelay()
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Put () around the "else" result, for readability.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:73: // Compute the R2
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:75: if (!delay) {
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> If no delay, then calculate RecentMaximum. Care to explain why?

The purpose of this is to estimate the echo power in a reliable way such that it
is not underestimated. If the delay is not known, we do not know what render
frame it was that caused the echo signal in the microphone. Therefore, the
conservative choice of the maximum render power over the latest signals is
chosen as the render signal that generated the echo power seen in the microphone
signal.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:87:
std::transform(X2_active.begin(), X2_active.end(), Y2.begin(),
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Math comment, please.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:91: auto H2_updater =
[&](float a, CountedFloat b) {
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> What math does this lambda implement?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:93: b.counter = a >
b.value ? max_update_counter_value : b.counter - 1;
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> You're making this branch again just below. You may consider incorporating
> updating of b.counter in the if-else below.
> 
> if (a > b.value) {
>   b.value = a;
>   b.counter = max_update_counter_value;
> } else {
>   --b.counter;
>   if (b.counter <= 0) {  // I find it odd that the counter can be negative.
>     b.value = std::max(b.value * 0.9f, 1.f);
>   }
> }

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:107:
std::transform(H2_.begin(), H2_.end(), X2_active.begin(), S2->begin(),
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Math comment

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:110: std::array<float,
kFftLengthBy2Plus1> H2_tmp;
On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> Why do you calculate H2_tmp? It is not used.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model_unittest.cc:104: // Verify
that stronger echo paths are detected immediately
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc:15: #include
"webrtc/base/array_view.h"
On 2017/02/13 16:44:51, aleloi wrote:
> Not used here.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc:59: (*v)[k - 2] =
(*v)[k - 1] = (*v)[k] = (*v)[k + 2] = (*v)[k + 1] = 0.f;
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Nit: For a more natural readability I would suggest swapping k+1 and k+2.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.cc:59: (*v)[k - 2] =
(*v)[k - 1] = (*v)[k] = (*v)[k + 2] = (*v)[k + 1] = 0.f;
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Nit: For a more natural readability I would suggest swapping k+1 and k+2.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:14: #include
<stdio.h>
On 2017/02/13 16:44:51, aleloi wrote:
> Not used, I think.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:16: #include
<vector>
On 2017/02/13 16:44:51, aleloi wrote:
> Replace with std::array.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:18: #include
"webrtc/base/constructormagic.h"
On 2017/02/13 16:44:51, aleloi wrote:
> Not used. But perhaps it should? Why would we copy/assign a
> RenderSignalAnalyzer?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer.h:31: // Updates the
render signal analysis with the most recent render signal.
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> What does delay_partitions mean?

That is the delay measured in number partitions, similarly to sample_rate_hz, or
frame_size_ms

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc
(right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:31: void
ProduceSinusoidalSignal(int sample_rate_hz,
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Shorten the name to ProduceSinusoid.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:78:
std::for_each(mask.begin(), mask.end(), [](float a) { EXPECT_EQ(1.f, a); });
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Consider:
> EXPECT_TRUE(std::all_of(mask.begin(), mask.end(), [](float a) { a == 1.f; });

Nice one!

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:94: //
Verify that a sinusiod signal is detected as narrow bands.
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> This seems like a new TEST.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:103: //
Verify that a no bands are detected as narrow when the delay is unknown.
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Seems like yet another TEST.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:103: //
Verify that a no bands are detected as narrow when the delay is unknown.
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> Seems like yet another TEST.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:110: 
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> You are only testing with an input mask with all ones. But if I understand the
> implementation correctly, it should leave non-masked bins untouched. So, I'd
> like to see a test where a mask set to !=1 is verified to be untouched outside
> of the masked bins.

That is tested on line 99.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:34: // Do not
update the filter if the render is not sufficiently persistently
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> "sufficiently persistently"?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:47: const
std::array<float, kFftLengthBy2Plus1>& X2 =
On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> const auto& ?

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:26: return v;
On 2017/02/13 16:44:51, aleloi wrote:
> return {kMainFilterLength, kShadowFilterLength};
> 
> should do the same thing. Same for the else clause below.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:77: [&](float a, float b) {
return a - b * kScale; });
On 2017/02/13 16:44:51, aleloi wrote:
> Beginner question: why the scaling? Is it because it's missing in Ifft and FFT
> composed with Ifft is not identity?

It is not included in the ifft method. My guess is that it is not desired to do
the scaling if that is not strictly needed and that therefore that scaling needs
to be explicitly done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.cc:93: 
On 2017/02/13 16:44:51, aleloi wrote:
> I suggest putting the filtering & subtraction in a function to avoid having to
> almost identical blocks.

I did a variant of that. I think it is nice to see both parts together. Very
likely this code should be changed after the tuning is done.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:18: 
On 2017/02/13 16:44:51, aleloi wrote:
> stdio, memory not needed, <array> missing.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:48: std::vector<size_t>
NumBlocksInRenderSums() const;
On 2017/02/13 16:44:51, aleloi wrote:
> This could probably be a constexpr function if the return type is changed from
> std::vector and the filter length constants are constexpr.

That makes sense. This should be changed, but I prefer to keep that for another
CL.

If the current tuning holds, the shadow and main filters are equally long, which
means that they should be applied and updated jointly. Then this method will be
changed as well.

WDYT?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:52: return
std::max(kMainFilterLength, kShadowFilterLength);
On 2017/02/13 16:44:51, aleloi wrote:
> Can be a constexpr function if the constants are costexpr.

Not fully sure how I can make the constants constexpr. Either way I try, I get
different errors. Do you have any suggestion on how to accomplish that?

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:65: const size_t
kShadowFilterLength = 12;
On 2017/02/13 16:44:51, aleloi wrote:
> constexpr.

See above.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_output.h:27: #include
"webrtc/modules/audio_processing/utility/ooura_fft.h"
On 2017/02/09 17:02:59, aleloi wrote:
> I don't think this file needs all these includes.

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:25: void
RunSubtractorTest(int num_blocks_to_process,
On 2017/02/13 16:44:51, aleloi wrote:
> I can't find a reason for why echo_to_nearend_power shouldn't be a return
value.
> 

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:73: float y_power =
std::inner_product(y.begin(), y.end(), y.begin(), 0.f);
On 2017/02/13 16:44:51, aleloi wrote:
> const float *_power

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:129: //
blocks_with_echo_path_changes.push_back(99);
On 2017/02/13 16:44:51, aleloi wrote:
> remove comment

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:143: //
blocks_with_echo_path_changes.push_back(99);
On 2017/02/13 16:44:51, aleloi wrote:
> remove comment

Done.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:151:
EXPECT_GT(1.05f, echo_to_nearend_power);
On 2017/02/13 16:44:51, aleloi wrote:
> I think EXPECT_NEAR(a, b, 0.05f) can be used here.

Done.

[peah-webrtc, 2017-02-20 08:52:50]

Added a minor patch which fixes the failing memcheck test.

[peah-webrtc, 2017-02-20 23:29:07]

Added refactoring of the residual echo estimation code.

[peah-webrtc, 2017-02-20 23:29:19]

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

[commit-bot: I haz the power, 2017-02-20 23:29:22]

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

[peah-webrtc, 2017-02-20 23:41:37]

The CQ bit was unchecked by peah@webrtc.org

[peah-webrtc, 2017-02-20 23:42:34]

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

[commit-bot: I haz the power, 2017-02-20 23:42:44]

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-02-21 00:39:28]

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

[commit-bot: I haz the power, 2017-02-21 00:39:29]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-02-21 09:40:04]

Some comments on the files that I have now re-reviewed. LG in general.

I will take a look at files that I have not reviewed earlier.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:43: H_j.re[k] +=
X.re[k] * G.re[k] + X.im[k] * G.im[k];
On 2017/02/20 07:37:14, peah-webrtc wrote:
> On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > Please, comment on why the signs are this way. Normally, one would expect
> > x * y = (xr + i*xi) * (yr + i*yi) = (xr*yr - xi*yi) + i*(xr*yi + xi*yr)
> 
> It is the conjugate, I think. I added that to the equation.
> 
> PTAL

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:137:
ApplyFilter(X_buffer, H_, S);
On 2017/02/20 07:37:14, peah-webrtc wrote:
> On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > Why the extra function? You can implement it here.
> 
> The reason was that I was planning for it to be optimized. That optimization
has
> now been added.
> 
> PTAL.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/20 07:37:14, peah-webrtc wrote:
> On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > Forward declare.
> 
> That does not work, as the data dumper functionality is used in the methods in
> the header file.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:63: // Do not
update the filter if the render is not sufficiently persistently
On 2017/02/20 07:37:17, peah-webrtc wrote:
> On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> > "sufficiently persistently" sounds weird.
> 
> I rephrased it. PTAL

Better.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:23: for (size_t j = 0;
j < 20; ++j) {
On 2017/02/20 07:37:17, peah-webrtc wrote:
> On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> > Why 20?
> 
> Tuning parameter. Ok?

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:20: #include
"webrtc/base/atomicops.h"
Now you won't need this, right?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:64: // Receives
reports that known echo path changes have occured and takes asjust
"takes asjust"?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:72: // the usage of
filter statistics have been specified during the creation of
have -> has ('usage' is singular (or possibly uncountable))

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:82: // only be used
if the usage of filter statistics have been specified during
have -> has

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:75:
EXPECT_NEAR(S_C.re[j], S_SSE2.re[j], 0.00001f);
Will EXPECT_FLOAT_EQ work here? See
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid....

Here and below too.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:54:
fft.Spectrum(optimization_, &spectrum_buffer_[position_]);
I think it is a bit weird to have optimization_ as a state in the FftBuffer. How
about pass it as a parameter to FftBuffer::Insert, which is the only place it is
actually used?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.h:1: 
Blank line inserted.

[hlundin-webrtc, 2017-02-21 16:34:03]

I took a look at the files that seemed to have had no reviews so far (zero
comments). See comments inline. No biggies, mostly nits and minor comments.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:19: void
Aec3Fft::ZeroPaddedFft(rtc::ArrayView<const float> x, FftData* X) const {
Any reason why you are not using an std::array here? I mean, the size of the
input cannot change anyway.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:25: ooura_fft_.Fft(fft.data());
Why not call Fft(&fft, X), like you do in PaddedFft below?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:37: std::copy(x.begin(),
x.end(), fft.begin() + x.size());
I guess this should be x_old.size(). Not that it matters...

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.h:29: // Computes the Fft. Note
that both the input and output are modified.
Fft -> FFT

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:18: namespace {}  //
namespace
... but why?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:93:
std::for_each(X.re.begin(), X.re.end(), [](float a) { EXPECT_EQ(0.f, a); });
After googling around a bit, I think you can make these tests a bit more compact
by using some tricks from gmock (even though you are not mocking anything).

EXPECT_THAT(X.re, ::testing::Each(0.f));

See
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
and
http://stackoverflow.com/questions/12340470/compare-containers-with-googletest.

This is totally optional, since the code works well as it is.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:150:
EXPECT_NEAR(x_ref[j], x[j], 0.1f);
0.1 doesn't see to be very precise. Can you tighten the margin?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:32: // After an
amount of active render sumples for which an echo should have
sumples -> samples

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:49: void
HalfDuplecPowerEstimate(bool active_render,
Duplec -> Duplex

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:173: // Counte
the blocks since saturation.
Counte?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:180: // Count
the upon each other following blocks after saturation.
Read this comment one more time.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:14: #include
<algorithm>
<array> missing

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:19: #include
"webrtc/base/optional.h"
Not used.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:22: #include
"webrtc/modules/audio_processing/aec3/echo_path_variability.h"
Not used?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:23: #include
"webrtc/modules/audio_processing/aec3/erle_estimator.h"
Not used?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:24: #include
"webrtc/modules/audio_processing/aec3/erl_estimator.h"
Not used?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:26: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Not used.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator_unittest.cc:20:
namespace {}  // namespace
Why?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.cc:85: // Analysis
filterbank
.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:14: #include <stdio.h>
Used for what?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:16: #include <memory>
What do you need memory for?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:19: #include
"webrtc/base/array_view.h"
Not used.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:24: void
ProduceSinusoidalSignal(int sample_rate_hz,
ProduceSinusoid

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:58: //
Verifies that no comfort noise is added whent the gain is 1.
whent

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:110:
EXPECT_GT(e0_input, 1000.f * e0_output);
This reads backwards for me. I suggest
EXPECT_LT(e0_output, e0_input / 1000.f);
Note that the convention to have the expected value as first parameter is only
for *_EQ and *_NE tests.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:113: //
Verifies that the suppressor is able to pass through a a desired signal while
" a a"

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:40: std::array<float,
kFftLengthBy2 - 1>* previous_gain_squared,
You already have kFftLengthBy2Plus1 defined. Why not add kFftLengthBy2Minus1?
You would use it a lot in this file at least.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:159: [](float a) {
return a > 0.f ? 1.f / a : -1.f; });
Is there any risk that we get _really_ large results here, when a is very close
to 0? Should you compare with some threshold larger than 0? Just a thought, it
is up to you.
EDIT: I see on line 211 that the subsequent processing with this result is
capped by 1. So I guess this is not an issue.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:178: // Add masker for
neightboring bands.
neighboring

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:230:
std::copy(gain->begin() + 1, gain->end() - 1, previous_gain_squared->begin());
It's a bit confusing that you copy to "previous_gain_squared" from "gain"
(presumably not squared). Can you align the naming somewhat?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:255: if (optimization_
== Aec3Optimization::kNone) {
I think you should use switch instead:
switch (optimization_) {
#if defined(WEBRTC_ARCH_X86_FAMILY)
  case Aec3Optimization::kSse2: {
    ComputeGains_SSE2(...);
  }
#endif
  default: {
    ComputeGains(...);
  }
}

This has the benefit that if you add another type of optimization, there is no
risk of exiting this method without having computed anything at all. As an
alternative, you can omit the default case, and explicitly name both
alternatives. That way, the compiler will help you find all places that needs
attention when you add another optimization type.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:15: #include <vector>
Not used.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:25: void
ComputeGains_SSE2(
Why are these two non-member functions exposed in the h-file? For testing?
Consider wrapping them in an extra namespace, e.g., namespace
suppression_gain_helpers, or namespace aec3.
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain_unittest.cc:60:
EXPECT_NEAR(G2_old[j], G2_old_SSE2[j], 0.0000001f);
Would EXPECT_FLOAT_EQ work here?

[ivoc, 2017-02-21 17:26:02]

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:29: const float kX2Min =
44015068.0f / 1000.f;
On 2017/02/20 07:37:15, peah-webrtc wrote:
> On 2017/02/10 13:52:33, ivoc wrote:
> > A comment about where this value comes from would be helpful.
> 
> Good point! It has now been removed though.
> 
> Done.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:34: } else {
On 2017/02/20 07:37:15, peah-webrtc wrote:
> On 2017/02/10 13:52:33, ivoc wrote:
> > this can be "else if" to avoid the nested if. One thing that worries me a
bit
> in
> > this loop is the possibility for elements in |bands_with_reliable_filter| to
> not
> > be changed. if the "if" and the "else if" don't hold, the old value will
> remain.
> > Is that the intended behavior?
> 
> You definitely have a point. And in the tuning, this did not perform how I
> wanted. Therefore I have removed it.
> 
> Done.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:19: #include
"webrtc/base/optional.h"
On 2017/02/20 07:37:15, peah-webrtc wrote:
> On 2017/02/10 13:52:33, ivoc wrote:
> > I don't think Optional is used in this header.
> 
> You are right. But I have now changed it, so it is again used.
> 
> Done.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:45: [](float a,
float b) { return a + 0.1f * (b - a); });
On 2017/02/20 07:37:15, peah-webrtc wrote:
> On 2017/02/10 13:52:33, ivoc wrote:
> > Out of curiousity, what kind of filter is this?
> 
> This is a low-pass filter: Y2_smoothed(t)=0.9f*Y2_smoothed(t) + 0.1*Y2;

ah, I should have seen that :)

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:18: #include
"webrtc/base/array_view.h"
On 2017/02/20 07:37:15, peah-webrtc wrote:
> On 2017/02/10 13:52:34, ivoc wrote:
> > Looks like ArrayView is not used in this header, but perhaps it should be
used
> > on the interfaces (instead of array), for more flexibility. WDYT?
> 
> I prefer array as it will give a compile-time warning about the size which
means
> that DCHECKS on the size is not needed. Everything is anyway so tightly
coupled
> with the sizes so I don't the flexibility achieved by array-view could be used
> anyway.
> 
> WDYT?

Ok, sounds good.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:16: #include
"webrtc/base/array_view.h"
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/10 13:52:34, ivoc wrote:
> > Again, no ArrayViews in this file, but maybe there should be, WDYT?
> 
> Not in this case, since we know what the incoming sizes will be and not using
> ArrayViews will allow us to rely on the compiler for verifying the correctness
> of the input.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:13: #include
"webrtc/typedefs.h"
This should be with the other webrtc includes below, right?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:112: for (; j <
limit; ++j, ++H_j, ++X) {
I think this could potentially be faster if the inner loop was over k. That way
the memory that's being read would be in the cache more often. However, I'm sure
you've benchmarked this, so if it's fast enough now, let's leave it as is.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Include can be in the .cc, if you add a forward declaration.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:32: #if
defined(WEBRTC_ARCH_X86_FAMILY)
I think a possibly better option would be to use an #if/#else construction
inside the regular AdaptPartitions function, to use the SSE implementation when
the flag is set. That way the caller doesn't have to know or care if SSE is
available or not. The same applies for the other SSE function below.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:37: // Corresponds of WGN
of power -46 dBFS.
Corresponds to

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:67: // Do not
update the filter if the render is not persistently exciting of a
"persistently exciting of" sounds a bit strange to me, maybe "persistently
excited by"?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:37:
std::for_each(H2_.begin(), H2_.end(), [](CountedFloat& a) {
Could this be replaced by a call to H2_->fill()?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:47:
std::for_each(H2_.begin(), H2_.end(), [](CountedFloat& a) {
Same here?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:61: const
std::array<float, kFftLengthBy2Plus1> Y2 = capture_spectrum;
Is this necessary? it looks like the value of Y2 is not modified, so a reference
would be more efficient, right?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:15: #include <string>
Can be removed.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:16: #include <vector>
Can be removed.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
Can be removed.

[peah-webrtc, 2017-02-21 23:00:42]

Thanks again for the comments! I've uploaded a patch with changes in response to
those.

PTAL

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:43: H_j.re[k] +=
X.re[k] * G.re[k] + X.im[k] * G.im[k];
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> On 2017/02/20 07:37:14, peah-webrtc wrote:
> > On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > > Please, comment on why the signs are this way. Normally, one would expect
> > > x * y = (xr + i*xi) * (yr + i*yi) = (xr*yr - xi*yi) + i*(xr*yi + xi*yr)
> > 
> > It is the conjugate, I think. I added that to the equation.
> > 
> > PTAL
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:137:
ApplyFilter(X_buffer, H_, S);
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> On 2017/02/20 07:37:14, peah-webrtc wrote:
> > On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > > Why the extra function? You can implement it here.
> > 
> > The reason was that I was planning for it to be optimized. That optimization
> has
> > now been added.
> > 
> > PTAL.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> On 2017/02/20 07:37:14, peah-webrtc wrote:
> > On 2017/02/13 21:36:59, hlundin-webrtc wrote:
> > > Forward declare.
> > 
> > That does not work, as the data dumper functionality is used in the methods
in
> > the header file.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:29: const float kX2Min =
44015068.0f / 1000.f;
On 2017/02/21 17:26:00, ivoc wrote:
> On 2017/02/20 07:37:15, peah-webrtc wrote:
> > On 2017/02/10 13:52:33, ivoc wrote:
> > > A comment about where this value comes from would be helpful.
> > 
> > Good point! It has now been removed though.
> > 
> > Done.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:34: } else {
On 2017/02/21 17:26:00, ivoc wrote:
> On 2017/02/20 07:37:15, peah-webrtc wrote:
> > On 2017/02/10 13:52:33, ivoc wrote:
> > > this can be "else if" to avoid the nested if. One thing that worries me a
> bit
> > in
> > > this loop is the possibility for elements in |bands_with_reliable_filter|
to
> > not
> > > be changed. if the "if" and the "else if" don't hold, the old value will
> > remain.
> > > Is that the intended behavior?
> > 
> > You definitely have a point. And in the tuning, this did not perform how I
> > wanted. Therefore I have removed it.
> > 
> > Done.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.h:19: #include
"webrtc/base/optional.h"
On 2017/02/21 17:26:00, ivoc wrote:
> On 2017/02/20 07:37:15, peah-webrtc wrote:
> > On 2017/02/10 13:52:33, ivoc wrote:
> > > I don't think Optional is used in this header.
> > 
> > You are right. But I have now changed it, so it is again used.
> > 
> > Done.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.h:18: #include
"webrtc/base/array_view.h"
On 2017/02/21 17:26:01, ivoc wrote:
> On 2017/02/20 07:37:15, peah-webrtc wrote:
> > On 2017/02/10 13:52:34, ivoc wrote:
> > > Looks like ArrayView is not used in this header, but perhaps it should be
> used
> > > on the interfaces (instead of array), for more flexibility. WDYT?
> > 
> > I prefer array as it will give a compile-time warning about the size which
> means
> > that DCHECKS on the size is not needed. Everything is anyway so tightly
> coupled
> > with the sizes so I don't the flexibility achieved by array-view could be
used
> > anyway.
> > 
> > WDYT?
> 
> Ok, sounds good.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/erl_estimator.h:16: #include
"webrtc/base/array_view.h"
On 2017/02/21 17:26:01, ivoc wrote:
> On 2017/02/20 07:37:16, peah-webrtc wrote:
> > On 2017/02/10 13:52:34, ivoc wrote:
> > > Again, no ArrayViews in this file, but maybe there should be, WDYT?
> > 
> > Not in this case, since we know what the incoming sizes will be and not
using
> > ArrayViews will allow us to rely on the compiler for verifying the
correctness
> > of the input.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:63: // Do not
update the filter if the render is not sufficiently persistently
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> On 2017/02/20 07:37:17, peah-webrtc wrote:
> > On 2017/02/13 21:37:00, hlundin-webrtc wrote:
> > > "sufficiently persistently" sounds weird.
> > 
> > I rephrased it. PTAL
> 
> Better.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/power_echo_model.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:23: for (size_t j = 0;
j < 20; ++j) {
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> On 2017/02/20 07:37:17, peah-webrtc wrote:
> > On 2017/02/13 21:37:01, hlundin-webrtc wrote:
> > > Why 20?
> > 
> > Tuning parameter. Ok?
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:166: EXPECT_EQ(1.f,
echo_to_nearend_power);
On 2017/02/13 16:44:51, aleloi wrote:
> Does EXPECT_EQ use exact comparison for floats? Maybe use EXPECT_NEAR to not
> attract attention of the vengeful floating point deity?

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:13: #include
"webrtc/typedefs.h"
On 2017/02/21 17:26:01, ivoc wrote:
> This should be with the other webrtc includes below, right?

I'm not sure how to do that properly.
WEBRTC_ARCH_X86_FAMILY is defined in webrtc/typedefs.h, so in order for that #if
to work webrtc/typedefs.h must be included before emmintrin.h.
emmintrin.h is, on the other hand, a system c header file, which means it should
be included before any C++ and WebRTC includes.

Do you have any suggestion on how to solve that?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:20: #include
"webrtc/base/atomicops.h"
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> Now you won't need this, right?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:112: for (; j <
limit; ++j, ++H_j, ++X) {
On 2017/02/21 17:26:01, ivoc wrote:
> I think this could potentially be faster if the inner loop was over k. That
way
> the memory that's being read would be in the cache more often. However, I'm
sure
> you've benchmarked this, so if it's fast enough now, let's leave it as is.

I cannot really say that I've benchmarked it superwell as I haven't gotten a
reliable benchmark framework. I believe it was a bit slower though. But you
definitely have a point with this. The reason I chose to put the outer loop as
k, is that that reduces the amount of loadu operations (the G values are loaded
for each k). But maybe the resulting cache misses will be worse than that.

I'll keep it as it is for now, but let's look further into this!

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/21 17:26:01, ivoc wrote:
> Include can be in the .cc, if you add a forward declaration.

No, that is not possible since the DumpFilter method uses members of that class.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:32: #if
defined(WEBRTC_ARCH_X86_FAMILY)
On 2017/02/21 17:26:01, ivoc wrote:
> I think a possibly better option would be to use an #if/#else construction
> inside the regular AdaptPartitions function, to use the SSE implementation
when
> the flag is set. That way the caller doesn't have to know or care if SSE is
> available or not. The same applies for the other SSE function below.

That makes sense. I don't think it works though.

As it is now SSE2 instructions can be used if
1) WEBRTC_ARCH_X86_FAMILY is defined.
and
2) WebRtc_GetCPUInfo(kSSE2) does not return 0.

In order to do what is proposed, 2) needs to be called at each called to the
optimised method and I don't think that is a good thing.

A second advantage of having the optimized methods exposed in the header, and
separate from their unoptimised counterparts, is that the optimised code may be
benchmarked against the unoptimized code in unit tests. That would not be
possible if the SSE implementations were implemented inside the same function as
the unoptimized code.

In order to make the testing simple, and to make the selection of the optimized
code for running, this was the best approach I came up with. If you find a
better way, I'd be delighted though! (but due to 2, the approach suggested above
won't work).
WDYT?

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:64: // Receives
reports that known echo path changes have occured and takes asjust
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> "takes asjust"?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:72: // the usage of
filter statistics have been specified during the creation of
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> have -> has ('usage' is singular (or possibly uncountable))

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:82: // only be used
if the usage of filter statistics have been specified during
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> have -> has

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:75:
EXPECT_NEAR(S_C.re[j], S_SSE2.re[j], 0.00001f);
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> Will EXPECT_FLOAT_EQ work here? See
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid....
> 
> Here and below too.

Seems to work! Great suggestion! Thanks!

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:19: void
Aec3Fft::ZeroPaddedFft(rtc::ArrayView<const float> x, FftData* X) const {
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Any reason why you are not using an std::array here? I mean, the size of the
> input cannot change anyway.

I agree. The reason is that it is called at least once using a vector (inside
echo_remover.cc). Once we refactor out the vector-implementation of the block
implementation, we can, and should, do that change. I'll add a todo for that.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:25: ooura_fft_.Fft(fft.data());
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Why not call Fft(&fft, X), like you do in PaddedFft below?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.cc:37: std::copy(x.begin(),
x.end(), fft.begin() + x.size());
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> I guess this should be x_old.size(). Not that it matters...

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft.h:29: // Computes the Fft. Note
that both the input and output are modified.
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Fft -> FFT

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:18: namespace {}  //
namespace
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> ... but why?

:-)

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:93:
std::for_each(X.re.begin(), X.re.end(), [](float a) { EXPECT_EQ(0.f, a); });
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> After googling around a bit, I think you can make these tests a bit more
compact
> by using some tricks from gmock (even though you are not mocking anything).
> 
> EXPECT_THAT(X.re, ::testing::Each(0.f));
> 
> See
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> and
>
http://stackoverflow.com/questions/12340470/compare-containers-with-googletest.
> 
> This is totally optional, since the code works well as it is.

Nice!
For some reason, it does not build for me though: error: no member named 'Each'
in namespace 'testing'
Therefore, for now I'll keep it as it is, but let's adopt that version in an
upcoming CL.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:150:
EXPECT_NEAR(x_ref[j], x[j], 0.1f);
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> 0.1 doesn't see to be very precise. Can you tighten the margin?

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:37: // Corresponds of WGN
of power -46 dBFS.
On 2017/02/21 17:26:01, ivoc wrote:
> Corresponds to

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:54:
fft.Spectrum(optimization_, &spectrum_buffer_[position_]);
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> I think it is a bit weird to have optimization_ as a state in the FftBuffer.
How
> about pass it as a parameter to FftBuffer::Insert, which is the only place it
is
> actually used?

I agree, the optimization makes the code less nice. The reason for that
optimization needs to be used as this is that FftData is a pod, and I did not
burden that to have to remember what kind of optimization should be used for the
Spectrum method. 
However, apart from that I've strived not to let the code become more complex
due to the optimization functionality. Including it as a parameter to Insert,
puts the burden of the user of Insert to use that parameter correctly and would
furthermore make the Insert call less readable (why an insert-call needs to be
optimized is not immediately evident as the pre-computation of the Spectrum is a
side-effect of the Insert-call that is not really related to an Insert).

I'd prefer to let the optimization be a state variable of FftBuffer as that will
simplify the rest of the code. And I think it makes sense for FftBuffer to have
that stored.

WDYT? Is it ok to keep the optimization as it is?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.h:1: 
On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> Blank line inserted.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:67: // Do not
update the filter if the render is not persistently exciting of a
On 2017/02/21 17:26:01, ivoc wrote:
> "persistently exciting of" sounds a bit strange to me, maybe "persistently
> excited by"?

No, that is not really mathematically correct I think. What about is not
sufficiently excited?

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:37:
std::for_each(H2_.begin(), H2_.end(), [](CountedFloat& a) {
On 2017/02/21 17:26:01, ivoc wrote:
> Could this be replaced by a call to H2_->fill()?

Great suggestion!

That worked!!!

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:47:
std::for_each(H2_.begin(), H2_.end(), [](CountedFloat& a) {
On 2017/02/21 17:26:01, ivoc wrote:
> Same here?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.cc:61: const
std::array<float, kFftLengthBy2Plus1> Y2 = capture_spectrum;
On 2017/02/21 17:26:01, ivoc wrote:
> Is this necessary? it looks like the value of Y2 is not modified, so a
reference
> would be more efficient, right?

Ouch! That was a mistake! Good find. It should definitely be a reference.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:15: #include <string>
On 2017/02/21 17:26:01, ivoc wrote:
> Can be removed.

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:16: #include <vector>
On 2017/02/21 17:26:01, ivoc wrote:
> Can be removed.

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/power_echo_model.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/21 17:26:01, ivoc wrote:
> Can be removed.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:32: // After an
amount of active render sumples for which an echo should have
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> sumples -> samples

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:49: void
HalfDuplecPowerEstimate(bool active_render,
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Duplec -> Duplex

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:173: // Counte
the blocks since saturation.
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Counte?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:180: // Count
the upon each other following blocks after saturation.
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Read this comment one more time.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:14: #include
<algorithm>
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> <array> missing

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:19: #include
"webrtc/base/optional.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used.

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:22: #include
"webrtc/modules/audio_processing/aec3/echo_path_variability.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:23: #include
"webrtc/modules/audio_processing/aec3/erle_estimator.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:24: #include
"webrtc/modules/audio_processing/aec3/erl_estimator.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.h:26: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/residual_echo_estimator_unittest.cc:20:
namespace {}  // namespace
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Why?

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.cc:85: // Analysis
filterbank
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> .

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:14: #include <stdio.h>
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Used for what?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:16: #include <memory>
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> What do you need memory for?

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.h:19: #include
"webrtc/base/array_view.h"
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> Not used.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:24: void
ProduceSinusoidalSignal(int sample_rate_hz,
On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> ProduceSinusoid

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:58: //
Verifies that no comfort noise is added whent the gain is 1.
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> whent

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:110:
EXPECT_GT(e0_input, 1000.f * e0_output);
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> This reads backwards for me. I suggest
> EXPECT_LT(e0_output, e0_input / 1000.f);
> Note that the convention to have the expected value as first parameter is only
> for *_EQ and *_NE tests.

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter_unittest.cc:113: //
Verifies that the suppressor is able to pass through a a desired signal while
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> " a a"

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:40: std::array<float,
kFftLengthBy2 - 1>* previous_gain_squared,
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> You already have kFftLengthBy2Plus1 defined. Why not add kFftLengthBy2Minus1?
> You would use it a lot in this file at least.

Nice!
Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:159: [](float a) {
return a > 0.f ? 1.f / a : -1.f; });
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> Is there any risk that we get _really_ large results here, when a is very
close
> to 0? Should you compare with some threshold larger than 0? Just a thought, it
> is up to you.
> EDIT: I see on line 211 that the subsequent processing with this result is
> capped by 1. So I guess this is not an issue.

Yes, the division should actually be done as it is. But I think the separation
of doing this in separate steps is making this more difficult that it actually
is. This code should be optimized in upcoming CLs. Let's rewrite this part then.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:178: // Add masker for
neightboring bands.
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> neighboring

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:230:
std::copy(gain->begin() + 1, gain->end() - 1, previous_gain_squared->begin());
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> It's a bit confusing that you copy to "previous_gain_squared" from "gain"
> (presumably not squared). Can you align the naming somewhat?

I simplified this with an alias.
PTAL.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:255: if (optimization_
== Aec3Optimization::kNone) {
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> I think you should use switch instead:
> switch (optimization_) {
> #if defined(WEBRTC_ARCH_X86_FAMILY)
>   case Aec3Optimization::kSse2: {
>     ComputeGains_SSE2(...);
>   }
> #endif
>   default: {
>     ComputeGains(...);
>   }
> }
> 
> This has the benefit that if you add another type of optimization, there is no
> risk of exiting this method without having computed anything at all. As an
> alternative, you can omit the default case, and explicitly name both
> alternatives. That way, the compiler will help you find all places that needs
> attention when you add another optimization type.

Great one!
I also changed to this in the other places using optimizations.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:15: #include <vector>
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> Not used.

Done.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:25: void
ComputeGains_SSE2(
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> Why are these two non-member functions exposed in the h-file? For testing?
> Consider wrapping them in an extra namespace, e.g., namespace
> suppression_gain_helpers, or namespace aec3.
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

Yes, these are only there for the sake of the methods being possible to test.

I'll put these in the namespace aec3, as this is something that occurs in other
places as well.

PTAL

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain_unittest.cc:60:
EXPECT_NEAR(G2_old[j], G2_old_SSE2[j], 0.0000001f);
On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> Would EXPECT_FLOAT_EQ work here?

No, it would not, as the SSE2 sqrt method seems not to be that accurate.

[hlundin-webrtc, 2017-02-22 15:24:07]

Getting close now! New batch of comments, all of them minor or nits.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:93:
std::for_each(X.re.begin(), X.re.end(), [](float a) { EXPECT_EQ(0.f, a); });
On 2017/02/21 23:00:40, peah-webrtc wrote:
> On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> > After googling around a bit, I think you can make these tests a bit more
> compact
> > by using some tricks from gmock (even though you are not mocking anything).
> > 
> > EXPECT_THAT(X.re, ::testing::Each(0.f));
> > 
> > See
> >
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> > and
> >
>
http://stackoverflow.com/questions/12340470/compare-containers-with-googletest.
> > 
> > This is totally optional, since the code works well as it is.
> 
> Nice!
> For some reason, it does not build for me though: error: no member named
'Each'
> in namespace 'testing'
> Therefore, for now I'll keep it as it is, but let's adopt that version in an
> upcoming CL.

You will have to #include "webrtc/test/gmock.h" since (weirdly enough)
EXPECT_THAT and ::testing::Each are part of gmock. But still, totally optional.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:54:
fft.Spectrum(optimization_, &spectrum_buffer_[position_]);
On 2017/02/21 23:00:40, peah-webrtc wrote:
> On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> > I think it is a bit weird to have optimization_ as a state in the FftBuffer.
> How
> > about pass it as a parameter to FftBuffer::Insert, which is the only place
it
> is
> > actually used?
> 
> I agree, the optimization makes the code less nice. The reason for that
> optimization needs to be used as this is that FftData is a pod, and I did not
> burden that to have to remember what kind of optimization should be used for
the
> Spectrum method. 
> However, apart from that I've strived not to let the code become more complex
> due to the optimization functionality. Including it as a parameter to Insert,
> puts the burden of the user of Insert to use that parameter correctly and
would
> furthermore make the Insert call less readable (why an insert-call needs to be
> optimized is not immediately evident as the pre-computation of the Spectrum is
a
> side-effect of the Insert-call that is not really related to an Insert).
> 
> I'd prefer to let the optimization be a state variable of FftBuffer as that
will
> simplify the rest of the code. And I think it makes sense for FftBuffer to
have
> that stored.
> 
> WDYT? Is it ok to keep the optimization as it is?

Fair enough. You have convinced me.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:230:
std::copy(gain->begin() + 1, gain->end() - 1, previous_gain_squared->begin());
On 2017/02/21 23:00:41, peah-webrtc wrote:
> On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > It's a bit confusing that you copy to "previous_gain_squared" from "gain"
> > (presumably not squared). Can you align the naming somewhat?
> 
> I simplified this with an alias.
> PTAL.

Better.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:255: if (optimization_
== Aec3Optimization::kNone) {
On 2017/02/21 23:00:41, peah-webrtc wrote:
> On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > I think you should use switch instead:
> > switch (optimization_) {
> > #if defined(WEBRTC_ARCH_X86_FAMILY)
> >   case Aec3Optimization::kSse2: {
> >     ComputeGains_SSE2(...);
> >   }
> > #endif
> >   default: {
> >     ComputeGains(...);
> >   }
> > }
> > 
> > This has the benefit that if you add another type of optimization, there is
no
> > risk of exiting this method without having computed anything at all. As an
> > alternative, you can omit the default case, and explicitly name both
> > alternatives. That way, the compiler will help you find all places that
needs
> > attention when you add another optimization type.
> 
> Great one!
> I also changed to this in the other places using optimizations.

That was the way I hoped it would pan out. :)

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:25: void
ComputeGains_SSE2(
On 2017/02/21 23:00:41, peah-webrtc wrote:
> On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > Why are these two non-member functions exposed in the h-file? For testing?
> > Consider wrapping them in an extra namespace, e.g., namespace
> > suppression_gain_helpers, or namespace aec3.
> >
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...
> 
> Yes, these are only there for the sake of the methods being possible to test.
> 
> I'll put these in the namespace aec3, as this is something that occurs in
other
> places as well.
> 
> PTAL

Good.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain_unittest.cc:60:
EXPECT_NEAR(G2_old[j], G2_old_SSE2[j], 0.0000001f);
On 2017/02/21 23:00:41, peah-webrtc wrote:
> On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > Would EXPECT_FLOAT_EQ work here?
> 
> No, it would not, as the SSE2 sqrt method seems not to be that accurate.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec3_common.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_common.h:27: enum class
Aec3Optimization { kNone, kSse2, kNeon };
kNeon is not used yet, right? I suggest you remove it for now. You are not
handling it in DetectOptimization().

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:122:
std::accumulate(x.begin(), x.end(), 0.f,
Optional: Could also use inner_product, I suppose.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:126: :
echo_path_change_counter_ - 1;
What happens when echo_path_change_counter_ goes negative? Is that intentional?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: : active_render_counter_
- 1;
Same here.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:138: const std::array<float,
kFftLengthBy2Plus1>& X2 =
const auto&

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:154: headset_detected_ =
false;
Is this short-circuit because the above detection does not work? If so, please
comment that explicitly.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:60: // Returns the delay
estimate based on the linear filter,
, -> .

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:61: const
rtc::Optional<size_t>& FilterDelay() const { return filter_delay_; }
I think you should return rtc::Optional<size_t>, not a const reference.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:64: const
rtc::Optional<size_t>& ExternalDelay() const { return external_delay_; }
I think you should return rtc::Optional<size_t>, not a const reference.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:89: // Returns whether a
probably headset setup has been detected.
probably -> probable?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:234: //
TODO(peah): Re-enable the test once the issue with memory leaks during DEATH
Is this comment accurate?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erl_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:19: constexpr float
kMinErl = 0.01f;
Are these in dB?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:38: const float kX2Min =
44015068.0f;
constexpr

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.h:31: // Returns the most
recent ERL estimate.
Is this in dB?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.h:36: std::array<int,
kFftLengthBy2Plus1 - 2> hold_counters_;
kFftLengthBy2Minus1

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:19: constexpr float
kMinErle = 1.f;
In dB?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:40: const float kX2Min =
44015068.0f;
constexpr

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erle_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.h:32: // Returns the most
recent ERLE estimate.
In dB?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.h:37: std::array<int,
kFftLengthBy2Plus1 - 2> hold_counters_;
kFftLengthBy2Minus1

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/frame_blocker.cc:16: #include
"webrtc/modules/audio_processing/aec3/aec3_common.h"
Not needed, since it is included in .h file.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc
(right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:1: 
Empty line.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:34: }
DCHECK that averaging reached the desired end value after the loop.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:50: rtc::ArrayView<const
float>& e_main = subtractor_output;
RTC_DCHECK_EQ(subtractor_output.size(), capture.size());

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.h:19: // Performs the
selection betwween the which of the linear aec output and the
the which -> which

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:53:
EXPECT_EQ(selector.UseSubtractorOutput(), y_is_weakest);
Flip order of parameters.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:61:
EXPECT_EQ(selector.UseSubtractorOutput(), !y_is_weakest);
Flip order of parameters.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:17: #include
"webrtc/modules/audio_processing/aec3/aec3_common.h"
Not needed here since it is included in the .h file.

[aleloi, 2017-02-22 16:20:31]

A few comments, more coming later.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:90: "aec3/suppression_gain.h",
I guess this feedback comes a bit late in the review process, but it would
really make sense for aec3 to be its own build target. Or even consist of
several targets all defined in a new BUILD.gn file in its sub-directory. It
would take some work to decouple it from the rest (I'm volunteering :D). But in
reward, we will have faster compilation time, and protection from some kinds of
undesirable changes in the future.

Currently aec3 is an pretty nicely designed independent component that consists
of rather independent cooperating sub-components. To make it stay that way in
the future, the logical component division should be reflected in the build
system.

TLDR: it's fine as is, but cutting off pieces of the monolith some day would be
nice.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:208:
suppression_filter_.ApplyGain(comfort_noise, high_band_comfort_noise, G, y);
Is 'high_band_comfort_noise' needed at lower sample rates? Is it possible to
save on computing it?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.cc:62: if
(echo_path_variability.delay_change) {
Just 'if (echo_path_variability.delay_change) ' instead of the double 'if' would
behave equivalently.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
Please change to constexpr.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
The constant is called 'filter length' here, but 'size_partitions' in the filter
c-tor. I suggest choosing a common name.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:64: const size_t
kShadowFilterLength = 12;
Same here

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.cc:26: // fprintf(1,
'\t%.14f, %.14f, %.14f,\n', win);
This seems to be a few more than 32 numbers. It also goes from 0 to 1 and back
to 0, while [0 ; win(1:32)] goes from 0 to 1, I think. At least if Octave has
the same 'hanning()' as Matlab does. Maybe it's hanning(128)?

[ivoc, 2017-02-22 17:08:13]

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/21 23:00:40, peah-webrtc wrote:
> On 2017/02/21 17:26:01, ivoc wrote:
> > Include can be in the .cc, if you add a forward declaration.
> 
> No, that is not possible since the DumpFilter method uses members of that
class.

Ah, I missed that, sorry.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:32: #if
defined(WEBRTC_ARCH_X86_FAMILY)
On 2017/02/21 23:00:39, peah-webrtc wrote:
> On 2017/02/21 17:26:01, ivoc wrote:
> > I think a possibly better option would be to use an #if/#else construction
> > inside the regular AdaptPartitions function, to use the SSE implementation
> when
> > the flag is set. That way the caller doesn't have to know or care if SSE is
> > available or not. The same applies for the other SSE function below.
> 
> That makes sense. I don't think it works though.
> 
> As it is now SSE2 instructions can be used if
> 1) WEBRTC_ARCH_X86_FAMILY is defined.
> and
> 2) WebRtc_GetCPUInfo(kSSE2) does not return 0.
> 
> In order to do what is proposed, 2) needs to be called at each called to the
> optimised method and I don't think that is a good thing.
> 
> A second advantage of having the optimized methods exposed in the header, and
> separate from their unoptimised counterparts, is that the optimised code may
be
> benchmarked against the unoptimized code in unit tests. That would not be
> possible if the SSE implementations were implemented inside the same function
as
> the unoptimized code.
> 
> In order to make the testing simple, and to make the selection of the
optimized
> code for running, this was the best approach I came up with. If you find a
> better way, I'd be delighted though! (but due to 2, the approach suggested
above
> won't work).
> WDYT?
Ok, that's a good point. Let's keep it like this.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:67: // Do not
update the filter if the render is not persistently exciting of a
On 2017/02/21 23:00:40, peah-webrtc wrote:
> On 2017/02/21 17:26:01, ivoc wrote:
> > "persistently exciting of" sounds a bit strange to me, maybe "persistently
> > excited by"?
> 
> No, that is not really mathematically correct I think. What about is not
> sufficiently excited? 

Sounds good, thanks.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:24: #include
"webrtc/modules/audio_processing/aec3/comfort_noise_generator.h"
Order of includes

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:135:
data_dumper_->DumpWav("aec3_processblock_capture_input", kBlockSize, &y0[0],
Is this debug code?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:170: // Performs a
sanity check that the echo_remover is being able to properly
"being" can be safely removed, in my opinion.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:182:
ProduceDebugText(rate, delay_samples);
scoped trace?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/fft_data.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_data.h:58:
(*power_spectrum)[kFftLengthBy2] =
Is it necessary to compute this coefficient separate from the SSE loop? It looks
like the same calculation, right?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.cc:40:
RTC_DCHECK_EQ(h.size(), (h.size() / 4) * 4);
I would prefer a check for h.size() % 4 == 0.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.h:23: namespace aec3 {
Most things are not in this namespace, right? Why is this added?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:51:
std::vector<float> x(2000, 0.f);
I'm pretty sure vectors are default initialized to zero, so the ", 0.f" part is
optional (but I guess it's not a big deal to leave it).

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:72:
EXPECT_NEAR(error_sum, error_sum_SSE2, 10000.f);
Does this still count as being "near"? Is the difference really that large? Or
is this value just really really large?

[hlundin-webrtc, 2017-02-22 20:54:16]

Comments for echo_canceller3_unittest.cc.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:609: void
OptionalRenderBandSplit() {
OptionalRenderBandSplit looks exactly the same as OptionalCaptureBandSplit. What
am I missing?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:718: //
TODO(peah): Re-enable the test once the issue with memory leaks during DEATH
Wrong comment? This is not a death test.

[hlundin-webrtc, 2017-02-22 21:17:29]

Comments for Subtractor.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:52: return
std::max(kMainFilterLength, kShadowFilterLength);
On 2017/02/20 07:37:19, peah-webrtc wrote:
> On 2017/02/13 16:44:51, aleloi wrote:
> > Can be a constexpr function if the constants are costexpr.
> 
> Not fully sure how I can make the constants constexpr. Either way I try, I get
> different errors. Do you have any suggestion on how to accomplish that?

You will have to make them static constexpr. Then you should be able to  make
the function constexpr too, I believe.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.cc:62: if
(echo_path_variability.delay_change) {
On 2017/02/22 16:20:31, aleloi wrote:
> Just 'if (echo_path_variability.delay_change) ' instead of the double 'if'
would
> behave equivalently.

bool AudioPathChanged() const { return gain_change || delay_change; }

Yes, you are right. But maybe add a DCHECK for AudioPathChanged()? I don't know
what the future holds for the EchoPathVariability struct.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/22 16:20:31, aleloi wrote:
> Please change to constexpr.

static constexpr

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_output.h:22: struct
SubtractorOutput {
You could just as well include this as a struct inside the Subtractor class. Up
to you.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:80: EXPECT_LT(0.f,
y_power);
If you are going to be strict about this, you will have to do something like:
if (y_power == 0.f) {
  ADD_FAILURE();
  return -1.0;  // ... or whatever suitable value for error.
}
return output_power / y_power;

(This is the downside of changing this function to return something other than
void -- you can no longer ASSERT.)

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:140: float
echo_to_nearend_power;
Don't declare on a separate line.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:153: float
echo_to_nearend_power;
Don't declare on separate line.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:156:
EXPECT_NEAR(1.f, echo_to_nearend_power, 0.05);
EXPECT_FLOAT_EQ?

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:168: float
echo_to_nearend_power;
Don't declare on a separate line.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:171:
EXPECT_NEAR(1.f, echo_to_nearend_power, 0.0000001f);
EXPECT_FLOAT_EQ?

[hlundin-webrtc, 2017-02-22 21:17:32]

Comments for Subtractor.

[peah-webrtc, 2017-02-22 23:51:40]

Hi,
Thanks for the comments!

I've uploaded a new patch with the changes in response to the comments.

The new patch also changes the behavior of the OutputSelector class such that
the selection of the outputs is biased towards favoring the subtractor output.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/comfort_noise_generator.cc:45: [](float a,
float b) { return a + 0.1f * (b - a); });
On 2017/02/21 17:26:00, ivoc wrote:
> On 2017/02/20 07:37:15, peah-webrtc wrote:
> > On 2017/02/10 13:52:33, ivoc wrote:
> > > Out of curiousity, what kind of filter is this?
> > 
> > This is a low-pass filter: Y2_smoothed(t)=0.9f*Y2_smoothed(t) + 0.1*Y2;
> 
> ah, I should have seen that :)

No worries! Good question!

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:24: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/22 17:08:12, ivoc wrote:
> On 2017/02/21 23:00:40, peah-webrtc wrote:
> > On 2017/02/21 17:26:01, ivoc wrote:
> > > Include can be in the .cc, if you add a forward declaration.
> > 
> > No, that is not possible since the DumpFilter method uses members of that
> class.
> 
> Ah, I missed that, sorry.

Np.

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.h:32: #if
defined(WEBRTC_ARCH_X86_FAMILY)
On 2017/02/22 17:08:12, ivoc wrote:
> On 2017/02/21 23:00:39, peah-webrtc wrote:
> > On 2017/02/21 17:26:01, ivoc wrote:
> > > I think a possibly better option would be to use an #if/#else construction
> > > inside the regular AdaptPartitions function, to use the SSE implementation
> > when
> > > the flag is set. That way the caller doesn't have to know or care if SSE
is
> > > available or not. The same applies for the other SSE function below.
> > 
> > That makes sense. I don't think it works though.
> > 
> > As it is now SSE2 instructions can be used if
> > 1) WEBRTC_ARCH_X86_FAMILY is defined.
> > and
> > 2) WebRtc_GetCPUInfo(kSSE2) does not return 0.
> > 
> > In order to do what is proposed, 2) needs to be called at each called to the
> > optimised method and I don't think that is a good thing.
> > 
> > A second advantage of having the optimized methods exposed in the header,
and
> > separate from their unoptimised counterparts, is that the optimised code may
> be
> > benchmarked against the unoptimized code in unit tests. That would not be
> > possible if the SSE implementations were implemented inside the same
function
> as
> > the unoptimized code.
> > 
> > In order to make the testing simple, and to make the selection of the
> optimized
> > code for running, this was the best approach I came up with. If you find a
> > better way, I'd be delighted though! (but due to 2, the approach suggested
> above
> > won't work).
> > WDYT?
> Ok, that's a good point. Let's keep it like this.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_fft_unittest.cc:93:
std::for_each(X.re.begin(), X.re.end(), [](float a) { EXPECT_EQ(0.f, a); });
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:40, peah-webrtc wrote:
> > On 2017/02/21 16:34:02, hlundin-webrtc wrote:
> > > After googling around a bit, I think you can make these tests a bit more
> > compact
> > > by using some tricks from gmock (even though you are not mocking
anything).
> > > 
> > > EXPECT_THAT(X.re, ::testing::Each(0.f));
> > > 
> > > See
> > >
> >
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> > > and
> > >
> >
>
http://stackoverflow.com/questions/12340470/compare-containers-with-googletest.
> > > 
> > > This is totally optional, since the code works well as it is.
> > 
> > Nice!
> > For some reason, it does not build for me though: error: no member named
> 'Each'
> > in namespace 'testing'
> > Therefore, for now I'll keep it as it is, but let's adopt that version in an
> > upcoming CL.
> 
> You will have to #include "webrtc/test/gmock.h" since (weirdly enough)
> EXPECT_THAT and ::testing::Each are part of gmock. But still, totally
optional.

Ah! Thanks! That worked.

I changed to using what you suggested.

Done.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:54:
fft.Spectrum(optimization_, &spectrum_buffer_[position_]);
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:40, peah-webrtc wrote:
> > On 2017/02/21 09:40:03, hlundin-webrtc wrote:
> > > I think it is a bit weird to have optimization_ as a state in the
FftBuffer.
> > How
> > > about pass it as a parameter to FftBuffer::Insert, which is the only place
> it
> > is
> > > actually used?
> > 
> > I agree, the optimization makes the code less nice. The reason for that
> > optimization needs to be used as this is that FftData is a pod, and I did
not
> > burden that to have to remember what kind of optimization should be used for
> the
> > Spectrum method. 
> > However, apart from that I've strived not to let the code become more
complex
> > due to the optimization functionality. Including it as a parameter to
Insert,
> > puts the burden of the user of Insert to use that parameter correctly and
> would
> > furthermore make the Insert call less readable (why an insert-call needs to
be
> > optimized is not immediately evident as the pre-computation of the Spectrum
is
> a
> > side-effect of the Insert-call that is not really related to an Insert).
> > 
> > I'd prefer to let the optimization be a state variable of FftBuffer as that
> will
> > simplify the rest of the code. And I think it makes sense for FftBuffer to
> have
> > that stored.
> > 
> > WDYT? Is it ok to keep the optimization as it is?
> 
> Fair enough. You have convinced me.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/main_filter_update_gain.cc:67: // Do not
update the filter if the render is not persistently exciting of a
On 2017/02/22 17:08:12, ivoc wrote:
> On 2017/02/21 23:00:40, peah-webrtc wrote:
> > On 2017/02/21 17:26:01, ivoc wrote:
> > > "persistently exciting of" sounds a bit strange to me, maybe "persistently
> > > excited by"?
> > 
> > No, that is not really mathematically correct I think. What about is not
> > sufficiently excited? 
> 
> Sounds good, thanks.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:230:
std::copy(gain->begin() + 1, gain->end() - 1, previous_gain_squared->begin());
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:41, peah-webrtc wrote:
> > On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > > It's a bit confusing that you copy to "previous_gain_squared" from "gain"
> > > (presumably not squared). Can you align the naming somewhat?
> > 
> > I simplified this with an alias.
> > PTAL.
> 
> Better.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.h:25: void
ComputeGains_SSE2(
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:41, peah-webrtc wrote:
> > On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > > Why are these two non-member functions exposed in the h-file? For testing?
> > > Consider wrapping them in an extra namespace, e.g., namespace
> > > suppression_gain_helpers, or namespace aec3.
> > >
> >
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...
> > 
> > Yes, these are only there for the sake of the methods being possible to
test.
> > 
> > I'll put these in the namespace aec3, as this is something that occurs in
> other
> > places as well.
> > 
> > PTAL
> 
> Good.

Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain_unittest.cc:60:
EXPECT_NEAR(G2_old[j], G2_old_SSE2[j], 0.0000001f);
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:41, peah-webrtc wrote:
> > On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > > Would EXPECT_FLOAT_EQ work here?
> > 
> > No, it would not, as the SSE2 sqrt method seems not to be that accurate.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:90: "aec3/suppression_gain.h",
On 2017/02/22 16:20:31, aleloi wrote:
> I guess this feedback comes a bit late in the review process, but it would
> really make sense for aec3 to be its own build target. Or even consist of
> several targets all defined in a new BUILD.gn file in its sub-directory. It
> would take some work to decouple it from the rest (I'm volunteering :D). But
in
> reward, we will have faster compilation time, and protection from some kinds
of
> undesirable changes in the future.
> 
> Currently aec3 is an pretty nicely designed independent component that
consists
> of rather independent cooperating sub-components. To make it stay that way in
> the future, the logical component division should be reflected in the build
> system.
> 
> TLDR: it's fine as is, but cutting off pieces of the monolith some day would
be
> nice.

I agree that that makes sense.  However, I think we should postpone that until
another CL. Currently all other submodules to APM are also included in the BUILD
file in the same monolithic manner, and I think it makes sense to break out
those as well at the same time.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec3_common.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec3_common.h:27: enum class
Aec3Optimization { kNone, kSse2, kNeon };
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> kNeon is not used yet, right? I suggest you remove it for now. You are not
> handling it in DetectOptimization().

Done.

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:122:
std::accumulate(x.begin(), x.end(), 0.f,
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Optional: Could also use inner_product, I suppose.

I changed to that as it makes the code more compact. I'm concerned it could be
slower, as inner_product uses 2 iterators. But I think compactness is more
important in this case, since very likely we should anyway replace this by
SSE2/NEON versions.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:126: :
echo_path_change_counter_ - 1;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> What happens when echo_path_change_counter_ goes negative? Is that
intentional?

Yes, that is intentional, as allowing it to go below 0 avoids the need to check
to avoid that. This is reflected in the check below.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: : active_render_counter_
- 1;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Same here.

That is intended as well. The check is for when it is above 0. We should,
however, probably harmonize the way counters are used within AEC3 in a followup
cleanup CL.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:138: const std::array<float,
kFftLengthBy2Plus1>& X2 =
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> const auto&

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:154: headset_detected_ =
false;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Is this short-circuit because the above detection does not work? If so, please
> comment that explicitly.

That is correct, or rather, it does not work yet. I instead #ifdef that code
away for now.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:60: // Returns the delay
estimate based on the linear filter,
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> , -> .

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:61: const
rtc::Optional<size_t>& FilterDelay() const { return filter_delay_; }
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> I think you should return rtc::Optional<size_t>, not a const reference.

I adopt your suggested change, but I'm not convinced that is the right thing to
do.
If it is a const ref, it is cheap to use code-constructs as

size_t delay = aec_state.FilterDelay() ? *aec_state.FilterDelay() : 0.

Without a reference, that would mean that there are two copies of FilterDelay
being done in that statement.
Please have a look and let me know if you change your mind, in which case I'll
change this back.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:64: const
rtc::Optional<size_t>& ExternalDelay() const { return external_delay_; }
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> I think you should return rtc::Optional<size_t>, not a const reference.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:89: // Returns whether a
probably headset setup has been detected.
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> probably -> probable?

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:234: //
TODO(peah): Re-enable the test once the issue with memory leaks during DEATH
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Is this comment accurate?

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:609: void
OptionalRenderBandSplit() {
On 2017/02/22 20:54:16, hlundin-webrtc wrote:
> OptionalRenderBandSplit looks exactly the same as OptionalCaptureBandSplit.
What
> am I missing?

Good find! That is not indended. Since OptionalRenderBandSplit is not used, I
removed that method and renamed the other to a more proper name.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:718: //
TODO(peah): Re-enable the test once the issue with memory leaks during DEATH
On 2017/02/22 20:54:16, hlundin-webrtc wrote:
> Wrong comment? This is not a death test.

It is actually a death test, but the actual test is done within the methof
RunAnalyzeRenderFrameLengthCheckVerification.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:24: #include
"webrtc/modules/audio_processing/aec3/comfort_noise_generator.h"
On 2017/02/22 17:08:12, ivoc wrote:
> Order of includes

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:135:
data_dumper_->DumpWav("aec3_processblock_capture_input", kBlockSize, &y0[0],
On 2017/02/22 17:08:12, ivoc wrote:
> Is this debug code?

Yes, it is. The dumping of this data is only done if specified by a build flag.
Otherwise, that method call maps to an empty method.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:208:
suppression_filter_.ApplyGain(comfort_noise, high_band_comfort_noise, G, y);
On 2017/02/22 16:20:31, aleloi wrote:
> Is 'high_band_comfort_noise' needed at lower sample rates? Is it possible to
> save on computing it?

You are correct, it is only needed for rates including and above 32 kHz so
avoiding to compute that would lower the computational cost. I prefer to keep it
as this for now though, since the cost is very low for this, and since 16 and 8
kHz are not that commonly used.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:170: // Performs a
sanity check that the echo_remover is being able to properly
On 2017/02/22 17:08:13, ivoc wrote:
> "being" can be safely removed, in my opinion.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:182:
ProduceDebugText(rate, delay_samples);
On 2017/02/22 17:08:12, ivoc wrote:
> scoped trace?

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erl_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:19: constexpr float
kMinErl = 0.01f;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Are these in dB?

No, they are linear.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.cc:38: const float kX2Min =
44015068.0f;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> constexpr

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erl_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.h:31: // Returns the most
recent ERL estimate.
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> Is this in dB?

No, it is linear.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erl_estimator.h:36: std::array<int,
kFftLengthBy2Plus1 - 2> hold_counters_;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> kFftLengthBy2Minus1

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erle_estimator.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:19: constexpr float
kMinErle = 1.f;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> In dB?

No, it is linear.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.cc:40: const float kX2Min =
44015068.0f;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> constexpr

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/erle_estimator.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.h:32: // Returns the most
recent ERLE estimate.
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> In dB?

No, it is linear.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/erle_estimator.h:37: std::array<int,
kFftLengthBy2Plus1 - 2> hold_counters_;
On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> kFftLengthBy2Minus1

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/fft_data.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_data.h:58:
(*power_spectrum)[kFftLengthBy2] =
On 2017/02/22 17:08:13, ivoc wrote:
> Is it necessary to compute this coefficient separate from the SSE loop? It
looks
> like the same calculation, right?

There is no gain in doing this in the SSE loop as the SSE2 operations operate on
blocks of 4 floats. Since the array is 65 long, that leaves the last operation
to operate on a 1 float long block. Therefore, I partitioned it such that the
SSE loop takes care of the first 64 values (divides evenly by 4) and value 65 is
computed separately.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/frame_blocker.cc:16: #include
"webrtc/modules/audio_processing/aec3/aec3_common.h"
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> Not needed, since it is included in .h file.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.cc:40:
RTC_DCHECK_EQ(h.size(), (h.size() / 4) * 4);
On 2017/02/22 17:08:13, ivoc wrote:
> I would prefer a check for h.size() % 4 == 0.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.h:23: namespace aec3 {
On 2017/02/22 17:08:13, ivoc wrote:
> Most things are not in this namespace, right? Why is this added?

It was suggested by another reviewer to follow the style guideline on nonlocal
methods that are not part of a class.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc
(right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:1: 
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> Empty line.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:51:
std::vector<float> x(2000, 0.f);
On 2017/02/22 17:08:13, ivoc wrote:
> I'm pretty sure vectors are default initialized to zero, so the ", 0.f" part
is
> optional (but I guess it's not a big deal to leave it).

Good point! 

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:72:
EXPECT_NEAR(error_sum, error_sum_SSE2, 10000.f);
On 2017/02/22 17:08:13, ivoc wrote:
> Does this still count as being "near"? Is the difference really that large? Or
> is this value just really really large?

The absolute difference is large as the values are really large. To illustrate
this better, I made the margin larger instead.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:34: }
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> DCHECK that averaging reached the desired end value after the loop.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:50: rtc::ArrayView<const
float>& e_main = subtractor_output;
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> RTC_DCHECK_EQ(subtractor_output.size(), capture.size());

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.h:19: // Performs the
selection betwween the which of the linear aec output and the
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> the which -> which

Done.

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:53:
EXPECT_EQ(selector.UseSubtractorOutput(), y_is_weakest);
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> Flip order of parameters.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector_unittest.cc:61:
EXPECT_EQ(selector.UseSubtractorOutput(), !y_is_weakest);
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> Flip order of parameters.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:17: #include
"webrtc/modules/audio_processing/aec3/aec3_common.h"
On 2017/02/22 15:24:07, hlundin-webrtc wrote:
> Not needed here since it is included in the .h file.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.cc:62: if
(echo_path_variability.delay_change) {
On 2017/02/22 21:17:28, hlundin-webrtc wrote:
> On 2017/02/22 16:20:31, aleloi wrote:
> > Just 'if (echo_path_variability.delay_change) ' instead of the double 'if'
> would
> > behave equivalently.
> 
> bool AudioPathChanged() const { return gain_change || delay_change; }
> 
> Yes, you are right. But maybe add a DCHECK for AudioPathChanged()? I don't
know
> what the future holds for the EchoPathVariability struct.

You are both correct. I will adopt the proposal by aleloi@.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.cc:62: if
(echo_path_variability.delay_change) {
On 2017/02/22 16:20:31, aleloi wrote:
> Just 'if (echo_path_variability.delay_change) ' instead of the double 'if'
would
> behave equivalently.

You are both correct. I will adopt the proposal by aleloi@.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/22 16:20:31, aleloi wrote:
> The constant is called 'filter length' here, but 'size_partitions' in the
filter
> c-tor. I suggest choosing a common name.

Good point!

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/22 16:20:31, aleloi wrote:
> Please change to constexpr.

I cannot get that to work as I get linker errors. I don't get fully clear on why
that happens, some references claim it is a clang bug, and some claims it is due
to the definition of constexpr. I'll keep it as it is for now.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/22 16:20:31, aleloi wrote:
> Please change to constexpr.

I cannot get that to work as I get linker errors. I don't get fully clear on why
that happens, some references claim it is a clang bug, and some claims it is due
to the definition of constexpr. I'll keep it as it is for now.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:64: const size_t
kShadowFilterLength = 12;
On 2017/02/22 16:20:31, aleloi wrote:
> Same here

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_output.h:22: struct
SubtractorOutput {
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> You could just as well include this as a struct inside the Subtractor class.
Up
> to you.

I don't think that works, since SubtractorOutput is used by
MainFilterUpdateGain. That would cause main_filter_update_gain.h to have to
include subtractor.h, which itself includes main_filter_update_gain.h which
means that there is a problem with a circular inclusion.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:80: EXPECT_LT(0.f,
y_power);
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> If you are going to be strict about this, you will have to do something like:
> if (y_power == 0.f) {
>   ADD_FAILURE();
>   return -1.0;  // ... or whatever suitable value for error.
> }
> return output_power / y_power;
> 
> (This is the downside of changing this function to return something other than
> void -- you can no longer ASSERT.)

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:140: float
echo_to_nearend_power;
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> Don't declare on a separate line.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:153: float
echo_to_nearend_power;
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> Don't declare on separate line.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:156:
EXPECT_NEAR(1.f, echo_to_nearend_power, 0.05);
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> EXPECT_FLOAT_EQ?

No, not in this case. A bigger difference than that is expected.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:168: float
echo_to_nearend_power;
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> Don't declare on a separate line.

Done.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_unittest.cc:171:
EXPECT_NEAR(1.f, echo_to_nearend_power, 0.0000001f);
On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> EXPECT_FLOAT_EQ?

No, not in this case. A bigger difference than that is expected.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/suppression_filter.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_filter.cc:26: // fprintf(1,
'\t%.14f, %.14f, %.14f,\n', win);
On 2017/02/22 16:20:31, aleloi wrote:
> This seems to be a few more than 32 numbers. It also goes from 0 to 1 and back
> to 0, while [0 ; win(1:32)] goes from 0 to 1, I think. At least if Octave has
> the same 'hanning()' as Matlab does. Maybe it's hanning(128)?

True. Good find!
Done.

[peah-webrtc, 2017-02-22 23:53:53]

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

[commit-bot: I haz the power, 2017-02-22 23:53:58]

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-02-23 00:36:17]

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

[commit-bot: I haz the power, 2017-02-23 00:36:18]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-02-23 07:42:09]

I did not look at the following files:
aec_state_unittest.cc
comfort_noise_generator*
echo_remover*
fft_data*
matched_filter.{h,cc}
matcher_filter_unittest.cc

All the other files LGTM!
Stellar work!

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:126: :
echo_path_change_counter_ - 1;
On 2017/02/22 23:51:37, peah-webrtc wrote:
> On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > What happens when echo_path_change_counter_ goes negative? Is that
> intentional?
> 
> Yes, that is intentional, as allowing it to go below 0 avoids the need to
check
> to avoid that. This is reflected in the check below.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:154: headset_detected_ =
false;
On 2017/02/22 23:51:37, peah-webrtc wrote:
> On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > Is this short-circuit because the above detection does not work? If so,
please
> > comment that explicitly.
> 
> That is correct, or rather, it does not work yet. I instead #ifdef that code
> away for now.
> 
> Done.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:61: const
rtc::Optional<size_t>& FilterDelay() const { return filter_delay_; }
On 2017/02/22 23:51:37, peah-webrtc wrote:
> On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > I think you should return rtc::Optional<size_t>, not a const reference.
> 
> I adopt your suggested change, but I'm not convinced that is the right thing
to
> do.
> If it is a const ref, it is cheap to use code-constructs as
> 
> size_t delay = aec_state.FilterDelay() ? *aec_state.FilterDelay() : 0.
> 
> Without a reference, that would mean that there are two copies of FilterDelay
> being done in that statement.
> Please have a look and let me know if you change your mind, in which case I'll
> change this back.
> 
> Done.

Hmm. Yeah, you are probably right. Feel free to change back.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.h:23: namespace aec3 {
On 2017/02/22 23:51:39, peah-webrtc wrote:
> On 2017/02/22 17:08:13, ivoc wrote:
> > Most things are not in this namespace, right? Why is this added?
> 
> It was suggested by another reviewer to follow the style guideline on nonlocal
> methods that are not part of a class.

Yes. They were exposed outside of the .cc for testability, but I didn't want
them to be exposed to all of ::webrtc.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/22 23:51:39, peah-webrtc wrote:
> On 2017/02/22 16:20:31, aleloi wrote:
> > Please change to constexpr.
> 
> I cannot get that to work as I get linker errors. I don't get fully clear on
why
> that happens, some references claim it is a clang bug, and some claims it is
due
> to the definition of constexpr. I'll keep it as it is for now.

Keep it as it is now, but I wonder if the reason it won't work is because
std::max takes a reference to its parameters. And, taking a reference to a
static const member won't work, unless it is defined somewhere (to give it an
addressable place in memory).

So, in the .cc file, you will have to add 
static constexpr size_t Subtractor::kMainFilterLength;

Then I believe it should work. See:
http://stackoverflow.com/questions/272900/undefined-reference-to-static-class...
http://stackoverflow.com/questions/185844/initializing-private-static-members

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_output.h:22: struct
SubtractorOutput {
On 2017/02/22 23:51:40, peah-webrtc wrote:
> On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> > You could just as well include this as a struct inside the Subtractor class.
> Up
> > to you.
> 
> I don't think that works, since SubtractorOutput is used by
> MainFilterUpdateGain. That would cause main_filter_update_gain.h to have to
> include subtractor.h, which itself includes main_filter_update_gain.h which
> means that there is a problem with a circular inclusion.

Acknowledged.

[peah-webrtc, 2017-02-23 08:57:45]

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

[commit-bot: I haz the power, 2017-02-23 08:57:59]

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

[peah-webrtc, 2017-02-23 09:02:36]

The CQ bit was unchecked by peah@webrtc.org

[peah-webrtc, 2017-02-23 09:02:45]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-02-23 09:02:45]

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/2678423005/#ps240001 (title:
"Corrected the unittest for the OutputSelector")

[commit-bot: I haz the power, 2017-02-23 09:02:57]

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

[peah-webrtc, 2017-02-23 09:03:04]

The CQ bit was unchecked by peah@webrtc.org

[peah-webrtc, 2017-02-23 09:04:51]

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

[commit-bot: I haz the power, 2017-02-23 09:04:56]

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

[hlundin-webrtc, 2017-02-23 09:12:00]

The tuning in PS8-9 LGTM.

[commit-bot: I haz the power, 2017-02-23 10:22:49]

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

[commit-bot: I haz the power, 2017-02-23 10:22:51]

Dry run: This issue passed the CQ dry run.

[ivoc, 2017-02-23 10:33:50]

On 2017/02/23 09:12:00, hlundin-webrtc wrote:
> The tuning in PS8-9 LGTM.

LGTM. I didn't look at a bunch of files, but I trust hlundin/aleloi's judgement
on those :)

[aleloi, 2017-02-23 10:56:45]

A few comments more.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:35: #include
"webrtc/modules/audio_processing/logging/apm_data_dumper.h"
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/13 16:44:50, aleloi wrote:
> > Include order
> 
> But this one is correctly ordered, right?

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:118:
std::vector<std::vector<float>>* y = capture;
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/13 16:44:50, aleloi wrote:
> > Is it infeasible to change the inner vectors to array<float, kBlockSize>?
That
> > would affect everything starting from FrameBlocker, right?
> 
> We should definitely do that. But as you point out, that needs to start from
the
> top. Ok if we take that in another CL?
> 

SGTM

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:119: 
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/13 16:44:50, aleloi wrote:
> > A few general comments:
> > 
> > I think it would improve readability somewhat to declare some of the arrays
> > closer to point of use.
> > 
> > Also, perhaps consider inner scopes. Both to limit what parts of the program
> an
> > array is visible in, and to structure the control flow a bit. E.g. 'G' can
be
> > declared, computed and used in its own scope.
> 
> I definitely think you have a point. However, doing that will increase the
size
> of the core of the functionality. As it is now, having the variables declared
on
> top. The actual functionality of the method is visible on one single page on
> screen, which I really like as it increases readability.
> 
> WDYT? Is it ok to have it as it is?

SGTM

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover.cc:172: [](float a, float b)
-> float { return a + b * b; });
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/09 17:02:59, aleloi wrote:
> > Maybe use std::inner_product? Less lambdas is simpler.
> 
> Inner product looks perfect for this. I have some concerns, however, that the
> inner product might be more expensive as it uses two iterators.
> 
> I broke this out into a separate method for now that we definitely should have
> an optimized version for later on. I kept the accumulate for now though.
> 
> WDYT, can inner_product be more expensive than accumulate when it operates on
> one single vector?

I think you are right: 

https://godbolt.org/g/ZVubS0

Both the accumulate and the inner product version use 128 bit registers and loop
out-rolling, but the inner product one also uses a 32-bit register for
something. Also, the accumulate outrolls more: it handles 32 words in the loop
body. The inner product does 16.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:170:
TEST(EchoRemover, BasicEchoRemoval) {
On 2017/02/20 07:37:16, peah-webrtc wrote:
> On 2017/02/09 17:02:59, aleloi wrote:
> > In this test, the near-end signal is comprised entirely of echo. Is it
> possible
> > to write similar test/tests for echo + noise?
> 
> Absolutely, but I'm not sure if we should have that here/yet as the AEC
> functionality is still being developed. I prefer to stay with having  this
> sanity check test for now and instead consider adding more refined tests
later.
> Is that ok?

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:208:
suppression_filter_.ApplyGain(comfort_noise, high_band_comfort_noise, G, y);
On 2017/02/22 23:51:37, peah-webrtc wrote:
> On 2017/02/22 16:20:31, aleloi wrote:
> > Is 'high_band_comfort_noise' needed at lower sample rates? Is it possible to
> > save on computing it?
> 
> You are correct, it is only needed for rates including and above 32 kHz so
> avoiding to compute that would lower the computational cost. I prefer to keep
it
> as this for now though, since the cost is very low for this, and since 16 and
8
> kHz are not that commonly used.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3.cc:114:
RTC_DCHECK_EQ(num_bands, frame->size());
Suggest renaming into 'CopyLowestBandIntoFrame' or similar. Also, please add a
DCHECK for (*frame)[0].size().

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:55: bool
subtractor_output_is_best = BlockPower(y) > 1.5f * BlockPower(e_main);
const bool subtractor_output_is_best

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.h:19: // Performs the
selection betwween which of the linear aec output and the
betwween -> between

[peah-webrtc, 2017-02-23 11:18:33]

Hi,
Thanks for the feedback!
I've uploaded a new patch.
PTAL

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

https://codereview.webrtc.org/2678423005/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:255: if (optimization_
== Aec3Optimization::kNone) {
On 2017/02/22 15:24:05, hlundin-webrtc wrote:
> On 2017/02/21 23:00:41, peah-webrtc wrote:
> > On 2017/02/21 16:34:03, hlundin-webrtc wrote:
> > > I think you should use switch instead:
> > > switch (optimization_) {
> > > #if defined(WEBRTC_ARCH_X86_FAMILY)
> > >   case Aec3Optimization::kSse2: {
> > >     ComputeGains_SSE2(...);
> > >   }
> > > #endif
> > >   default: {
> > >     ComputeGains(...);
> > >   }
> > > }
> > > 
> > > This has the benefit that if you add another type of optimization, there
is
> no
> > > risk of exiting this method without having computed anything at all. As an
> > > alternative, you can omit the default case, and explicitly name both
> > > alternatives. That way, the compiler will help you find all places that
> needs
> > > attention when you add another optimization type.
> > 
> > Great one!
> > I also changed to this in the other places using optimizations.
> 
> That was the way I hoped it would pan out. :)

:-)
Acknowledged.

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

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:126: :
echo_path_change_counter_ - 1;
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:37, peah-webrtc wrote:
> > On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > > What happens when echo_path_change_counter_ goes negative? Is that
> > intentional?
> > 
> > Yes, that is intentional, as allowing it to go below 0 avoids the need to
> check
> > to avoid that. This is reflected in the check below.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.cc:154: headset_detected_ =
false;
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:37, peah-webrtc wrote:
> > On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > > Is this short-circuit because the above detection does not work? If so,
> please
> > > comment that explicitly.
> > 
> > That is correct, or rather, it does not work yet. I instead #ifdef that code
> > away for now.
> > 
> > Done.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/aec_state.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/aec_state.h:61: const
rtc::Optional<size_t>& FilterDelay() const { return filter_delay_; }
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:37, peah-webrtc wrote:
> > On 2017/02/22 15:24:06, hlundin-webrtc wrote:
> > > I think you should return rtc::Optional<size_t>, not a const reference.
> > 
> > I adopt your suggested change, but I'm not convinced that is the right thing
> to
> > do.
> > If it is a const ref, it is cheap to use code-constructs as
> > 
> > size_t delay = aec_state.FilterDelay() ? *aec_state.FilterDelay() : 0.
> > 
> > Without a reference, that would mean that there are two copies of
FilterDelay
> > being done in that statement.
> > Please have a look and let me know if you change your mind, in which case
I'll
> > change this back.
> > 
> > Done.
> 
> Hmm. Yeah, you are probably right. Feel free to change back.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_remover.cc:208:
suppression_filter_.ApplyGain(comfort_noise, high_band_comfort_noise, G, y);
On 2017/02/23 10:56:45, aleloi wrote:
> On 2017/02/22 23:51:37, peah-webrtc wrote:
> > On 2017/02/22 16:20:31, aleloi wrote:
> > > Is 'high_band_comfort_noise' needed at lower sample rates? Is it possible
to
> > > save on computing it?
> > 
> > You are correct, it is only needed for rates including and above 32 kHz so
> > avoiding to compute that would lower the computational cost. I prefer to
keep
> it
> > as this for now though, since the cost is very low for this, and since 16
and
> 8
> > kHz are not that commonly used.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/matched_filter.h:23: namespace aec3 {
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:39, peah-webrtc wrote:
> > On 2017/02/22 17:08:13, ivoc wrote:
> > > Most things are not in this namespace, right? Why is this added?
> > 
> > It was suggested by another reviewer to follow the style guideline on
nonlocal
> > methods that are not part of a class.
> 
> Yes. They were exposed outside of the .cc for testability, but I didn't want
> them to be exposed to all of ::webrtc.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterLength = 12;
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:39, peah-webrtc wrote:
> > On 2017/02/22 16:20:31, aleloi wrote:
> > > Please change to constexpr.
> > 
> > I cannot get that to work as I get linker errors. I don't get fully clear on
> why
> > that happens, some references claim it is a clang bug, and some claims it is
> due
> > to the definition of constexpr. I'll keep it as it is for now.
> 
> Keep it as it is now, but I wonder if the reason it won't work is because
> std::max takes a reference to its parameters. And, taking a reference to a
> static const member won't work, unless it is defined somewhere (to give it an
> addressable place in memory).
> 
> So, in the .cc file, you will have to add 
> static constexpr size_t Subtractor::kMainFilterLength;
> 
> Then I believe it should work. See:
>
http://stackoverflow.com/questions/272900/undefined-reference-to-static-class...
> http://stackoverflow.com/questions/185844/initializing-private-static-members

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor_output.h (right):

https://codereview.webrtc.org/2678423005/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor_output.h:22: struct
SubtractorOutput {
On 2017/02/23 07:42:09, hlundin-webrtc wrote:
> On 2017/02/22 23:51:40, peah-webrtc wrote:
> > On 2017/02/22 21:17:29, hlundin-webrtc wrote:
> > > You could just as well include this as a struct inside the Subtractor
class.
> > Up
> > > to you.
> > 
> > I don't think that works, since SubtractorOutput is used by
> > MainFilterUpdateGain. That would cause main_filter_update_gain.h to have to
> > include subtractor.h, which itself includes main_filter_update_gain.h which
> > means that there is a problem with a circular inclusion.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/echo_canceller3.cc:114:
RTC_DCHECK_EQ(num_bands, frame->size());
On 2017/02/23 10:56:45, aleloi wrote:
> Suggest renaming into 'CopyLowestBandIntoFrame' or similar. Also, please add a
> DCHECK for (*frame)[0].size().

Done.

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.cc (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.cc:55: bool
subtractor_output_is_best = BlockPower(y) > 1.5f * BlockPower(e_main);
On 2017/02/23 10:56:45, aleloi wrote:
> const bool subtractor_output_is_best

Done.

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/output_selector.h (right):

https://codereview.webrtc.org/2678423005/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/output_selector.h:19: // Performs the
selection betwween which of the linear aec output and the
On 2017/02/23 10:56:45, aleloi wrote:
> betwween -> between

Done.

[peah-webrtc, 2017-02-23 11:18:58]

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

[commit-bot: I haz the power, 2017-02-23 11:19:12]

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-02-23 11:23:18]

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

[commit-bot: I haz the power, 2017-02-23 11:23:19]

Dry run: Try jobs failed on following builders:
  linux_arm on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/6072)
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[peah-webrtc, 2017-02-23 11:59:35]

Description was changed from

==========
Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

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

to

==========
Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

TBR=aleloi@webrtc.org
BUG=webrtc:6018
==========

[peah-webrtc, 2017-02-23 11:59:52]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-02-23 11:59:53]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2678423005/#ps300001 (title:
"Fixed compilation error")

[commit-bot: I haz the power, 2017-02-23 11:59: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-02-23 12:06:29]

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

[commit-bot: I haz the power, 2017-02-23 12:06:31]

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

[peah-webrtc, 2017-02-23 12:23:30]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-02-23 12:23:44]

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-02-23 12:37:26]

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

[commit-bot: I haz the power, 2017-02-23 12:37:28]

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

[peah-webrtc, 2017-02-23 13:13:01]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-02-23 13:13:17]

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-02-23 13:16:28]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487855581118420,
"parent_rev": "1e7732c3d9c1b8d4d3fa7f7c2c94ba383c50a1e5", "commit_rev":
"522d71bf3605e99f4a2b5fd253d5ccd171575e7d"}

[commit-bot: I haz the power, 2017-02-23 13:16:31]

Description was changed from

==========
Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

TBR=aleloi@webrtc.org
BUG=webrtc:6018
==========

to

==========
Finalization of the first version of EchoCanceller 3

This CL adds the remaining code for the first version of EchoCanceller3.

TBR=aleloi@webrtc.org
BUG=webrtc:6018

Review-Url: https://codereview.webrtc.org/2678423005
Cr-Commit-Position: refs/heads/master@{#16801}
Committed:
https://chromium.googlesource.com/external/webrtc/+/522d71bf3605e99f4a2b5fd25...
==========

[commit-bot: I haz the power, 2017-02-23 13:16:34]

Committed patchset #12 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/522d71bf3605e99f4a2b5fd25...

[aleloi, 2017-02-23 15:16:01]

A few comments more.

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:48: std::vector<size_t>
NumBlocksInRenderSums() const;
On 2017/02/20 07:37:19, peah-webrtc wrote:
> On 2017/02/13 16:44:51, aleloi wrote:
> > This could probably be a constexpr function if the return type is changed
from
> > std::vector and the filter length constants are constexpr.
> 
> That makes sense. This should be changed, but I prefer to keep that for
another
> CL.
> 
> If the current tuning holds, the shadow and main filters are equally long,
which
> means that they should be applied and updated jointly. Then this method will
be
> changed as well.
> 
> WDYT?

sgtm

https://codereview.webrtc.org/2678423005/diff/70001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/subtractor.h:52: return
std::max(kMainFilterLength, kShadowFilterLength);
On 2017/02/22 21:17:28, hlundin-webrtc wrote:
> On 2017/02/20 07:37:19, peah-webrtc wrote:
> > On 2017/02/13 16:44:51, aleloi wrote:
> > > Can be a constexpr function if the constants are costexpr.
> > 
> > Not fully sure how I can make the constants constexpr. Either way I try, I
get
> > different errors. Do you have any suggestion on how to accomplish that?
> 
> You will have to make them static constexpr. Then you should be able to  make
> the function constexpr too, I believe.

Acknowledged.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/fft_buffer.cc (right):

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:56: // Pre-compute and cachec
the spectral sums.
cachec -> cache

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:58:
spectrum_buffer_[position_].end(), spectral_sums_[0].begin());
Does this caching really save time? I profiled something similar recently where
3 frames were added (instead of 12), and there was no noticeable improvement.
Code would be a little simpler if all calculation is done in the loop.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.cc:63: 
IMO readability improvement (optional): compute the position by

position = position_;
for j=1..:
  position = (condition) ? position + 1 : 0;
  // spectrum update


Alternatively update the position inside the loop header:

for (position = ..., j=...; condition; ++j, position = ...)

pro: scope of 'position' limited to loop.
con: more syntactic overhead.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/fft_buffer.h (right):

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.h:28: // sums in the call to
SpectralSum.
Comment about the vector seemed unclear to me. Suggested rewording: "vector
contains values |num_ffts| that the SpectralSum method of the created instance
will support."

That didn't sound perfect either.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer.h:44: // Returns the sum of the
spectrums for a certain number of FFTs.
Suggestion: change comment to "Returns the sum of the spectrums of the
|num_ffts| most recent FFTs."

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc (right):

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc:27:
EXPECT_DEATH(FftBuffer(Aec3Optimization::kNone, 1, std::vector<size_t>(2, 1)),
This
FftBuffer(Aec3Optimization::kNone, 1, {1, 1})

saves reading/thinking time :)

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc:32:
EXPECT_DEATH(FftBuffer(Aec3Optimization::kNone, 1, std::vector<size_t>()),
Same here.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc:38: TEST(FftBuffer,
FeasibleNumberOfFftsInSum) {
Perhaps 
Feasible -> InFeasible
?
Otherwise naming inconsistent with the two test names above.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc:39:
EXPECT_DEATH(FftBuffer(Aec3Optimization::kNone, 1, std::vector<size_t>(1, 2)),
And here.

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/fft_buffer_unittest.cc:49:
std::vector<size_t>(1, kBufferSize));
vector<...>(...) --> {kBufferSize}

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec3/subtractor.h (right):

https://codereview.webrtc.org/2678423005/diff/300001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec3/subtractor.h:63: const size_t
kMainFilterSizePartitions = 12;
Here's how to make it constexpr:

* declare/define as 'static constexpr size_t ... = 12' here in the .h file
* make sure the compiler allocates memory by writing 'constexpr size_t
Subtractor:...' in the .cc file.