Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 11 months ago by peah-webrtc Modified:
3 years, 10 months ago Reviewers:
aleloi, hlundin-webrtc CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding second layer of the echo canceller 3 functionality.
This CL adds code to the BlockProcessor, which basically constitutes
the second layer in echo canceller 3. The CL includes two incomplete
classes (EchoRemover and EchoPathDelayEstimator) which will be completed
in upcoming CLs. Because of this, some of the unittests are disabled
until those are added.
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2611223003
Cr-Commit-Position: refs/heads/master@{#16319}
Committed: https://chromium.googlesource.com/external/webrtc/+/69221db53413bdca8cb1bacff11b3651f283dd95
Patch Set 1 #
Total comments: 180
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 46
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 6
Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Rebase #Patch Set 6 : Corrected usage of begin and end for array-view which caused compile error on android #Patch Set 7 : Fixed incorrect method call #Patch Set 8 : Rebase #Patch Set 9 : Disabled DEATH tests that were causing memory leakage reports on test bots #Patch Set 10 : Disabled DEATH tests that were causing memory leakage reports on test bots #
Created: 3 years, 11 months ago
Dependent Patchsets: Messages
Total messages: 39 (20 generated)
Patchset #1 (id:1) has been deleted
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org
Hi, Here is the next CL for the echo canceller 3 functionality. Please take a look!
Description was changed from ========== Adding second layer of the echo canceller 3 functionality. This CL adds code to the BlockProcessor, which basically constitutes the second layer in echo canceller 3. The CL includes two incomplete classes (EchoRemover and EchoPathDelayEstimator) which will be completed in upcoming CLs. Because of this, some of the unittests are disabled until those are added. BUG=webrtc:6018 ========== to ========== Adding second layer of the echo canceller 3 functionality. This CL adds code to the BlockProcessor, which basically constitutes the second layer in echo canceller 3. The CL includes two incomplete classes (EchoRemover and EchoPathDelayEstimator) which will be completed in upcoming CLs. Because of this, some of the unittests are disabled until those are added. BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: - ivoc@webrtc.org
Very good CL! I have a number of mostly minor comments. But in addition to those, I'd like you look through the code with three questions in your mind: 1. Does this code only work for a few selected sample rates? That is, does it have to DCHECK that the sample rate is 8000, 16000, 32000, or 48000. The APM in general, and some components in particular only supports these rates, but a lot of the code in this CL seem generic enough to remove those sanity checks. 2. In the unit tests, can the test fail for only a subset of the sample rates? That is, are there any code paths in the code under test that are sample rate dependent? If not, I would urge you to run the test for only one sample rate. (Maybe even for a non-standard rate if the code under test can handle it.) As it is now, you are increasing the total test time by a factor 4, and I'm not sure it is worth it. 3. What year is it?! :) https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:12: #include <memory> memory already included in .h file. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:62: new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))), You only need one ctor (the one with 4 parameters), and have the Create functions create the dependencies when needed. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:114: const std::vector<std::vector<float>>& render_block = auto https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:121: LOG(LS_INFO) << "AEC3 empty render buffer"; Is this an error? When does it happen? Should we (D)CHECK? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:130: bool overrun = !delay_controller_->AnalyzeRender((*block)[0]); I think this code would be easier to read if you stored the results in two distinct variables: const bool overrun1 = !delay_controller_->AnalyzeRender((*block)[0]); const bool overrun2 = !render_buffer_->Insert(block); if (overrun1 || overrun2) { ... (Better names are encouraged...) https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:133: LOG(LS_INFO) << "AEC3 Buffer overrrun"; buffer https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:134: return true; I guess "true" means "overrun happened". But we usually have "true" mean "success". Would it be ok to change to that? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:28: // Only used for testing purposes End with . https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:46: // Buffers a block of render data supplied by a FrameBlocker object. What does it return? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:107: for (auto rate : {8000, 16000, 32000, 48000}) { Are any of the tested code paths dependent on the sample rate? If not, I would argue that you can settle for testing one. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:120: .Times(1) Use .WillOnce(Return(x)) instead of .Times(1).WillRepeatedly(Return(x)). https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:123: .Times(1) WillOnce https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:149: for (auto rate : {8000, 16000, 32000, 48000}) { Same here; would testing one rate suffice? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:168: EXPECT_CALL(*render_delay_buffer_mock, MaxDelay()).Times(0); You don't need this when you have a StrictMock. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:169: EXPECT_CALL(*render_delay_buffer_mock, MaxApiJitter()).Times(0); You don't need this when you have a StrictMock. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:18: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" Forward-declare ApmDataDumper instead. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:71: std::vector<float> render(std::vector<float>(kBlockSize - 1, 0.f)); Copy-paste error; modifying the render block size in this test too. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:79: for (auto rate : {16000, 32000, 48000}) { No need to do this for more than one sample rate. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_variability.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/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/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_variability.cc:15: EchoPathVariability::EchoPathVariability(bool gain_change, bool delay_change) Will this struct be expanded later? Otherwise, can you get these definitions inlined into the h file? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_variability.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_variability.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.h:18: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" Forward-declare. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:155: for (auto rate : {16000, 32000, 48000}) { Do you have to test all rates? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:174: for (auto rate : {16000, 32000, 48000}) { Do you have to test all rates? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_echo_remover.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_echo_remover.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:32: virtual const std::vector<std::vector<float>>& GetNext() { return block_; } I think the preferred pattern is to have the mock of the real method delegate to a fake. See https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#.... For a tangible example in WebRTC, see webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc, line 36. In your code, I would suggest the following. Mock GetNext as any other method: MOCK_METHOD0(GetNext, const std::vector<std::vector<float>>&()); In the body of the mock's ctor, add: ON_CALL(*this, GetNext()) .WillByDefault(Invoke(this, &MockRenderDelayBuffer::FakeGetNext)); Finally, define a new method, private to the mock: const std::vector<std::vector<float>>& FakeGetNext() { return block_; } This way, you can still set expectations on GetNext being called (or not), but it will by default return the fake block. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_controller.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:43: std::vector<std::vector<std::vector<float>>> buffer_; <inception meme> A vector of vectors of vectors... </inception meme> :) https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:44: int last_insert_index_ = 0; Shouldn't this be size_t? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:78: RTC_DCHECK(IsBlockAvailable()); I gather it is the caller's responsibility to make sure that there is a block available before calling GetNext. Better document that in the h file. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:79: const int extract_index_ = size_t? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:81: buffer_.size(); DCHECK that extract_index ends up in the valid range. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:88: RTC_DCHECK_GE(MaxDelay(), delay); Also the caller's responsibility. Document in h file. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:73: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE. If this is not true, the GetNext method will DCHECK and kill the test binary. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:95: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:124: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:153: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:169: // Verifies that no blocks are lost when the buffer is overflowed. The block that caused the overflow is still lost, right? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:182: NumBandsForRate(rate), std::vector<float>(kBlockSize, 31)); Why 31? Magic number... https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:188: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:212: // Verifies that the check for available block works.. Double .. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:215: RenderDelayBuffer::Create(20, 1, kMaxApiCallJitter)); kNumBands = 1 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:218: 1, std::vector<float>(kBlockSize, 1.f)); kNumBands https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:220: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:225: // Verifies that the maximum delay is computed correctly . https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:27: explicit RenderBuffer(size_t size); I think you should move all the ctor and method definitions up into the class declaration. Avoids scrolling up and down in the file. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:69: const int echo_path_delay_blocks = echo_path_delay_samples / kBlockSize; Is this division supposed to be exact, or is truncation intended? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:106: return false; Eh, this must be a mistake. The return type is size_t. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:150: if (size_ == buffer_.size() - 1) { Why not err on the side of caution, and compare size_ >= buffer_.size() - 1? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; Select sounds a bit like the caller can select a delay through this call. Maybe change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:37: // expressed in samples. When does it return empty? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:36: std::ostringstream ss; I think you should be able to cascade these. std::ostringstream ss(ProduceDebugText(sample_rate_hz)); ss << ", Delay: " << delay; return ss.str(); https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:45: std::ostringstream ss; std::ostringstream ss(ProduceDebugText(sample_rate_hz, delay)); ss << ", Max Api call jitter: " << max_jitter; return ss.str(); https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:151: EXPECT_TRUE(headroom_samples); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:204: EXPECT_TRUE(headroom_samples); ASSERT_TRUE https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:67: void DumpRaw(const char* name, int v_length, const double* v) { size_t v_length https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:102: FILE* file = GetRawFile(name); You could just call DumpRaw(name, static_cast<int16_t>(v)); https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:158: void DumpRaw(const char* name, size_t v) { Hmmm. This starts to look a bit like a candidate for templetization... https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. http://whatyearisit.info/ https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:17: #include "webrtc/base/constructormagic.h" Order of includes. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:27: explicit DelayBuffer(size_t delay) : buffer_(delay, 0.f) {} You don't know that the vector will be of float type, so 0.f is probably not correct. I think you can omit the value and get zero-initialization anyway. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:31: void Delay(rtc::ArrayView<const T> x, rtc::ArrayView<T> x_delayed) { This is quite a big chunk of inlined code. Move to .cc file? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:46: int next_insert_index_ = 0; size_t https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:30: const size_t kBlockSize = 50; constexpr https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:44: std::vector<int> v(1000, 0.f); Not 0.f.
Thanks :-) And thank you for a really great review! -Regarding 1) (the restriction of the sample rates): It may be possible to generalize the code in some places to support other rates than the ones selected but I doubt it is worth it. Therefore I'd rather lock down everything to be restricted to that. Most of the code does actually not care about the sample rate but rather about the number of bands. The difference is the distinguishing between 8 and 16 kHz that both have only one band but which instead have different frame lengths for that one band. Furthermore, 8 and 16 kHz needs different downsampling and band-limiting (will come in the upcoming CLs. Apart from that the code does not really care about the sampling rate but rather on the fixed blocksize. That blocksize does, however, implicitly place restrictions on the possible sample rates. Since some of the code is dependent on the sample rate, and almost all of the code is dependent on the number of bands, and the number of bands can be computed based on the sample rate, I've chosen only to propagate the sample rate to the components of the echo canceller. In some places that is used to choose downsampling rates and filter coefficients but in most places it is stored to be able to verify (via DCHECKS) that the method parameters have the expected number of bands. I think it will become more clear in upcoming CLs that the sample rate is only passed where it is needed either to 1) Verify/choose the number of bands, 2) choose filter coefficients or 3) choose decimation factors. My proposal is to keep it as it is and , if needed, revise it when more code is needed. -Regarding 2) (tests of all sample rates): I definitely agree, and I removed some of this when this was superclear that it is not needed. For the more complicated tests, such as those for the Mocks in the BlockProcessor, I'd rather keep it as it is. Currently it is fine, but imposing the assumption that this is sample rate independent may cause problems for future changes to BlockProcessor. That test is quite cheap as it is anyway it is, in my mind, not something that should be streamlined to improve test performance. Please let me know what you think on that. -Regarding 3) (what year it is): That depends, as time is relative as well as subjective, and as WebRTC gain in usage the correct year to put in the code may be questioned by an increasing amount of users of the code. For instance, there are likely future worm-hole travelers to Alpha Centauri that would claim that the stated date in echo_canceller_test_tools.h would be the most appropriate. I do, however, agree with the reviewer that until we get feedback from those it may be more appropriate to choose a year that can be accepted by a majority of our users, which at the moment happen to be in fairly close proximity to our planet. We can always revise this in future (or past) updates.
https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:12: #include <memory> On 2017/01/18 13:08:48, hlundin-webrtc wrote: > memory already included in .h file. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:62: new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))), On 2017/01/18 13:08:47, hlundin-webrtc wrote: > You only need one ctor (the one with 4 parameters), and have the Create > functions create the dependencies when needed. Good point! I needed to do some other changes as well as it is in that construction no longer possible nicely pass around the pointer to the ApmDataDumper. I therefore let the other objects create their own ApmDataDumper objects. PTAL Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:114: const std::vector<std::vector<float>>& render_block = On 2017/01/18 13:08:47, hlundin-webrtc wrote: > auto Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:121: LOG(LS_INFO) << "AEC3 empty render buffer"; On 2017/01/18 13:08:48, hlundin-webrtc wrote: > Is this an error? When does it happen? Should we (D)CHECK? I'm not sure how we should handle this. It is basically related to clock-drift in that it happens when there are more samples being rendered than there is being captured. As the clock-drift handling is not yet ready, I'd like to keep it as it is. I neither want to DCHECK as it is is a valid condition for clockdrift. WDYT, can we keep it as it is and change this when we land the code for handling clock-drift? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:130: bool overrun = !delay_controller_->AnalyzeRender((*block)[0]); On 2017/01/18 13:08:47, hlundin-webrtc wrote: > I think this code would be easier to read if you stored the results in two > distinct variables: > > const bool overrun1 = !delay_controller_->AnalyzeRender((*block)[0]); > const bool overrun2 = !render_buffer_->Insert(block); > if (overrun1 || overrun2) { > ... > > (Better names are encouraged...) Good point! Agree I do. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:133: LOG(LS_INFO) << "AEC3 Buffer overrrun"; On 2017/01/18 13:08:47, hlundin-webrtc wrote: > buffer Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:134: return true; On 2017/01/18 13:08:48, hlundin-webrtc wrote: > I guess "true" means "overrun happened". But we usually have "true" mean > "success". Would it be ok to change to that? Makes sense. I did that change in some other places as well. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:28: // Only used for testing purposes On 2017/01/18 13:08:48, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:46: // Buffers a block of render data supplied by a FrameBlocker object. On 2017/01/18 13:08:48, hlundin-webrtc wrote: > What does it return? Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:107: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/18 13:08:48, hlundin-webrtc wrote: > Are any of the tested code paths dependent on the sample rate? If not, I would > argue that you can settle for testing one. I agree that it is probably sufficient to only test one rate. I would, however, prefer to keep it as it is as the test is very cheap, and that as doing that it does not include any assumption on the code being tested. Such an assumption could easily be broken during future code development and would very likely be missed as the test will (may) not brake for that code change. WDYT, can we keep it as it is? The test is cheaper, and the test will become weaker if we only test one rate. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:120: .Times(1) On 2017/01/18 13:08:48, hlundin-webrtc wrote: > Use .WillOnce(Return(x)) instead of .Times(1).WillRepeatedly(Return(x)). Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:123: .Times(1) On 2017/01/18 13:08:48, hlundin-webrtc wrote: > WillOnce Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:149: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/18 13:08:48, hlundin-webrtc wrote: > Same here; would testing one rate suffice? Probably yes, but for the same arguments as above, I'd like to keep it. WDYT? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:168: EXPECT_CALL(*render_delay_buffer_mock, MaxDelay()).Times(0); On 2017/01/18 13:08:48, hlundin-webrtc wrote: > You don't need this when you have a StrictMock. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:169: EXPECT_CALL(*render_delay_buffer_mock, MaxApiJitter()).Times(0); On 2017/01/18 13:08:48, hlundin-webrtc wrote: > You don't need this when you have a StrictMock. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:48, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:18: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" On 2017/01/18 13:08:48, hlundin-webrtc wrote: > Forward-declare ApmDataDumper instead. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:71: std::vector<float> render(std::vector<float>(kBlockSize - 1, 0.f)); On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Copy-paste error; modifying the render block size in this test too. Great find! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:79: for (auto rate : {16000, 32000, 48000}) { On 2017/01/18 13:08:49, hlundin-webrtc wrote: > No need to do this for more than one sample rate. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_variability.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/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/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_variability.cc:15: EchoPathVariability::EchoPathVariability(bool gain_change, bool delay_change) On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Will this struct be expanded later? Otherwise, can you get these definitions > inlined into the h file? Good point. The struct is as it is intended and it worked. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_variability.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_variability.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover.h:18: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Forward-declare. This is now solved in another way. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:155: for (auto rate : {16000, 32000, 48000}) { On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Do you have to test all rates? Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc:174: for (auto rate : {16000, 32000, 48000}) { On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Do you have to test all rates? This test is now removed. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_echo_remover.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_echo_remover.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:32: virtual const std::vector<std::vector<float>>& GetNext() { return block_; } On 2017/01/18 13:08:49, hlundin-webrtc wrote: > I think the preferred pattern is to have the mock of the real method delegate to > a fake. See > https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#.... > > For a tangible example in WebRTC, see > webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc, > line 36. > > In your code, I would suggest the following. Mock GetNext as any other method: > MOCK_METHOD0(GetNext, const std::vector<std::vector<float>>&()); > > In the body of the mock's ctor, add: > ON_CALL(*this, GetNext()) > .WillByDefault(Invoke(this, &MockRenderDelayBuffer::FakeGetNext)); > > Finally, define a new method, private to the mock: > const std::vector<std::vector<float>>& FakeGetNext() { return block_; } > > This way, you can still set expectations on GetNext being called (or not), but > it will by default return the fake block. Great suggestion!!! Thanks! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_controller.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:49, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:43: std::vector<std::vector<std::vector<float>>> buffer_; On 2017/01/18 13:08:50, hlundin-webrtc wrote: > <inception meme> A vector of vectors of vectors... </inception meme> > :) :-D Yes, it is almost like a 5D world. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:44: int last_insert_index_ = 0; On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Shouldn't this be size_t? Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:78: RTC_DCHECK(IsBlockAvailable()); On 2017/01/18 13:08:49, hlundin-webrtc wrote: > I gather it is the caller's responsibility to make sure that there is a block > available before calling GetNext. Better document that in the h file. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:79: const int extract_index_ = On 2017/01/18 13:08:49, hlundin-webrtc wrote: > size_t? Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:81: buffer_.size(); On 2017/01/18 13:08:49, hlundin-webrtc wrote: > DCHECK that extract_index ends up in the valid range. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:88: RTC_DCHECK_GE(MaxDelay(), delay); On 2017/01/18 13:08:49, hlundin-webrtc wrote: > Also the caller's responsibility. Document in h file. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:73: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE. If this is not true, the GetNext method will DCHECK and kill the > test binary. Good point! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:95: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:124: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:153: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:169: // Verifies that no blocks are lost when the buffer is overflowed. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > The block that caused the overflow is still lost, right? True, but that depends on how the this is handled by the caller. The block is never inserted so it is actually not lost. I changed the comment to be more clear on this though. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:182: NumBandsForRate(rate), std::vector<float>(kBlockSize, 31)); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Why 31? Magic number... Thanks! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:188: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:212: // Verifies that the check for available block works.. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Double .. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:215: RenderDelayBuffer::Create(20, 1, kMaxApiCallJitter)); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > kNumBands = 1 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:218: 1, std::vector<float>(kBlockSize, 1.f)); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > kNumBands Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:220: EXPECT_TRUE(delay_buffer->IsBlockAvailable()); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:225: // Verifies that the maximum delay is computed correctly On 2017/01/18 13:08:50, hlundin-webrtc wrote: > . Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:27: explicit RenderBuffer(size_t size); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > I think you should move all the ctor and method definitions up into the class > declaration. Avoids scrolling up and down in the file. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:69: const int echo_path_delay_blocks = echo_path_delay_samples / kBlockSize; On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Is this division supposed to be exact, or is truncation intended? It is not supposed to be exact, and truncation is intended. I added a comment. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:106: return false; On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Eh, this must be a mistake. The return type is size_t. Good find! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:150: if (size_ == buffer_.size() - 1) { On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Why not err on the side of caution, and compare size_ >= buffer_.size() - 1? Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; On 2017/01/18 13:08:50, hlundin-webrtc wrote: > Select sounds a bit like the caller can select a delay through this call. Maybe > change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? I agree. I changed it to GetDelay. I like that better than CalculateDelay and EstimateDelay as the class is not called something with estimator. Furthermore, I prefer that to GetAlignment as the class does not perform the alignment but rather controls the delay in the render delay buffer. WDYT? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:37: // expressed in samples. On 2017/01/18 13:08:50, hlundin-webrtc wrote: > When does it return empty? What it does is to return the alignment headroom that would be achieved if the delay provided by the controller is applied to the render delay buffer. That it only does, however, if the controller has confidence in the delay that is reported, hence the possible empty return. For instance, consider the case where the delay controller at first finds good correlation between render and capture and from that chooses a render buffer delay, but then for a long while does not find any correlation. In that case, the chosen render buffer delay should remain the same, but it cannot promise any reliable alignment headroom. Maybe we should change the name for the AlignmentHeadroom method as that is misleading as the controller does not align the signals. This rather the headroom that is achieved if the provided delay is applied to the render buffer. WDYT? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:51, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:36: std::ostringstream ss; On 2017/01/18 13:08:51, hlundin-webrtc wrote: > I think you should be able to cascade these. > > std::ostringstream ss(ProduceDebugText(sample_rate_hz)); > ss << ", Delay: " << delay; > return ss.str(); Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:45: std::ostringstream ss; On 2017/01/18 13:08:51, hlundin-webrtc wrote: > std::ostringstream ss(ProduceDebugText(sample_rate_hz, delay)); > ss << ", Max Api call jitter: " << max_jitter; > return ss.str(); Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:151: EXPECT_TRUE(headroom_samples); On 2017/01/18 13:08:50, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller_unittest.cc:204: EXPECT_TRUE(headroom_samples); On 2017/01/18 13:08:51, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:67: void DumpRaw(const char* name, int v_length, const double* v) { On 2017/01/18 13:08:51, hlundin-webrtc wrote: > size_t v_length Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:102: FILE* file = GetRawFile(name); On 2017/01/18 13:08:51, hlundin-webrtc wrote: > You could just call > DumpRaw(name, static_cast<int16_t>(v)); Good find! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:158: void DumpRaw(const char* name, size_t v) { On 2017/01/18 13:08:51, hlundin-webrtc wrote: > Hmmm. This starts to look a bit like a candidate for templetization... True. Let's do that. But I'd rather keep that change separate from this CL. WDYT? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:51, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:51, hlundin-webrtc wrote: > http://whatyearisit.info/ :-D https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:17: #include "webrtc/base/constructormagic.h" On 2017/01/18 13:08:51, hlundin-webrtc wrote: > Order of includes. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:27: explicit DelayBuffer(size_t delay) : buffer_(delay, 0.f) {} On 2017/01/18 13:08:51, hlundin-webrtc wrote: > You don't know that the vector will be of float type, so 0.f is probably not > correct. I think you can omit the value and get zero-initialization anyway. Good find! Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:31: void Delay(rtc::ArrayView<const T> x, rtc::ArrayView<T> x_delayed) { On 2017/01/18 13:08:51, hlundin-webrtc wrote: > This is quite a big chunk of inlined code. Move to .cc file? I don't think we can do that. This is a template and afaics we cannot move that into the .cc file. Or have I gotten that wrong? https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:46: int next_insert_index_ = 0; On 2017/01/18 13:08:51, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2017/01/18 13:08:51, hlundin-webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:30: const size_t kBlockSize = 50; On 2017/01/18 13:08:51, hlundin-webrtc wrote: > constexpr Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools_unittest.cc:44: std::vector<int> v(1000, 0.f); On 2017/01/18 13:08:51, hlundin-webrtc wrote: > Not 0.f. Done.
Nice. Not much more to do now... https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:12: #include <memory> On 2017/01/19 15:33:04, peah-webrtc wrote: > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > memory already included in .h file. > > Done. No, not done. I meant that you should remove it from this file. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:121: LOG(LS_INFO) << "AEC3 empty render buffer"; On 2017/01/19 15:33:04, peah-webrtc wrote: > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > Is this an error? When does it happen? Should we (D)CHECK? > > I'm not sure how we should handle this. It is basically related to clock-drift > in that it happens when there are more samples being rendered than there is > being captured. As the clock-drift handling is not yet ready, I'd like to keep > it as it is. I neither want to DCHECK as it is is a valid condition for > clockdrift. > > WDYT, can we keep it as it is and change this when we land the code for handling > clock-drift? Sure. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:107: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/19 15:33:04, peah-webrtc wrote: > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > Are any of the tested code paths dependent on the sample rate? If not, I would > > argue that you can settle for testing one. > > I agree that it is probably sufficient to only test one rate. I would, however, > prefer to keep it as it is as the test is very cheap, and that as doing that it > does not include any assumption on the code being tested. Such an assumption > could easily be broken during future code development and would very likely be > missed as the test will (may) not brake for that code change. > > WDYT, can we keep it as it is? The test is cheaper, and the test will become > weaker if we only test one rate. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:149: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/19 15:33:04, peah-webrtc wrote: > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > Same here; would testing one rate suffice? > > Probably yes, but for the same arguments as above, I'd like to keep it. > > WDYT? Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:169: // Verifies that no blocks are lost when the buffer is overflowed. On 2017/01/19 15:33:06, peah-webrtc wrote: > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > The block that caused the overflow is still lost, right? > > True, but that depends on how the this is handled by the caller. The block is > never inserted so it is actually not lost. I changed the comment to be more > clear on this though. > > Done. Got it. But then you should probably also check that the block you tried to insert is not swapped out. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; On 2017/01/19 15:33:07, peah-webrtc wrote: > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > Select sounds a bit like the caller can select a delay through this call. > Maybe > > change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? > > I agree. I changed it to GetDelay. I like that better than CalculateDelay and > EstimateDelay as the class is not called something with estimator. Furthermore, > I prefer that to GetAlignment as the class does not perform the alignment but > rather controls the delay in the render delay buffer. > > WDYT? > Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:37: // expressed in samples. On 2017/01/19 15:33:07, peah-webrtc wrote: > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > When does it return empty? > > What it does is to return the alignment headroom that would be achieved if the > delay provided by the controller is applied to the render delay buffer. That it > only does, however, if the controller has confidence in the delay that is > reported, hence the possible empty return. > > For instance, consider the case where the delay controller at first finds good > correlation between render and capture and from that chooses a render buffer > delay, but then for a long while does not find any correlation. In that case, > the chosen render buffer delay should remain the same, but it cannot promise any > reliable alignment headroom. > > Maybe we should change the name for the AlignmentHeadroom method as that is > misleading as the controller does not align the signals. This rather the > headroom that is achieved if the provided delay is applied to the render buffer. > > WDYT? I think some of this explanation should go into the code. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:158: void DumpRaw(const char* name, size_t v) { On 2017/01/19 15:33:07, peah-webrtc wrote: > On 2017/01/18 13:08:51, hlundin-webrtc wrote: > > Hmmm. This starts to look a bit like a candidate for templetization... > > True. Let's do that. But I'd rather keep that change separate from this CL. > > WDYT? Good. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:31: void Delay(rtc::ArrayView<const T> x, rtc::ArrayView<T> x_delayed) { On 2017/01/19 15:33:07, peah-webrtc wrote: > On 2017/01/18 13:08:51, hlundin-webrtc wrote: > > This is quite a big chunk of inlined code. Move to .cc file? > > I don't think we can do that. This is a template and afaics we cannot move that > into the .cc file. Or have I gotten that wrong? You can move it to the .cc file. But you have to do some special tricks: The method definition must look like template <typename T> void DelayBuffer<T>::Delay(...) { ... } And, you must instantiate the template for the types you will be using. At the end of the .cc file, add template class DelayBuffer<float>; template class DelayBuffer<int>; } // namespace webrtc https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:47: // bool indicating whether there was a buffer overrun. Well, it now returns false when overrun did happen. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:292: RTC_DCHECK(!successful_buffering); I think you better invert the DCHECK. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:99: bool EmptyRenderQueue(); Document the meaning of the return value. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:45: const std::vector<std::vector<float>>& FakeGetNext() const { return block_; } Methods before member variables. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:34: // avalailablem which means that that must be checked prior to the call using avalailablem? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:43: RenderBuffer::RenderBuffer(size_t size) Better, but you could actually have them _in_ the class declaration. But this is good enough for me.
No questions about WebRTC running at relativistic speeds. Lgtm except for a few small comments. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; On 2017/01/19 15:33:07, peah-webrtc wrote: > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > Select sounds a bit like the caller can select a delay through this call. > Maybe > > change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? > > I agree. I changed it to GetDelay. I like that better than CalculateDelay and > EstimateDelay as the class is not called something with estimator. Furthermore, > I prefer that to GetAlignment as the class does not perform the alignment but > rather controls the delay in the render delay buffer. > > WDYT? > GetDelay sounds fine. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:19: #include "webrtc/modules/audio_processing/aec3/echo_remover.h" echo_remover.h is also included in parent header. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:55: constexpr size_t kMaxApiJitter = 30; I think kRenderBufferSize and kMaxApiJitter should either both be static const or both constexpr. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:119: BlockProcessor* BlockProcessor::Create(int sample_rate_hz) { I think keeping the number of 'new' low is a good guideline. Therefore this Create should be implemented in terms of the last factory method. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:131: BlockProcessor* BlockProcessor::Create( Same here. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:17: #include "webrtc/base/constructormagic.h" Not used in .h file and already included in .cc file. Remove? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:118: EXPECT_CALL(*render_delay_buffer_mock, SetDelay(9)).Times(AtLeast(1)); Is it obvious that the correct delay is 9 blocks? If not, there is a dependency between this and the implementation of DelayController/DelayEstimator and the extra headroom. (Which I don't fully understand, so I can be wrong about this). How about checking for SetDelay(AllOf(Ge(8),Le(10))) or whatever is correct instead? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:131: DelayBuffer<float> signal_delay_buffer(640); Related comment about correct delay above. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:333: bool successful_buffering = false; successful_buffering is initialized with false, and is updated with &&. I think the starting value should be 'true'. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:38: std::vector<float> capture(std::vector<float>(kBlockSize, 0.f)); I think there is an extra copy constructor call here. Would std::vector<float> render(kBlockSize, 0.f); work? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:59: std::vector<float> render(std::vector<float>(kBlockSize - 1, 0.f)); Same here. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:71: std::vector<float> render(std::vector<float>(kBlockSize, 0.f)); And here. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:143: delay_estimator_.EstimateDelay(render, capture); More questions (not review comments): will the delay estimator also have a buffer? How can it estimate delay in samples (which can be several blocks) with only one render and one capture block? Maybe it will get more clear with the next CL. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:150: if (new_delay != delay_ && align_call_counter_ > 250) { Is align_call_counter_ only for not setting delay during the first second? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:21: // Class for aligning the render and capture signal using an RenderDelayBuffer. an -> a https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:32: // Analyses the render signal and returns false if there is a buffer overrun. Nit: analyse/analyze inconsistent between method and comment. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:13: #include "webrtc/base/checks.h" Not used.
Thanks again for the great comments! I have adressed them with another patch. PTAL https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:12: #include <memory> On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:04, peah-webrtc wrote: > > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > > memory already included in .h file. > > > > Done. > > No, not done. I meant that you should remove it from this file. Sorry. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:121: LOG(LS_INFO) << "AEC3 empty render buffer"; On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:04, peah-webrtc wrote: > > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > > Is this an error? When does it happen? Should we (D)CHECK? > > > > I'm not sure how we should handle this. It is basically related to clock-drift > > in that it happens when there are more samples being rendered than there is > > being captured. As the clock-drift handling is not yet ready, I'd like to keep > > it as it is. I neither want to DCHECK as it is is a valid condition for > > clockdrift. > > > > WDYT, can we keep it as it is and change this when we land the code for > handling > > clock-drift? > > Sure. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:107: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:04, peah-webrtc wrote: > > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > > Are any of the tested code paths dependent on the sample rate? If not, I > would > > > argue that you can settle for testing one. > > > > I agree that it is probably sufficient to only test one rate. I would, > however, > > prefer to keep it as it is as the test is very cheap, and that as doing that > it > > does not include any assumption on the code being tested. Such an assumption > > could easily be broken during future code development and would very likely be > > missed as the test will (may) not brake for that code change. > > > > WDYT, can we keep it as it is? The test is cheaper, and the test will become > > weaker if we only test one rate. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:149: for (auto rate : {8000, 16000, 32000, 48000}) { On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:04, peah-webrtc wrote: > > On 2017/01/18 13:08:48, hlundin-webrtc wrote: > > > Same here; would testing one rate suffice? > > > > Probably yes, but for the same arguments as above, I'd like to keep it. > > > > WDYT? > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer_unittest.cc:169: // Verifies that no blocks are lost when the buffer is overflowed. On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:06, peah-webrtc wrote: > > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > > The block that caused the overflow is still lost, right? > > > > True, but that depends on how the this is handled by the caller. The block is > > never inserted so it is actually not lost. I changed the comment to be more > > clear on this though. > > > > Done. > > Got it. But then you should probably also check that the block you tried to > insert is not swapped out. Done. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:07, peah-webrtc wrote: > > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > > Select sounds a bit like the caller can select a delay through this call. > > Maybe > > > change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? > > > > I agree. I changed it to GetDelay. I like that better than CalculateDelay and > > EstimateDelay as the class is not called something with estimator. > Furthermore, > > I prefer that to GetAlignment as the class does not perform the alignment but > > rather controls the delay in the render delay buffer. > > > > WDYT? > > > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:31: virtual size_t SelectDelay(rtc::ArrayView<const float> capture) = 0; On 2017/01/20 14:45:37, aleloi wrote: > On 2017/01/19 15:33:07, peah-webrtc wrote: > > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > > Select sounds a bit like the caller can select a delay through this call. > > Maybe > > > change it to CalculateDelay or EstimateDelay? Or GetAlignment[Delay]? > > > > I agree. I changed it to GetDelay. I like that better than CalculateDelay and > > EstimateDelay as the class is not called something with estimator. > Furthermore, > > I prefer that to GetAlignment as the class does not perform the alignment but > > rather controls the delay in the render delay buffer. > > > > WDYT? > > > > GetDelay sounds fine. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:37: // expressed in samples. On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:07, peah-webrtc wrote: > > On 2017/01/18 13:08:50, hlundin-webrtc wrote: > > > When does it return empty? > > > > What it does is to return the alignment headroom that would be achieved if the > > delay provided by the controller is applied to the render delay buffer. That > it > > only does, however, if the controller has confidence in the delay that is > > reported, hence the possible empty return. > > > > For instance, consider the case where the delay controller at first finds good > > correlation between render and capture and from that chooses a render buffer > > delay, but then for a long while does not find any correlation. In that case, > > the chosen render buffer delay should remain the same, but it cannot promise > any > > reliable alignment headroom. > > > > Maybe we should change the name for the AlignmentHeadroom method as that is > > misleading as the controller does not align the signals. This rather the > > headroom that is achieved if the provided delay is applied to the render > buffer. > > > > WDYT? > > I think some of this explanation should go into the code. I changed the name of the method to achieve part of comment, and let the return of the optional value replace another part of the comment. PTAL https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:158: void DumpRaw(const char* name, size_t v) { On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:07, peah-webrtc wrote: > > On 2017/01/18 13:08:51, hlundin-webrtc wrote: > > > Hmmm. This starts to look a bit like a candidate for templetization... > > > > True. Let's do that. But I'd rather keep that change separate from this CL. > > > > WDYT? > > Good. Acknowledged. https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.h (right): https://codereview.webrtc.org/2611223003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.h:31: void Delay(rtc::ArrayView<const T> x, rtc::ArrayView<T> x_delayed) { On 2017/01/20 09:31:44, hlundin-webrtc wrote: > On 2017/01/19 15:33:07, peah-webrtc wrote: > > On 2017/01/18 13:08:51, hlundin-webrtc wrote: > > > This is quite a big chunk of inlined code. Move to .cc file? > > > > I don't think we can do that. This is a template and afaics we cannot move > that > > into the .cc file. Or have I gotten that wrong? > > You can move it to the .cc file. But you have to do some special tricks: > > The method definition must look like > template <typename T> > void DelayBuffer<T>::Delay(...) { ... } > > And, you must instantiate the template for the types you will be using. At the > end of the .cc file, add > > template class DelayBuffer<float>; > template class DelayBuffer<int>; > } // namespace webrtc Thanks! Great! Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:19: #include "webrtc/modules/audio_processing/aec3/echo_remover.h" On 2017/01/20 14:45:37, aleloi wrote: > echo_remover.h is also included in parent header. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:55: constexpr size_t kMaxApiJitter = 30; On 2017/01/20 14:45:37, aleloi wrote: > I think kRenderBufferSize and kMaxApiJitter should either both be static const > or both constexpr. I agree. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:119: BlockProcessor* BlockProcessor::Create(int sample_rate_hz) { On 2017/01/20 14:45:37, aleloi wrote: > I think keeping the number of 'new' low is a good guideline. Therefore this > Create should be implemented in terms of the last factory method. Good point! Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:131: BlockProcessor* BlockProcessor::Create( On 2017/01/20 14:45:37, aleloi wrote: > Same here. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:17: #include "webrtc/base/constructormagic.h" On 2017/01/20 14:45:37, aleloi wrote: > Not used in .h file and already included in .cc file. Remove? Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:47: // bool indicating whether there was a buffer overrun. On 2017/01/20 09:31:44, hlundin-webrtc wrote: > Well, it now returns false when overrun did happen. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:118: EXPECT_CALL(*render_delay_buffer_mock, SetDelay(9)).Times(AtLeast(1)); On 2017/01/20 14:45:37, aleloi wrote: > Is it obvious that the correct delay is 9 blocks? If not, there is a dependency > between this and the implementation of DelayController/DelayEstimator and the > extra headroom. (Which I don't fully understand, so I can be wrong about this). > How about checking for SetDelay(AllOf(Ge(8),Le(10))) or whatever is correct > instead? I agree. I tried to make it more clear by using constants. I would, however, want the value 9 to stay as that is the delay that the delay controller is designed to produced. 8 or 10 would not be correct and therefore we should not test against that. One could argue that this test is too much tied to the implementation of the delay controller but so is the rest of the AEC3. For instance, if delay controller would produce 10 instead of 9, the likelihood of the AEC leaking echoes would be very high. Therefore I think it is ok to use a strict value for the output of the delay controller in this case. WDYT? https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:131: DelayBuffer<float> signal_delay_buffer(640); On 2017/01/20 14:45:37, aleloi wrote: > Related comment about correct delay above. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:292: RTC_DCHECK(!successful_buffering); On 2017/01/20 09:31:44, hlundin-webrtc wrote: > I think you better invert the DCHECK. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:333: bool successful_buffering = false; On 2017/01/20 14:45:37, aleloi wrote: > successful_buffering is initialized with false, and is updated with &&. I think > the starting value should be 'true'. Good find! I must have missed running the test after this update. With this, and another related change, thy now pass. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:99: bool EmptyRenderQueue(); On 2017/01/20 09:31:44, hlundin-webrtc wrote: > Document the meaning of the return value. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:38: std::vector<float> capture(std::vector<float>(kBlockSize, 0.f)); On 2017/01/20 14:45:37, aleloi wrote: > I think there is an extra copy constructor call here. Would > > std::vector<float> render(kBlockSize, 0.f); > > work? Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:59: std::vector<float> render(std::vector<float>(kBlockSize - 1, 0.f)); On 2017/01/20 14:45:37, aleloi wrote: > Same here. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:71: std::vector<float> render(std::vector<float>(kBlockSize, 0.f)); On 2017/01/20 14:45:37, aleloi wrote: > And here. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/mock/mock_render_delay_buffer.h:45: const std::vector<std::vector<float>>& FakeGetNext() const { return block_; } On 2017/01/20 09:31:44, hlundin-webrtc wrote: > Methods before member variables. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:34: // avalailablem which means that that must be checked prior to the call using On 2017/01/20 09:31:44, hlundin-webrtc wrote: > avalailablem? :-) Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:43: RenderBuffer::RenderBuffer(size_t size) On 2017/01/20 09:31:44, hlundin-webrtc wrote: > Better, but you could actually have them _in_ the class declaration. But this is > good enough for me. Ah. Sorry. Missed that. I now included them in the class declaration which definitely became better. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:143: delay_estimator_.EstimateDelay(render, capture); On 2017/01/20 14:45:37, aleloi wrote: > More questions (not review comments): will the delay estimator also have a > buffer? How can it estimate delay in samples (which can be several blocks) with > only one render and one capture block? Maybe it will get more clear with the > next CL. Yes, it will, which makes it somehow redundant as we will have several buffers. However, the estimator buffer will be downsampled to 4 kHz to save computations an storage. How this works is clear from the next CL. Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.cc:150: if (new_delay != delay_ && align_call_counter_ > 250) { On 2017/01/20 14:45:37, aleloi wrote: > Is align_call_counter_ only for not setting delay during the first second? Yes, that is correct. As I consider it is impossible for the delay estimator to produce a good estimate until after 1 second, I added that as a safeguard to ensure that the delay is not randomly varied in the initial part of the call. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_controller.h (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:21: // Class for aligning the render and capture signal using an RenderDelayBuffer. On 2017/01/20 14:45:37, aleloi wrote: > an -> a Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_controller.h:32: // Analyses the render signal and returns false if there is a buffer overrun. On 2017/01/20 14:45:37, aleloi wrote: > Nit: analyse/analyze inconsistent between method and comment. Impressive catch! Done. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:13: #include "webrtc/base/checks.h" On 2017/01/20 14:45:37, aleloi wrote: > Not used. Done.
Well done! LGTM with comments. https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:13: #include "webrtc/base/checks.h" On 2017/01/23 14:16:41, peah-webrtc wrote: > On 2017/01/20 14:45:37, aleloi wrote: > > Not used. > > Done. Well, now it is used, so hurry and put it back. :) https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:108: constexpr size_t kDelayInBlocks = Is this division supposed to be exact? If so, you could be fancy and add static_assert((kDelayInBlocks + kDelayHeadroom) * kBlockSize == kDelayInSamples, ""); Totally optional, though. https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:34: // avalailable which means that that must be checked prior to the call using Close, but no cigar. You still need to drop some letters. Two, to be specific. https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:21: // Produces a delayed signal copy of x. Repeated comment. Drop this.
Thanks again for the comments! https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:13: #include "webrtc/base/checks.h" On 2017/01/23 19:48:28, hlundin-webrtc wrote: > On 2017/01/23 14:16:41, peah-webrtc wrote: > > On 2017/01/20 14:45:37, aleloi wrote: > > > Not used. > > > > Done. > > Well, now it is used, so hurry and put it back. :) :-) Done. https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:108: constexpr size_t kDelayInBlocks = On 2017/01/23 19:48:28, hlundin-webrtc wrote: > Is this division supposed to be exact? If so, you could be fancy and add > static_assert((kDelayInBlocks + kDelayHeadroom) * kBlockSize == kDelayInSamples, > ""); > > Totally optional, though. Thanks for the suggestion! The division is not require to be exact though. Not sure why I chose the delay such that it is exact, but it actually works for any delay which means that that assert is misleading. https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_delay_buffer.h (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_delay_buffer.h:34: // avalailable which means that that must be checked prior to the call using On 2017/01/23 19:48:28, hlundin-webrtc wrote: > Close, but no cigar. You still need to drop some letters. Two, to be specific. Argh! Thanks! Done. https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc (right): https://codereview.webrtc.org/2611223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/echo_canceller_test_tools.cc:21: // Produces a delayed signal copy of x. On 2017/01/23 19:48:28, hlundin-webrtc wrote: > Repeated comment. Drop this. Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611223003/#ps80001 (title: "Changes in response to reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16736) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16578) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/21221) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18036) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22002)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611223003/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...) android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20518)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611223003/#ps120001 (title: "Corrected usage of begin and end for array-view which caused compile error on android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/20829)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611223003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/4529)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611223003/#ps200001 (title: "Disabled DEATH tests that were causing memory leakage reports on test bots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485514193842900,
"parent_rev": "270048c070e091d4049132e0a761c997fba48a5b", "commit_rev":
"69221db53413bdca8cb1bacff11b3651f283dd95"}
Message was sent while issue was closed.
Description was changed from ========== Adding second layer of the echo canceller 3 functionality. This CL adds code to the BlockProcessor, which basically constitutes the second layer in echo canceller 3. The CL includes two incomplete classes (EchoRemover and EchoPathDelayEstimator) which will be completed in upcoming CLs. Because of this, some of the unittests are disabled until those are added. BUG=webrtc:6018 ========== to ========== Adding second layer of the echo canceller 3 functionality. This CL adds code to the BlockProcessor, which basically constitutes the second layer in echo canceller 3. The CL includes two incomplete classes (EchoRemover and EchoPathDelayEstimator) which will be completed in upcoming CLs. Because of this, some of the unittests are disabled until those are added. BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2611223003 Cr-Commit-Position: refs/heads/master@{#16319} Committed: https://chromium.googlesource.com/external/webrtc/+/69221db53413bdca8cb1bacff... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/69221db53413bdca8cb1bacff... |
