Major AEC3 render pipeline changes

Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much 
of the rest of the AEC3 files. Most of these are, however, caused by 
a change of class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

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

[peah-webrtc, 2017-03-29 20:16:26]

Description was changed from

==========
Changed the render pipeline


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

to

==========
Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much of
the rest of the AEC3 files. Most of these are, however, caused by a change of
class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

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

[peah-webrtc, 2017-03-29 20:17:05]

Description was changed from

==========
Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much of
the rest of the AEC3 files. Most of these are, however, caused by a change of
class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

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

to

==========
Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much 
of the rest of the AEC3 files. Most of these are, however, caused by 
a change of class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

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

[peah-webrtc, 2017-03-30 05:36:53]

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

[peah-webrtc, 2017-03-30 05:36:57]

Hi,
This is the CL for the AEC3 render pipeline changes. There are many files, but
most of those are only changed by a class renaming.

I've added comments in the first patch. Unfortunately though, I needed to make a
tiny buffer length adjustment which caused a second patch to be needed. I
suggest, however, that you first review the first patch to easily be able to see
the comments, and then the second patch.

Best,
Per

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter_unittest.cc:13: //
TODO(peah): Reactivate once the next CL has landed.
Since the changes in this CL breaks the code in this unittest, and since the
next CL will require even further changes to this code, I took the liberty to
deactivate the code in order to reduce the amount of rebasing, and the amount of
code review required.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/aec3_common.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec3_common.h:34: constexpr int
kAdaptiveFilterLength = 12;
The adaptive filter lengths has now been fixed into a constant (which is how it
is actually used). This change will take effect in the next CL, and the code
will be further refactored accordingly after that.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec3_common.h:48: constexpr size_t
kNumMatchedFilters = 4;
The matched filter number, as well as the alignment of that, has now as well
been set as a constant. That makes sense, as it is how it is actually used.
Apart from that, however, it was a necessity as the lengths of the render
buffers are now are no longer fully independent

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/block_processor.cc (left):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/block_processor.cc:51: constexpr size_t
kRenderBufferSize = 250;
These constants are now moved to aec3_common.h

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/block_processor.cc:23: enum class
BlockProcessorApiCall { kCapture, kRender };
This enum is used in order to allow logging of the API call order.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/block_processor.cc:138: void
BlockProcessorImpl::BufferRender(std::vector<std::vector<float>>* block) {
I chose to remove the boolean return value as there is nothing sensible that the
caller can do to handle an under/overrun. Therefore, it is solely up to the echo
canceller functionality to handle this case properly.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:146:
TEST(BlockProcessor, DISABLED_SubmoduleIntegration) {
I doubt that we should maintain the integration tests any longer. BlockProcessor
is now too complex and contains too much state to do this in a sensible manner.
Let's keep this as they are, and then look into refactoring these. Therefore, I
don't think too much effort should go into reviewing this, and the previous,
test.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/decimator_by_4.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/decimator_by_4.h:29: void
Decimate(rtc::ArrayView<const float> in, rtc::ArrayView<float> out);
This is needed due to that now multi-band render blocks are processed in the
render pipeline. I expect this to be changed back once the multi-band blocks are
changed from being vectors of vectors.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h:21: struct
DownsampledRenderBuffer {
This is a bit too simple to keep in a separate file. However, I always ended up
in circular include-dependencies if I did not place it in a separate file.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_canceller3.cc:21: enum class
EchoCanceller3ApiCall { kCapture, kRender };
Used to record the API call sequence.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_canceller3.cc:198:
static_cast<void>(render_transfer_queue_->Insert(&render_queue_input_frame_));
Ignore return value, as any overrun will be implicitly detected further down
into the AEC.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_canceller3.h:74: void
AnalyzeRender(AudioBuffer* farend);
A bool is no longer returned as there is no sensible action to take when the
analysis was not successful.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_canceller3.h:100: void
EmptyRenderQueue();
A bool is no longer returned as there is no sensible action to take when the
emptying was not successful.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (left):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:649: for (auto
rate : {8000, 16000, 32000, 48000}) {
I chose to simply remove the multi-band rates, as the band-split filter makes
checking these trickier, and not at all suited for the current test
implementation.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (left):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:25: constexpr
size_t kMatchedFilterWindowSizeSubBlocks = 32;
This is now moved to aec3_common.h

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:52:
std::array<float, kSubBlockSize> downsampled_render;
The downsampling of the render is now done in the render buffer.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:38:
std::unique_ptr<RenderDelayBuffer> render_delay_buffer(
These tests are basically the same, and the changes only reflect the new render
pipeline implementation.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (left):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_remover.cc:147:
power_echo_model_.HandleEchoPathChange(echo_path_variability);
I removed this since it caused a build error, and will anyway be removed in the
next CL.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_remover.cc:183: //
power_echo_model_.EstimateEcho(render_buffer, Y2, aec_state_, &S2_power);
This will be removed in the next CL.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/main_filter_update_gain_unittest.cc:13: //
TODO(peah): Reactivate once the next CL has landed.
Since the changes in this CL breaks the code in this unittest, and since the
next CL will require even further changes to this code, I took the liberty to
deactivate the code in order to reduce the amount of rebasing, and the amount of
code review required.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_buffer.cc:61: FftData X;
The FFT needed to be moved here, as the render buffer is now controller outside
from echo remover. I think that makes sense from more aspects as well.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_buffer.h:43: const
std::vector<std::vector<float>>& MostRecentBlock() const {
As the render buffer how now been moved from the echo_remover and is instead
passed as a parameter, the most recent block is now stored as part of this
buffer as well.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:27: class
ApiCallJitterBuffer {
The purpose of this class is to handle jitter in the incoming capture and render
calls.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:110: std::max(30,
kAdaptiveFilterLength),
The lengths used in the constructor should be refactored to be compile-time
constants. It does not make sense to have them optional like this. I save that
for a later CL though.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:136: // Report
buffer overrun and let the caller handle the overrun.
An overruns is reliably detected when the api call jitter buffer overflows. The
buffer length of the swapqueue is also adjusted so that if that overflows, this
buffer overflows as well, which allows overflow to be reliably detected. The
overlow is, however, handled outside of this buffer as more actions than buffer
readjustments need to be taken.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:146: // Update the
buffers with a new block if such is available, otherwise repeat
A render buffer underrun can only happen if
1) The render thread has not yet started playing out samples.
2) There is clock-drift in which case the underrun can only consist of at most 1
block.

The proper way to address 2) is to re-add the previous block. which is done here
for that case. That so happens also to work well for 1).

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:155:
downsampled_render_buffer_.position =
Add new data to the render and downsampled_render buffers.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:180: if
((buffer_.size() - 1) < delay) {
This buffer readjustment is needed in order to avoid lost frames, or clockdrift,
to cause the buffer delay to be too long. The main issue with that is that a
very long buffer is needed to achieve that delay, and that the delay estimator
needs to cover that delay.

In order to avoid that, this code "rewinds" the buffer delays to be reasonable
and to be covered by the delay estimator.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_buffer.h:51: virtual const
RenderBuffer& GetRenderBuffer() const = 0;
This buffer now also owns the render buffer and downsampled render buffers. The
reason for this is that it needs to be able to access those in order to properly
handle under and overruns. Furthermore, as the delay is also imposed on the
render buffer, it makes sense from that perspective as well to have this buffer
here.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_delay_controller.cc:92: // If a the
delay set does not match the actual delay, reset the delay
The controller produces a delay in GetDelay, but if for some reason that delay
was not chosen, which is seen by this setdelay call, the controller must be
reset and the new delay applied.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/render_signal_analyzer_unittest.cc:12: 
Since the changes in this CL breaks the code in this unittest, and since the
next CL will require even further changes to this code, I took the liberty to
deactivate the code in order to reduce the amount of rebasing, and the amount of
code review required.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain_unittest.cc:14:
#if 0
Since the changes in this CL breaks the code in this unittest, and since the
next CL will require even further changes to this code, I took the liberty to
deactivate the code in order to reduce the amount of rebasing, and the amount of
code review required.

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2784023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:573:
!config_.echo_canceller3.enabled) {
This change causes the splitting filter to always be used when AEC3 is active
(and the sample rate is higher than 8 kHz).

[peah-webrtc, 2017-03-30 07:36:16]

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

[commit-bot: I haz the power, 2017-03-30 07:36:26]

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

[commit-bot: I haz the power, 2017-03-30 07:43:56]

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

[commit-bot: I haz the power, 2017-03-30 07:43:57]

Dry run: Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24389)

[peah-webrtc, 2017-03-30 11:48:15]

Sorry, but there were two lurking errors. One build error and one failing
unittest.

Those have now been fixed in the new small patches.

PTAL

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

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

[commit-bot: I haz the power, 2017-03-30 12:01:39]

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

[commit-bot: I haz the power, 2017-03-30 12:18:40]

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

[commit-bot: I haz the power, 2017-03-30 12:18:41]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)

[ivoc, 2017-03-31 13:58:32]

Looks pretty good overall! See some comments below.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:126: old_delay !=
new_delay ||
The formatting looks a bit funny here, is this really what 'git cl format' does?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:135:
render_buffer_overrun_occurred_ = false;
I think it's a bit more clear if this is moved into the if
(render_buffer_overrun_occurred) block.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.cc:19:
DownsampledRenderBuffer::~DownsampledRenderBuffer() {}
= default?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:42:
std::vector<float>(kBlockSize, 0.f));
I think the second argument: ", 0.f" can be omitted here, since the default
value is 0.f. This same pattern is used multiple times in this file.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/matched_filter.h:77: // Updates the
correlation with the values in the capture.
I think it looks a bit strange now. I could live with just "in capture" (as in,
the variable), or: the capture buffer/array.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:60: // Compute the FFT of
the data in the lowest band.
A DCHECK to verify the size of block would be useful.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:66:
std::copy(block[k].begin(), block[k].end(), last_block_[k].begin());
A DHECK for the size of block[k] would be good.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:76: // Pre-compute and
cachec the spectral sums.
cache

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:88: position = position <
(fft_buffer_.size() - 1) ? position + 1 : 0;
I think it would be nicer to update position in a similar way to the previous,
so position = (position_ + 1) % fft_buffer_.size();

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.h:27: // The constructor
takes apart from the other parameters a
This is unclear to me, how about something like this: "Besides the other
parameters, the contructor takes a vector ..."?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:20: namespace {} 
// namespace
Can be removed?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:24: // Verifies
that the check for that the provided numbers of Ffts to include in
The comment is hard to read, my suggestion: Verifies the check for the provided
number of FFTs to include in the spectral sum.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:38: // the
spectral is feasible works.
Also hard to read, consider simplifying.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:41: void
Insert(std::vector<std::vector<float>>* block) {
const ref

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:70: // TODO(peah):
Reduce the size to kMaxApiCallsJitterBlocks.
Can be removed.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:190:
(last_insert_index_ - (delay - (buffer_.size() - 1)) + buffer_.size()) %
If delay is sufficiently large (larger that 2 * buffer_.size()), this can result
in an underflow, right?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.h:41: // true if there
was not overrun, otherwise returns false.
.. if there was no overrun, ...

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/suppression_gain.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/suppression_gain.h:18: #include
"webrtc/modules/audio_processing/aec3/render_buffer.h"
This include is not actually used in this header, so I think it should be
removed.

[peah-webrtc, 2017-04-03 08:02:33]

Thanks for the comments!

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

PTAL

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:126: old_delay !=
new_delay ||
On 2017/03/31 13:58:31, ivoc wrote:
> The formatting looks a bit funny here, is this really what 'git cl format'
does?

Agree, that looks superstrange! I'll do a reformat on that using cl format.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:135:
render_buffer_overrun_occurred_ = false;
On 2017/03/31 13:58:31, ivoc wrote:
> I think it's a bit more clear if this is moved into the if
> (render_buffer_overrun_occurred) block.

I agree. There is, however, an issue with this code that is fixed in an upcoming
CL. The else, statement should always be run, regardless of whether the
if-statement passed.

That means that the render_buffer_overrun_occurred_ variable will be used in the
ProcessCapture call. Therefore, to avoid the risk of this being reset to early,
I'd prefer to keep it at the end of the method.

WDYT?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.cc:19:
DownsampledRenderBuffer::~DownsampledRenderBuffer() {}
On 2017/03/31 13:58:31, ivoc wrote:
> = default?

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:42:
std::vector<float>(kBlockSize, 0.f));
On 2017/03/31 13:58:31, ivoc wrote:
> I think the second argument: ", 0.f" can be omitted here, since the default
> value is 0.f. This same pattern is used multiple times in this file.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/matched_filter.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/matched_filter.h:77: // Updates the
correlation with the values in the capture.
On 2017/03/31 13:58:31, ivoc wrote:
> I think it looks a bit strange now. I could live with just "in capture" (as
in,
> the variable), or: the capture buffer/array.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:60: // Compute the FFT of
the data in the lowest band.
On 2017/03/31 13:58:31, ivoc wrote:
> A DCHECK to verify the size of block would be useful.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:66:
std::copy(block[k].begin(), block[k].end(), last_block_[k].begin());
On 2017/03/31 13:58:31, ivoc wrote:
> A DHECK for the size of block[k] would be good.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:76: // Pre-compute and
cachec the spectral sums.
On 2017/03/31 13:58:31, ivoc wrote:
> cache

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:88: position = position <
(fft_buffer_.size() - 1) ? position + 1 : 0;
On 2017/03/31 13:58:31, ivoc wrote:
> I think it would be nicer to update position in a similar way to the previous,
> so position = (position_ + 1) % fft_buffer_.size();

I agree that the code becomes nicer. However, the % adds a division, which in
this case would mean that 12 (spectral_sums_length_==12) divisions are added per
block if we do that. Therefore, I think that this construct is better, even
though it definitely is less readable.

The difference to what is done on line 70, is that it is not done inside a loop
so the total cost is less. I think, however, that it is not worth the
computational cost even then so I'll change that one to a conditional as well.

WDYT?

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.h:27: // The constructor
takes apart from the other parameters a
On 2017/03/31 13:58:31, ivoc wrote:
> This is unclear to me, how about something like this: "Besides the other
> parameters, the contructor takes a vector ..."?

Great!
I modified it along your suggestion.
PTAL

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:20: namespace {} 
// namespace
On 2017/03/31 13:58:31, ivoc wrote:
> Can be removed?

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:24: // Verifies
that the check for that the provided numbers of Ffts to include in
On 2017/03/31 13:58:31, ivoc wrote:
> The comment is hard to read, my suggestion: Verifies the check for the
provided
> number of FFTs to include in the spectral sum.

Nice!

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer_unittest.cc:38: // the
spectral is feasible works.
On 2017/03/31 13:58:31, ivoc wrote:
> Also hard to read, consider simplifying.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:41: void
Insert(std::vector<std::vector<float>>* block) {
On 2017/03/31 13:58:31, ivoc wrote:
> const ref

Nice!

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:70: // TODO(peah):
Reduce the size to kMaxApiCallsJitterBlocks.
On 2017/03/31 13:58:31, ivoc wrote:
> Can be removed.

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:190:
(last_insert_index_ - (delay - (buffer_.size() - 1)) + buffer_.size()) %
On 2017/03/31 13:58:32, ivoc wrote:
> If delay is sufficiently large (larger that 2 * buffer_.size()), this can
result
> in an underflow, right?

Hmm, that is true. In practice that cannot happen though. I cleaned up this code
to make what happens more explicit. I also added a DCHECK for this case.

PTAL

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.h:41: // true if there
was not overrun, otherwise returns false.
On 2017/03/31 13:58:32, ivoc wrote:
> .. if there was no overrun, ...

Done.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/suppression_gain.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/suppression_gain.h:18: #include
"webrtc/modules/audio_processing/aec3/render_buffer.h"
On 2017/03/31 13:58:32, ivoc wrote:
> This include is not actually used in this header, so I think it should be
> removed.

Good find. 

Done.

[ivoc, 2017-04-03 16:09:07]

LGTM, I had another look and I couldn't find much to complain about. See one new
remark below.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:126: old_delay !=
new_delay ||
On 2017/04/03 08:02:32, peah-webrtc wrote:
> On 2017/03/31 13:58:31, ivoc wrote:
> > The formatting looks a bit funny here, is this really what 'git cl format'
> does?
> 
> Agree, that looks superstrange! I'll do a reformat on that using cl format.

Looks like it didn't change, so maybe this is correct after all.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:135:
render_buffer_overrun_occurred_ = false;
On 2017/04/03 08:02:32, peah-webrtc wrote:
> On 2017/03/31 13:58:31, ivoc wrote:
> > I think it's a bit more clear if this is moved into the if
> > (render_buffer_overrun_occurred) block.
> 
> I agree. There is, however, an issue with this code that is fixed in an
upcoming
> CL. The else, statement should always be run, regardless of whether the
> if-statement passed.
> 
> That means that the render_buffer_overrun_occurred_ variable will be used in
the
> ProcessCapture call. Therefore, to avoid the risk of this being reset to
early,
> I'd prefer to keep it at the end of the method.
> 
> WDYT?

Ok, that sounds reasonable.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.cc:88: position = position <
(fft_buffer_.size() - 1) ? position + 1 : 0;
On 2017/04/03 08:02:33, peah-webrtc wrote:
> On 2017/03/31 13:58:31, ivoc wrote:
> > I think it would be nicer to update position in a similar way to the
previous,
> > so position = (position_ + 1) % fft_buffer_.size();
> 
> I agree that the code becomes nicer. However, the % adds a division, which in
> this case would mean that 12 (spectral_sums_length_==12) divisions are added
per
> block if we do that. Therefore, I think that this construct is better, even
> though it definitely is less readable.
> 
> The difference to what is done on line 70, is that it is not done inside a
loop
> so the total cost is less. I think, however, that it is not worth the
> computational cost even then so I'll change that one to a conditional as well.
> 
> WDYT?
> 
> 

Ok, let's keep it this way then.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_buffer.h:27: // The constructor
takes apart from the other parameters a
On 2017/04/03 08:02:33, peah-webrtc wrote:
> On 2017/03/31 13:58:31, ivoc wrote:
> > This is unclear to me, how about something like this: "Besides the other
> > parameters, the contructor takes a vector ..."?
> 
> Great!
> I modified it along your suggestion.
> PTAL

Ok, looks good now.

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:190:
(last_insert_index_ - (delay - (buffer_.size() - 1)) + buffer_.size()) %
On 2017/04/03 08:02:33, peah-webrtc wrote:
> On 2017/03/31 13:58:32, ivoc wrote:
> > If delay is sufficiently large (larger that 2 * buffer_.size()), this can
> result
> > in an underflow, right?
> 
> Hmm, that is true. In practice that cannot happen though. I cleaned up this
code
> to make what happens more explicit. I also added a DCHECK for this case.
> 
> PTAL

Great, it looks much better now.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h:24:
std::array<float, kDownsampledRenderBufferSize> buffer;
I think it should work to add " = {}" to initialize this array with zeros,
instead of in the constructor. You could possibly remove the explicit
constructor/destructor and the .cc file?

[aleloi, 2017-04-04 12:18:50]

A few comments.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:138: void
BlockProcessorImpl::BufferRender(std::vector<std::vector<float>>* block) {
I think 'block' can be changed to const ref if it is changed in
RenderDelayBuffer::Insert().

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:153: }
There is a similar guard in ProcessCapture. Does this mean that either an
initial capture or a render block will always be lost?

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h:24:
std::array<float, kDownsampledRenderBufferSize> buffer;
On 2017/04/03 16:09:07, ivoc wrote:
> I think it should work to add " = {}" to initialize this array with zeros,
> instead of in the constructor. You could possibly remove the explicit
> constructor/destructor and the .cc file?

Or replace with '= default' if the clang plugin complains.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:50: 
Unnecessary newline.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:77:
RenderDelayBufferImpl(size_t num_bands);
explicit

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:129: bool
RenderDelayBufferImpl::Insert(std::vector<std::vector<float>>* block) {
This can take 'block' by const ref.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.h:37: // returns true
if the insert is successful.
I think the comment should be updated. Also, since Insert is implemented by
ApiCallJitterBuffer, 'block' could be taken as constant ref.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:100:
msg.input_channel(i).size());
Here we read from msg.input_channel(i) index i for i = 0 .. num_channels().

With the DCHECK above commented out now, how do we know it's safe? Should it
perhaps change to DCHECK_LE instead?

Also, why has this changed?

[peah-webrtc, 2017-04-04 13:57:12]

Thanks for the comments!
I have uploaded a new patch with the related changes.

PTAL

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/block_processor.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:138: void
BlockProcessorImpl::BufferRender(std::vector<std::vector<float>>* block) {
On 2017/04/04 12:18:50, aleloi wrote:
> I think 'block' can be changed to const ref if it is changed in
> RenderDelayBuffer::Insert().

Done.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/block_processor.cc:153: }
On 2017/04/04 12:18:50, aleloi wrote:
> There is a similar guard in ProcessCapture. Does this mean that either an
> initial capture or a render block will always be lost?

Initial capture values will not be lost. What it means is, however, that the
render signal should not start being buffered until both a capture and render
call has been received.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h:24:
std::array<float, kDownsampledRenderBufferSize> buffer;
On 2017/04/03 16:09:07, ivoc wrote:
> I think it should work to add " = {}" to initialize this array with zeros,
> instead of in the constructor. You could possibly remove the explicit
> constructor/destructor and the .cc file?

Great! Thanks! The initialization works but clang complains about missing
constructors, and inlined constructors. I therefore keep those in the .cc file.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/downsampled_render_buffer.h:24:
std::array<float, kDownsampledRenderBufferSize> buffer;
On 2017/04/04 12:18:50, aleloi wrote:
> On 2017/04/03 16:09:07, ivoc wrote:
> > I think it should work to add " = {}" to initialize this array with zeros,
> > instead of in the constructor. You could possibly remove the explicit
> > constructor/destructor and the .cc file?
> 
> Or replace with '= default' if the clang plugin complains.

Acknowledged.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:50: 
On 2017/04/04 12:18:50, aleloi wrote:
> Unnecessary newline.

Done.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:77:
RenderDelayBufferImpl(size_t num_bands);
On 2017/04/04 12:18:50, aleloi wrote:
> explicit

Done.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:129: bool
RenderDelayBufferImpl::Insert(std::vector<std::vector<float>>* block) {
On 2017/04/04 12:18:50, aleloi wrote:
> This can take 'block' by const ref.

Done.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.h:37: // returns true
if the insert is successful.
On 2017/04/04 12:18:50, aleloi wrote:
> I think the comment should be updated. Also, since Insert is implemented by
> ApiCallJitterBuffer, 'block' could be taken as constant ref.

Done.

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right):

https://codereview.webrtc.org/2784023002/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:100:
msg.input_channel(i).size());
On 2017/04/04 12:18:50, aleloi wrote:
> Here we read from msg.input_channel(i) index i for i = 0 .. num_channels().
> 
> With the DCHECK above commented out now, how do we know it's safe? Should it
> perhaps change to DCHECK_LE instead?
> 
> Also, why has this changed?

That was done by mistake (while analyzing a 4 channel recording. 

Reverted that change.

Done.

[aleloi, 2017-04-05 11:46:47]

Can't find anything to complain about either. LGTM!

[peah-webrtc, 2017-04-05 12:41:52]

Thanks for the reviews! I added one more patch some minor changes that were not
caught during Release builds (caused by that change from a pointer to a
const-ref for the block).

I will land this CL now.

[peah-webrtc, 2017-04-05 12:42:01]

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

[peah-webrtc, 2017-04-05 12:42:13]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-05 12:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784023002/#ps120001 (title:
"Rebase and fixed small issues found during debug build")

[commit-bot: I haz the power, 2017-04-05 12:42:24]

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

[commit-bot: I haz the power, 2017-04-05 12:46:32]

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

[commit-bot: I haz the power, 2017-04-05 12:46:34]

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

[peah-webrtc, 2017-04-05 13:23:27]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-05 13:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784023002/#ps140001 (title:
"Added workaround for gcc-bug")

[commit-bot: I haz the power, 2017-04-05 13:23:38]

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

[commit-bot: I haz the power, 2017-04-05 13:42:55]

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

[commit-bot: I haz the power, 2017-04-05 13:42:57]

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

[peah-webrtc, 2017-04-05 17:53:31]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-05 17:53:31]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784023002/#ps160001 (title:
"Disabled DEATH tests that caused issues due to bug on bots and added
initialization of unititialize…")

[commit-bot: I haz the power, 2017-04-05 17:53:42]

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

[commit-bot: I haz the power, 2017-04-05 18:36:30]

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

[commit-bot: I haz the power, 2017-04-05 18:36:31]

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

[peah-webrtc, 2017-04-05 20:56:32]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-05 20:56:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2784023002/#ps180001 (title:
"Disabled one more DEATH test that caused issues due to bug on bots")

[commit-bot: I haz the power, 2017-04-05 20:56:43]

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

[commit-bot: I haz the power, 2017-04-05 21:18:08]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491425792663390,
"parent_rev": "4aceaf268e24c015bbb515ef84a03760699a5df8", "commit_rev":
"cf02cf13a70eb5c8b8be9254a200c5e48ccfe8d8"}

[commit-bot: I haz the power, 2017-04-05 21:18:12]

Description was changed from

==========
Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much 
of the rest of the AEC3 files. Most of these are, however, caused by 
a change of class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

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

to

==========
Major AEC3 render pipeline changes

This CL adds major render pipeline changes to the AEC3 code. The reason
for these are that
1) It allows the echo removal unit to receive information about the content 
in bands beyond band 0, thereby allowing removal of high-frequency
echoes
2) It allows more controlled handling of the render buffers, allowing proper
buffer behaviour during capture glitches and clock-drift.

Unfortunately, the render pipeline caused a lot of related changes in much 
of the rest of the AEC3 files. Most of these are, however, caused by 
a change of class name.

Another unfortunate effect of this CL, is that a number of unittest cease to
compile. I chose to temporarily solve that by removing them from the 
build using #if/#endif. The reason for that is that those will anyway again
need to be changed in the next review, and doing like this avoids them
having to be reviewed twice.

BUG=webrtc:6018

Review-Url: https://codereview.webrtc.org/2784023002
Cr-Commit-Position: refs/heads/master@{#17547}
Committed:
https://chromium.googlesource.com/external/webrtc/+/cf02cf13a70eb5c8b8be9254a...
==========

[commit-bot: I haz the power, 2017-04-05 21:18:14]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/cf02cf13a70eb5c8b8be9254a...