Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644123002:
Adding full initial version of delay estimation functionality in echo canceller 3 (Closed)
|
Created:
3 years, 11 months ago by peah-webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, 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 full initial version of delay estimation functionality in echo
canceller 3
This CL adds code to the all the delay estimation functionality that is
available for the first version of echo canceller 3. The code completes
the class EchoPathDelayEstimator.
Note that this code does not yet include any handling of clock-drift so
there will be upcoming versions of this code.
Also note that the CL includes some minor changes in other files for
echo canceller 3.
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2644123002
Cr-Commit-Position: refs/heads/master@{#16489}
Committed: https://chromium.googlesource.com/external/webrtc/+/219208991b63b868d38d4c78fc3b04fa8370b636
Patch Set 1 #
Total comments: 114
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 6
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 38
Patch Set 4 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 5 : Changed name of the unittest #Patch Set 6 : Rebase #Patch Set 7 : Added missing includes #Patch Set 8 : Rebase #Messages
Total messages: 45 (19 generated)
Description was changed from ========== Adding full initial version of delay estimation functionality in echo canceller 3 This CL adds code to the all the delay estimation functionality that is available for the first version of echo canceller 3. The code completes the class EchoPathDelayEstimator. Note that this code does not yet include any handling of clock-drift so there will be upcoming versions of this code. Also note that the CL includes some minor changes in other files for echo canceller 3. BUG=webrtc:6018 ========== to ========== Adding full initial version of delay estimation functionality in echo canceller 3 This CL adds code to the all the delay estimation functionality that is available for the first version of echo canceller 3. The code completes the class EchoPathDelayEstimator. Note that this code does not yet include any handling of clock-drift so there will be upcoming versions of this code. Also note that the CL includes some minor changes in other files for echo canceller 3. BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org
A new CL for the echo canceller 3. PTAL
peah@webrtc.org changed reviewers: + aleloi@webrtc.org - ivoc@webrtc.org
Another Cl for Aec3, PTAL
Hi! Here are some comments for parts of the change. I'll try to finish the rest in a few days... https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:51: // Insert the new x sub block into x_buffer. Is x_sub and x_buffer references to other variable names? If so, please update. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:53: (render_buffer_.index + kSubBlockSize) % render_buffer_.data.size(); Will the buffer always have room for kSubBlockSize samples more? Can you add a DCHECK? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:14: #include "webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h" included i parent header. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:23: // [B,A] = butter(2,1500/16000) Should the second argument be the same as for 8kHz? From the documentation, it should be a fraction between 0 and 1 of the Nyquist limit of the input signal. But it's different for 8 and 16 kHz. IIUC, for 16kHz, the filter will let twice as much bandwidth through. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:35: 3) { IIUC, the number 3 is the number of cascaded filters. I don't know if it has a noticeable impact on performance compared to the other functianality, but if it has, I think it would be better to define it in aec-constants. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:37: RTC_DCHECK_EQ(4, down_sampling_factor_); I seem to be missing something... Why does the down sampler pick every 4:th sample both in 8khz and 16khz mode? Shouldn't it take every 2:nd for 8kHz? Does it effectively resample to 2kHz if the input is 8kHz? (In that case, my comment about the filter parameters is wrong). https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:23: explicit DownSampler4kHz(int sample_rate_hz_); I suggest adding a comment that sample_rate_hz_ is the *full* band sample rate and that the down sampler operates on 8 or 16 khz bands. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:25: // Downsamples the signal. I think assumptions about frame size should be documented here. Or not, because they are clearly documented with DCHECKS in the implementation. I can't decide, and leave it to you. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:18: #include "webrtc/base/array_view.h" Included in related header. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:75: // EXPECT_LT(0.25f, downsampled_power); Remove outcommented code https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:119: EXPECT_LT(0.5f * input_power, output_power); It seems wrong if as much as half the energy/time disappears. Can the output power really differ this much?
Very nice! I few comments inline. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:1: /* This file needs: 1. Reference to or comments containing equations to explain the basic algorithm. 2. Shorter variable names, matching equations. 3. More comments on the sub-steps. 4. More DCHECK for documenting the expected outputs of various sub-steps. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:54: std::copy(render.begin(), render.end(), RTC_DCHECK_LE(render_buffer_.index + render.size(), render_buffer_.data.size()); https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:63: size_t render_0_index = What is this? Can the name be shorter? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:71: const size_t render_buffer_part1_size = Please, comment on the fact that this is due to processing across the boundary of a circular buffer. Also, the name render_buffer_part1_size is very long and reduces readability. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:75: float s = 0.f; For the sake of my own understanding, I'm going to match this with the notation in https://en.wikipedia.org/wiki/Least_mean_squares_filter#Normalised_least_mean.... render <=> x correlations <=> h s <=> h^T*x capture <=> d update_factor <=> e = d - h^T*x normalized_update_factor <=> mu * e / x^T*x https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:80: s += correlations_[sub_correlator][lag] * render_k; Consider calculating s only when you know you'll need it, i.e., when signal_strength is large enough. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:119: const float error_anchor = // Energy of capture. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:120: std::accumulate(capture.begin(), capture.end(), 0.f, This should be the same as: std::inner_product(capture.begin(), capture.end(), capture.begin(), 0.f); https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:122: const size_t lag_estimate = std::distance( // Distance from start of sub_correlator to strongest (squared) correlation peak. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:128: What can you say about lag_estimate. DCHECK any lower and upper bounds. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:130: error_anchor - error, error < 0.3f * error_anchor, Explain the thresholding with 0.3. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:19: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" Fwd declare instead. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:30: LagEstimate() {} Is this needed? If so, = default. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:31: void Set(float accuracy, bool reliable, size_t lag, bool updated) { You could probably just as well have a ctor that sets the members according to input parameters. And then instead of my_struct.Set(a, b, c, d), use my_struct = LagEstimate(a, b, c, d). https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:51: // Updates the correlation with the values in x and y. No x and y in parameter list. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:52: void Update(rtc::ArrayView<const float> render, This class seems to be agnostic of the concepts render and capture. Stick to something more generic, like x and y :) https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:66: public: public not needed in struct. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:18: #include "webrtc/modules/audio_processing/aec3/correlator.h" Will it be sufficient to fwd declare Correlator::LagEstimate? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:19: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" Fwd declare. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:32: // estimate This is the same comment as above. But without . https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:111: EXPECT_TRUE(aggregated_lag); Nothing changed since last time you checked this. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:112: EXPECT_EQ(kArtificialLag1, *aggregated_lag); And this. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:115: kThresholdForRequiredIdenticalLagAggregates + 1; You should be able to break out this for loop into a helper function, and get rid of a lot of code dupe. Also, consider making a SCOPED_TRACE before calling the helper function. You won't have to populate the trace string at all, just an empty string will do, since the SCOPED_TRACE will at least output the line number it was called on. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:160: TEST(CorrelatorLagAggregator, DelayBeforeReliableAggregatedLag) { This test is an exact subset of the test above. Redundant. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:57: rtc::ArrayView<const Correlator::LagEstimate> lag_estimates = auto https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:59: EXPECT_EQ(kNumAlignmentShifts + 1, lag_estimates.size()); You are already checking this explicitly in a test below. So you don't have to check this in all other tests too. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:73: RTC_DCHECK(expected_most_accurate_lag_estimate); ASSERT_TRUE https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:78: if (k != static_cast<size_t>(*expected_most_accurate_lag_estimate)) { Why the cast? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:125: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { for (auto le : lag_estimates) { EXPECT_FALSE(le.reliable); } https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:137: constexpr size_t kWindowSizeSubBlocks = 32; These constexprs are the same for all tests. Consider putting them on top of the file. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:159: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { Consider range-based. Also, you can merge this and the next loop into one. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:23: // [B,A] = butter(2,1500/16000) On 2017/01/27 15:37:47, aleloi wrote: > Should the second argument be the same as for 8kHz? From the documentation, it > should be a fraction between 0 and 1 of the Nyquist limit of the input signal. > But it's different for 8 and 16 kHz. > > IIUC, for 16kHz, the filter will let twice as much bandwidth through. I ask the same. The resulting coefficients are identical now, and I don't think that was intended. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:31: : down_sampling_factor_(rtc::CheckedDivExact(kBlockSize, kSubBlockSize)), I would have expected the downsampling factor to be dependent on sample_rate_hz. When I see the name of this class and the input parameters, I interpret this as a utility that downsamples an input signal at 8000 or 16000 Hz sample rate down to 4000 Hz sample rate. If this is not what the class does, it needs a better name. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:37: RTC_DCHECK_EQ(4, down_sampling_factor_); On 2017/01/27 15:37:47, aleloi wrote: > I seem to be missing something... Why does the down sampler pick every 4:th > sample both in 8khz and 16khz mode? Shouldn't it take every 2:nd for 8kHz? Does > it effectively resample to 2kHz if the input is 8kHz? (In that case, my comment > about the filter parameters is wrong). ... but the name of the class would be misleading. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:46: input[k] = 32767.f * sin(2.f * kPi * 3.f / 8.f * k); Produces a sinusoid at frequency 3/8*fs. Comment about this. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:67: std::accumulate(input_to_evaluate.begin(), input_to_evaluate.end(), 0., std::inner_product https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:71: std::accumulate(output_to_evaluate.begin(), output_to_evaluate.end(), std::inner_product https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:82: TEST(DownSampler4kHz, NoImpactOnLowerFrequencies) { Soooo much code dupe with the above. Iiuc, the only things that differs in this test are kNumBlocks, the sinusoid frequency, and the expected output power. Please, consolidate. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (left): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:12: #include "webrtc/base/checks.h" You are still using checks. Don't delete this. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:33: down_sampling_factor_(capture_down_sampler_.GetDownSamplingFactor()), Can't you call capture_down_sampler_.GetDownSamplingFactor() when needed instead of caching it in a member variable? Redundancy leads to discrepancy... :) But, if the plan is to move the below logging outside of this class, then you'll probably need a getter method for the downsampling factor. I would still argue that the getter should just ask the capture_down_sampler_. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:1: WAT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:74: EXPECT_TRUE(estimated_delay_samples); ASSERT_TRUE, or the below "dereferencing" will crash the test binary if there is no value. Of course, the ASSERT_TRUE will exit this test case without completing remaining delay and sample rate values. The alternative is to do if (estimated_delay_samples) { EXPECT_NEAR(delay_samples, *estimated_delay_samples, 4); } else { ADD_FAILURE(); }
Thanks for the great comments! I have uploaded a new patch. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:1: /* On 2017/02/01 08:30:00, hlundin-webrtc wrote: > This file needs: > 1. Reference to or comments containing equations to explain the basic algorithm. > 2. Shorter variable names, matching equations. > 3. More comments on the sub-steps. > 4. More DCHECK for documenting the expected outputs of various sub-steps. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:51: // Insert the new x sub block into x_buffer. On 2017/01/27 15:37:47, aleloi wrote: > Is x_sub and x_buffer references to other variable names? If so, please update. x sub block refers to x subblock. Changed the name to x_buffer_. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:53: (render_buffer_.index + kSubBlockSize) % render_buffer_.data.size(); On 2017/01/27 15:37:47, aleloi wrote: > Will the buffer always have room for kSubBlockSize samples more? Can you add a > DCHECK? Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:54: std::copy(render.begin(), render.end(), On 2017/02/01 08:30:00, hlundin-webrtc wrote: > RTC_DCHECK_LE(render_buffer_.index + render.size(), render_buffer_.data.size()); Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:63: size_t render_0_index = On 2017/02/01 08:30:00, hlundin-webrtc wrote: > What is this? Can the name be shorter? This is the index of the first value in the render buffer to use for the matched filtering. I changed the name to better reflect that. PTAL. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:71: const size_t render_buffer_part1_size = On 2017/02/01 08:30:00, hlundin-webrtc wrote: > Please, comment on the fact that this is due to processing across the boundary > of a circular buffer. > Also, the name render_buffer_part1_size is very long and reduces readability. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:75: float s = 0.f; On 2017/02/01 08:30:00, hlundin-webrtc wrote: > For the sake of my own understanding, I'm going to match this with the notation > in > https://en.wikipedia.org/wiki/Least_mean_squares_filter#Normalised_least_mean.... > > render <=> x > correlations <=> h > s <=> h^T*x > capture <=> d > update_factor <=> e = d - h^T*x > normalized_update_factor <=> mu * e / x^T*x Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:80: s += correlations_[sub_correlator][lag] * render_k; On 2017/02/01 08:30:00, hlundin-webrtc wrote: > Consider calculating s only when you know you'll need it, i.e., when > signal_strength is large enough. I don't think that's possible. In order to compute the matched filter accuracy, s is needed. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:119: const float error_anchor = On 2017/02/01 08:30:00, hlundin-webrtc wrote: > // Energy of capture. Yes, but I prefer to name it differently as it is used as an anchor for the matched filter error. To me that name makes the computation of the LagEstimate information more readable. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:120: std::accumulate(capture.begin(), capture.end(), 0.f, On 2017/02/01 08:30:00, hlundin-webrtc wrote: > This should be the same as: > std::inner_product(capture.begin(), capture.end(), capture.begin(), 0.f); Nice! Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:122: const size_t lag_estimate = std::distance( On 2017/02/01 08:30:00, hlundin-webrtc wrote: > // Distance from start of sub_correlator to strongest (squared) correlation > peak. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:128: On 2017/02/01 08:30:00, hlundin-webrtc wrote: > What can you say about lag_estimate. DCHECK any lower and upper bounds. There are no bounds, except from 0 and filters_[n].size(). Since the max_element method ensures that the lag_estimate is within these bounds, no DCHECKS are required. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.cc:130: error_anchor - error, error < 0.3f * error_anchor, On 2017/02/01 08:30:00, hlundin-webrtc wrote: > Explain the thresholding with 0.3. It is a tuning parameter. I made a constant and adjusted the naming to describe what it does. I'm not sure that we should describe it more than that in the text. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:19: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" On 2017/02/01 08:30:00, hlundin-webrtc wrote: > Fwd declare instead. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:30: LagEstimate() {} On 2017/02/01 08:30:00, hlundin-webrtc wrote: > Is this needed? If so, = default. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:31: void Set(float accuracy, bool reliable, size_t lag, bool updated) { On 2017/02/01 08:30:00, hlundin-webrtc wrote: > You could probably just as well have a ctor that sets the members according to > input parameters. And then instead of my_struct.Set(a, b, c, d), use my_struct = > LagEstimate(a, b, c, d). Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:51: // Updates the correlation with the values in x and y. On 2017/02/01 08:30:00, hlundin-webrtc wrote: > No x and y in parameter list. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:52: void Update(rtc::ArrayView<const float> render, On 2017/02/01 08:30:00, hlundin-webrtc wrote: > This class seems to be agnostic of the concepts render and capture. Stick to > something more generic, like x and y :) It is not in that it requires a certain causality between x and y. I'd therefore rather stick with render and capture, as that quite well describes that relationship, and instead in the implementation replace these with x and y. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:66: public: On 2017/02/01 08:30:01, hlundin-webrtc wrote: > public not needed in struct. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:18: #include "webrtc/modules/audio_processing/aec3/correlator.h" On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Will it be sufficient to fwd declare Correlator::LagEstimate? No, that does not seem to work. I get the error "forward declaration of struct cannot have a nested name specifier". https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:19: #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h" On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Fwd declare. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:32: // estimate On 2017/02/01 08:30:01, hlundin-webrtc wrote: > This is the same comment as above. But without . Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:111: EXPECT_TRUE(aggregated_lag); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Nothing changed since last time you checked this. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:112: EXPECT_EQ(kArtificialLag1, *aggregated_lag); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > And this. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:115: kThresholdForRequiredIdenticalLagAggregates + 1; On 2017/02/01 08:30:01, hlundin-webrtc wrote: > You should be able to break out this for loop into a helper function, and get > rid of a lot of code dupe. Also, consider making a SCOPED_TRACE before calling > the helper function. You won't have to populate the trace string at all, just an > empty string will do, since the SCOPED_TRACE will at least output the line > number it was called on. I added the SCOPED_TRACE. I don't think, however, that it makes sense to break out the loops into a helper function. These are two loops, with common loop bodies of 3 lines. Even though creating a helper function would avoid the duplication of those 3 lines, that action would actually cause the length of the code (measured in lines) to increase. Furthermore, it would move the testing to another place in the code which would I think would reduce the code readability. I think the best alternative would be to create a local lambda so that part of the testing is not moved to another place. That would, however, end up in at least 1 line longer code. (using a helper method instead of a lambda would result end up in a similar result). WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator_unittest.cc:160: TEST(CorrelatorLagAggregator, DelayBeforeReliableAggregatedLag) { On 2017/02/01 08:30:01, hlundin-webrtc wrote: > This test is an exact subset of the test above. Redundant. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:57: rtc::ArrayView<const Correlator::LagEstimate> lag_estimates = On 2017/02/01 08:30:01, hlundin-webrtc wrote: > auto Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:59: EXPECT_EQ(kNumAlignmentShifts + 1, lag_estimates.size()); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > You are already checking this explicitly in a test below. So you don't have to > check this in all other tests too. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:73: RTC_DCHECK(expected_most_accurate_lag_estimate); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > ASSERT_TRUE Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:78: if (k != static_cast<size_t>(*expected_most_accurate_lag_estimate)) { On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Why the cast? Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:125: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { On 2017/02/01 08:30:01, hlundin-webrtc wrote: > for (auto le : lag_estimates) { > EXPECT_FALSE(le.reliable); > } Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:137: constexpr size_t kWindowSizeSubBlocks = 32; On 2017/02/01 08:30:01, hlundin-webrtc wrote: > These constexprs are the same for all tests. Consider putting them on top of the > file. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:159: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Consider range-based. Also, you can merge this and the next loop into one. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:14: #include "webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h" On 2017/01/27 15:37:47, aleloi wrote: > included i parent header. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:23: // [B,A] = butter(2,1500/16000) On 2017/01/27 15:37:47, aleloi wrote: > Should the second argument be the same as for 8kHz? From the documentation, it > should be a fraction between 0 and 1 of the Nyquist limit of the input signal. > But it's different for 8 and 16 kHz. > > IIUC, for 16kHz, the filter will let twice as much bandwidth through. Oups! Yes, this became quite mixed up. Please see below. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:23: // [B,A] = butter(2,1500/16000) On 2017/02/01 08:30:01, hlundin-webrtc wrote: > On 2017/01/27 15:37:47, aleloi wrote: > > Should the second argument be the same as for 8kHz? From the documentation, it > > should be a fraction between 0 and 1 of the Nyquist limit of the input signal. > > But it's different for 8 and 16 kHz. > > > > IIUC, for 16kHz, the filter will let twice as much bandwidth through. > > I ask the same. The resulting coefficients are identical now, and I don't think > that was intended. Oups! Yes, this became quite mixed up. Please see below. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:31: : down_sampling_factor_(rtc::CheckedDivExact(kBlockSize, kSubBlockSize)), On 2017/02/01 08:30:01, hlundin-webrtc wrote: > I would have expected the downsampling factor to be dependent on sample_rate_hz. > When I see the name of this class and the input parameters, I interpret this as > a utility that downsamples an input signal at 8000 or 16000 Hz sample rate down > to 4000 Hz sample rate. If this is not what the class does, it needs a better > name. Yes, you are fully correct. Since the downsampling is done to convert blocks to subblocks the 4 kHz is wrong for 8 kHz signals. I instead renamed this to a decimator, with fixed decimation but with different anti-aliasing filter cutoff for 8 kHz compared to the other rates. PTAL. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:35: 3) { On 2017/01/27 15:37:47, aleloi wrote: > IIUC, the number 3 is the number of cascaded filters. I don't know if it has a > noticeable impact on performance compared to the other functianality, but if it > has, I think it would be better to define it in aec-constants. My purpose of aec3_constants is that it should contain the constants that are shared throughout the AEC3 code (or at least through parts of it. In my mind, this constant is different as it is only used here. It does affect the performance both in terms of aliasing and computational complexity, and in that it is a tuning parameter. I would, however, still want to keep it local to this class as it only concerns this class. WDYT? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:37: RTC_DCHECK_EQ(4, down_sampling_factor_); On 2017/01/27 15:37:47, aleloi wrote: > I seem to be missing something... Why does the down sampler pick every 4:th > sample both in 8khz and 16khz mode? Shouldn't it take every 2:nd for 8kHz? Does > it effectively resample to 2kHz if the input is 8kHz? (In that case, my comment > about the filter parameters is wrong). You are totally correct. I changed this now. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc:37: RTC_DCHECK_EQ(4, down_sampling_factor_); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > On 2017/01/27 15:37:47, aleloi wrote: > > I seem to be missing something... Why does the down sampler pick every 4:th > > sample both in 8khz and 16khz mode? Shouldn't it take every 2:nd for 8kHz? > Does > > it effectively resample to 2kHz if the input is 8kHz? (In that case, my > comment > > about the filter parameters is wrong). > > ... but the name of the class would be misleading. Agree. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:23: explicit DownSampler4kHz(int sample_rate_hz_); On 2017/01/27 15:37:47, aleloi wrote: > I suggest adding a comment that sample_rate_hz_ is the *full* band sample rate > and that the down sampler operates on 8 or 16 khz bands. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:25: // Downsamples the signal. On 2017/01/27 15:37:47, aleloi wrote: > I think assumptions about frame size should be documented here. Or not, because > they are clearly documented with DCHECKS in the implementation. I can't decide, > and leave it to you. I agree. This is a big drawback with ArrayView and vectors as you cannot ensure this until runtime. I have in the upcoming CLs changed the code to instead use std::array which checks this at compile time. I can only change this partly in this CL. Ok if I do this partial change, and wait with the other changes until an upcoming CL? https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:18: #include "webrtc/base/array_view.h" On 2017/01/27 15:37:47, aleloi wrote: > Included in related header. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:46: input[k] = 32767.f * sin(2.f * kPi * 3.f / 8.f * k); On 2017/02/01 08:30:01, hlundin-webrtc wrote: > Produces a sinusoid at frequency 3/8*fs. Comment about this. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:67: std::accumulate(input_to_evaluate.begin(), input_to_evaluate.end(), 0., On 2017/02/01 08:30:01, hlundin-webrtc wrote: > std::inner_product Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:71: std::accumulate(output_to_evaluate.begin(), output_to_evaluate.end(), On 2017/02/01 08:30:01, hlundin-webrtc wrote: > std::inner_product Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:75: // EXPECT_LT(0.25f, downsampled_power); On 2017/01/27 15:37:47, aleloi wrote: > Remove outcommented code Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:82: TEST(DownSampler4kHz, NoImpactOnLowerFrequencies) { On 2017/02/01 08:30:02, hlundin-webrtc wrote: > Soooo much code dupe with the above. Iiuc, the only things that differs in this > test are kNumBlocks, the sinusoid frequency, and the expected output power. > Please, consolidate. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz_unittest.cc:119: EXPECT_LT(0.5f * input_power, output_power); On 2017/01/27 15:37:47, aleloi wrote: > It seems wrong if as much as half the energy/time disappears. Can the output > power really differ this much? True. I increased it a bit, but the impact on 8 kHz is actually non-negligible so I cannot make it very tight. I think it is fine for now though, as it is more of a sanity check than a benchmark. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (left): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:12: #include "webrtc/base/checks.h" On 2017/02/01 08:30:02, hlundin-webrtc wrote: > You are still using checks. Don't delete this. Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.cc:33: down_sampling_factor_(capture_down_sampler_.GetDownSamplingFactor()), On 2017/02/01 08:30:02, hlundin-webrtc wrote: > Can't you call capture_down_sampler_.GetDownSamplingFactor() when needed instead > of caching it in a member variable? Redundancy leads to discrepancy... :) > > But, if the plan is to move the below logging outside of this class, then you'll > probably need a getter method for the downsampling factor. I would still argue > that the getter should just ask the capture_down_sampler_. I refactored it a bit. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:1: On 2017/02/01 08:30:02, hlundin-webrtc wrote: > WAT? Done. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:74: EXPECT_TRUE(estimated_delay_samples); On 2017/02/01 08:30:02, hlundin-webrtc wrote: > ASSERT_TRUE, or the below "dereferencing" will crash the test binary if there is > no value. Of course, the ASSERT_TRUE will exit this test case without completing > remaining delay and sample rate values. > The alternative is to do > if (estimated_delay_samples) { > EXPECT_NEAR(delay_samples, *estimated_delay_samples, 4); > } else { > ADD_FAILURE(); > } Great suggestion! Done.
Just a few more comments. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:125: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { On 2017/02/02 14:04:46, peah-webrtc wrote: > On 2017/02/01 08:30:01, hlundin-webrtc wrote: > > for (auto le : lag_estimates) { > > EXPECT_FALSE(le.reliable); > > } > > Done. That's not equivalent. lag_estimates has kNumAlignmentShift + 1 elements, but the previous loop checks kNumAlignmentShift. https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:62: EXPECT_EQ(kNumAlignmentShifts + 1, lag_estimates.size()); Why do we receive k+1 estimates from the matched filter, but only look a the first k? I haven't understood the estimator code yet; it may become more clear then... Also, I agree with hlundin@ that this check is superfluous, because it's already done for several matched filter parameter sets below. https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:105: TEST(MatchedFilter, LagNotUpdatedForLowLevelRender) { The test names got mixed up. This is the test for uncorrelated signals, and the test below named LagNotReliable... is for low level signals.
Patchset #3 (id:40001) has been deleted
Thanks! Good points! I did some further changes to reduce the risk of confusion. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_unittest.cc:125: for (size_t k = 0; k < kNumAlignmentShifts; ++k) { On 2017/02/02 16:33:11, aleloi wrote: > On 2017/02/02 14:04:46, peah-webrtc wrote: > > On 2017/02/01 08:30:01, hlundin-webrtc wrote: > > > for (auto le : lag_estimates) { > > > EXPECT_FALSE(le.reliable); > > > } > > > > Done. > > That's not equivalent. lag_estimates has kNumAlignmentShift + 1 elements, but > the previous loop checks kNumAlignmentShift. You are correct. I mixed this up. It is now corrected. PTAL https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:62: EXPECT_EQ(kNumAlignmentShifts + 1, lag_estimates.size()); On 2017/02/02 16:33:12, aleloi wrote: > Why do we receive k+1 estimates from the matched filter, but only look a the > first k? I haven't understood the estimator code yet; it may become more clear > then... > > Also, I agree with hlundin@ that this check is superfluous, because it's already > done for several matched filter parameter sets below. The reason is that the number of shifts are the number of times the alignment is shifted. If you have one matched filter, and shift the alignment of that once, you get two matched filters. I see that this is misleading, so I'll change this so that it is more intuitive. PTAL https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_unittest.cc:105: TEST(MatchedFilter, LagNotUpdatedForLowLevelRender) { On 2017/02/02 16:33:12, aleloi wrote: > The test names got mixed up. This is the test for uncorrelated signals, and the > test below named LagNotReliable... is for low level signals. Oups. Good find! Done.
... and a few comments more! I'w almost through! https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:25: // Downsamples the signal. On 2017/02/02 14:04:46, peah-webrtc wrote: > On 2017/01/27 15:37:47, aleloi wrote: > > I think assumptions about frame size should be documented here. Or not, > because > > they are clearly documented with DCHECKS in the implementation. I can't > decide, > > and leave it to you. > > I agree. This is a big drawback with ArrayView and vectors as you cannot ensure > this until runtime. I have in the upcoming CLs changed the code to instead use > std::array which checks this at compile time. I can only change this partly in > this CL. > > Ok if I do this partial change, and wait with the other changes until an > upcoming CL? SGTM! Great that fixed-size array could be used! https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3 (right): https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3:1: /usr/local/google/home/peah/dev/webrtc-checkout4/src/webrtc/modules/audio_processing/aec3: Included by mistake? https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:29: EchoPathDelayEstimator(ApmDataDumper* data_dumper, int sample_rate_hz); Does the delay estimator need to know the sample rate? Since it only operates on the lowest band and does the same operations for 8 and 16k, maybe we don't need this argument. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:74: EXPECT_NEAR(delay_samples, *estimated_delay_samples, 4); Is the 4 here because of the down sampling factor? https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:58: x_buffer_.data.begin() + x_buffer_.index + x.size()); I suggest replacing both these lines with std::copy(x.rbegin(), x.rend(), x_buffer_.data.begin() + x_buffer_.index); https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter.h (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.h:15: #include <array> <array> should go first.
Almost there. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:52: void Update(rtc::ArrayView<const float> render, On 2017/02/02 14:04:45, peah-webrtc wrote: > On 2017/02/01 08:30:00, hlundin-webrtc wrote: > > This class seems to be agnostic of the concepts render and capture. Stick to > > something more generic, like x and y :) > > It is not in that it requires a certain causality between x and y. I'd therefore > rather stick with render and capture, as that quite well describes that > relationship, and instead in the implementation replace these with x and y. > > WDYT? Acknowledged. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:18: #include "webrtc/modules/audio_processing/aec3/correlator.h" On 2017/02/02 14:04:45, peah-webrtc wrote: > On 2017/02/01 08:30:01, hlundin-webrtc wrote: > > Will it be sufficient to fwd declare Correlator::LagEstimate? > > No, that does not seem to work. I get the error "forward declaration of struct > cannot have a nested name specifier". Acknowledged. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.cc:32: std::array<float, kBlockSize> x; What is the cost of this allocation? The same as float x[kBlockSize]? Or is dynamic allocation involved? https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:37: void ProduceDecimatedSinusoidalOutputPower(int rate, sample_rate https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:43: // Produce a sinusoid of frequency 3/8 * sample rate. Two spaces between "sinusoid" and "of". And comment is no longer accurate. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:62: RTC_DCHECK_GT(kNumBlocks, kNumStartupBlocks); ASSERT_GT. Don't crash the test binary. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:71: input_to_evaluate.begin(), 0.) / Looks a bit odd with just "0."; maybe write "0.f" instead? https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:75: output_to_evaluate.begin(), 0.) / Same here. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:117: TEST(DecimatorBy4, WrongOutputSize) { The name of the test is not consistent with the comment, or what the test actually does. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:120: You can drop this blank line. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:89: // x * x. Add to comment that this is cumulative on top of previous result. It is easy to miss that x2_sum is the starting value to inner_product. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:94: // Apply the matched filter as filter * x. Same here. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:52: // Require the same lag to be detected 10 times in a row before co nsidering "co nsidering" https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:53: // it Orphaned words in these two lines. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator_unittest.cc:124: SCOPED_TRACE(""); If you're not branching out to a subfunction, you won't need the SCOPED_TRACEs either. As the code is now, any failing EXPECT_* in this code will point to unique lines.
https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.cc:32: std::array<float, kBlockSize> x; On 2017/02/06 09:13:18, hlundin-webrtc wrote: > What is the cost of this allocation? The same as float x[kBlockSize]? Or is > dynamic allocation involved? It's statically allocated: http://stackoverflow.com/a/39548345
https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.cc:32: std::array<float, kBlockSize> x; On 2017/02/06 09:24:42, aleloi wrote: > On 2017/02/06 09:13:18, hlundin-webrtc wrote: > > What is the cost of this allocation? The same as float x[kBlockSize]? Or is > > dynamic allocation involved? > > It's statically allocated: http://stackoverflow.com/a/39548345 Thanks. No problems then.
Thanks for another great round of comments! I've uploaded a new patch in response to the feedback. PTAL https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator.h:52: void Update(rtc::ArrayView<const float> render, On 2017/02/06 09:13:18, hlundin-webrtc wrote: > On 2017/02/02 14:04:45, peah-webrtc wrote: > > On 2017/02/01 08:30:00, hlundin-webrtc wrote: > > > This class seems to be agnostic of the concepts render and capture. Stick to > > > something more generic, like x and y :) > > > > It is not in that it requires a certain causality between x and y. I'd > therefore > > rather stick with render and capture, as that quite well describes that > > relationship, and instead in the implementation replace these with x and y. > > > > WDYT? > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/correlator_lag_aggregator.h:18: #include "webrtc/modules/audio_processing/aec3/correlator.h" On 2017/02/06 09:13:18, hlundin-webrtc wrote: > On 2017/02/02 14:04:45, peah-webrtc wrote: > > On 2017/02/01 08:30:01, hlundin-webrtc wrote: > > > Will it be sufficient to fwd declare Correlator::LagEstimate? > > > > No, that does not seem to work. I get the error "forward declaration of struct > > cannot have a nested name specifier". > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/down_sampler_4khz.h (right): https://codereview.webrtc.org/2644123002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/down_sampler_4khz.h:25: // Downsamples the signal. On 2017/02/03 15:46:48, aleloi wrote: > On 2017/02/02 14:04:46, peah-webrtc wrote: > > On 2017/01/27 15:37:47, aleloi wrote: > > > I think assumptions about frame size should be documented here. Or not, > > because > > > they are clearly documented with DCHECKS in the implementation. I can't > > decide, > > > and leave it to you. > > > > I agree. This is a big drawback with ArrayView and vectors as you cannot > ensure > > this until runtime. I have in the upcoming CLs changed the code to instead use > > std::array which checks this at compile time. I can only change this partly in > > this CL. > > > > Ok if I do this partial change, and wait with the other changes until an > > upcoming CL? > > SGTM! Great that fixed-size array could be used! :-) https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3 (right): https://codereview.webrtc.org/2644123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3:1: /usr/local/google/home/peah/dev/webrtc-checkout4/src/webrtc/modules/audio_processing/aec3: On 2017/02/03 15:46:48, aleloi wrote: > Included by mistake? Great find! Thanks!!! That is for sure not intended to be there. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.cc:32: std::array<float, kBlockSize> x; On 2017/02/06 09:13:18, hlundin-webrtc wrote: > What is the cost of this allocation? The same as float x[kBlockSize]? Or is > dynamic allocation involved? Afaics, std::array is a template that encapsulates an array of static size. That should mean that if it is instantiated on the stack, the array itself should be in the stack. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:37: void ProduceDecimatedSinusoidalOutputPower(int rate, On 2017/02/06 09:13:18, hlundin-webrtc wrote: > sample_rate Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:43: // Produce a sinusoid of frequency 3/8 * sample rate. On 2017/02/06 09:13:18, hlundin-webrtc wrote: > Two spaces between "sinusoid" and "of". And comment is no longer accurate. Ah, thanks! Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:62: RTC_DCHECK_GT(kNumBlocks, kNumStartupBlocks); On 2017/02/06 09:13:18, hlundin-webrtc wrote: > ASSERT_GT. > Don't crash the test binary. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:71: input_to_evaluate.begin(), 0.) / On 2017/02/06 09:13:18, hlundin-webrtc wrote: > Looks a bit odd with just "0."; maybe write "0.f" instead? I actually think that 0.0 would be the correct here, as the output is double. However, I think it is just confusing to not consistently use floats in this code so I change the doubles to float, which means that I can do the same for the constants. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:75: output_to_evaluate.begin(), 0.) / On 2017/02/06 09:13:18, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:117: TEST(DecimatorBy4, WrongOutputSize) { On 2017/02/06 09:13:18, hlundin-webrtc wrote: > The name of the test is not consistent with the comment, or what the test > actually does. Not fully sure that I understood your comment correctly but I changed the wording of the comment in the code a bit. What it should mean is that it verifies that the output parameter is not null. PTAL https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:120: On 2017/02/06 09:13:18, hlundin-webrtc wrote: > You can drop this blank line. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator.h:29: EchoPathDelayEstimator(ApmDataDumper* data_dumper, int sample_rate_hz); On 2017/02/03 15:46:48, aleloi wrote: > Does the delay estimator need to know the sample rate? Since it only operates on > the lowest band and does the same operations for 8 and 16k, maybe we don't need > this argument. That is correct. It is no longer needed. I removed it now. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_path_delay_estimator_unittest.cc:74: EXPECT_NEAR(delay_samples, *estimated_delay_samples, 4); On 2017/02/03 15:46:48, aleloi wrote: > Is the 4 here because of the down sampling factor? Yes, that is the reason. I added a comment to clarify this. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:58: x_buffer_.data.begin() + x_buffer_.index + x.size()); On 2017/02/03 15:46:48, aleloi wrote: > I suggest replacing both these lines with > > std::copy(x.rbegin(), x.rend(), x_buffer_.data.begin() + x_buffer_.index); > Great! Thanks! I did not know of that one! Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:89: // x * x. On 2017/02/06 09:13:19, hlundin-webrtc wrote: > Add to comment that this is cumulative on top of previous result. It is easy to > miss that x2_sum is the starting value to inner_product. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.cc:94: // Apply the matched filter as filter * x. On 2017/02/06 09:13:18, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter.h (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter.h:15: #include <array> On 2017/02/03 15:46:48, aleloi wrote: > <array> should go first. Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:52: // Require the same lag to be detected 10 times in a row before co nsidering On 2017/02/06 09:13:19, hlundin-webrtc wrote: > "co nsidering" Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator.cc:53: // it On 2017/02/06 09:13:19, hlundin-webrtc wrote: > Orphaned > words > in > these > two > lines. :-) Thanks! Done. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/matched_filter_lag_aggregator_unittest.cc:124: SCOPED_TRACE(""); On 2017/02/06 09:13:19, hlundin-webrtc wrote: > If you're not branching out to a subfunction, you won't need the SCOPED_TRACEs > either. As the code is now, any failing EXPECT_* in this code will point to > unique lines. Ah, thanks. I definitely misunderstood how SCOPED_TRACE works. Done.
LGTM with one remaining nit. Great work! https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:117: TEST(DecimatorBy4, WrongOutputSize) { On 2017/02/06 11:25:38, peah-webrtc wrote: > On 2017/02/06 09:13:18, hlundin-webrtc wrote: > > The name of the test is not consistent with the comment, or what the test > > actually does. > > Not fully sure that I understood your comment correctly but I changed the > wording of the comment in the code a bit. What it should mean is that it > verifies that the output parameter is not null. > > PTAL But the test name is WrongOutputSize.
Thanks! I changed the name in a new patch. https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc (right): https://codereview.webrtc.org/2644123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4_unittest.cc:117: TEST(DecimatorBy4, WrongOutputSize) { On 2017/02/06 14:33:34, hlundin-webrtc wrote: > On 2017/02/06 11:25:38, peah-webrtc wrote: > > On 2017/02/06 09:13:18, hlundin-webrtc wrote: > > > The name of the test is not consistent with the comment, or what the test > > > actually does. > > > > Not fully sure that I understood your comment correctly but I changed the > > wording of the comment in the code a bit. What it should mean is that it > > verifies that the output parameter is not null. > > > > PTAL > > But the test name is WrongOutputSize. (facepalm) Thanks! I cannot see how I could miss that. Done.
LGTM! https://codereview.webrtc.org/2644123002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.h (right): https://codereview.webrtc.org/2644123002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.h:29: void Decimate(rtc::ArrayView<const float> in, Were there plans for converting 'in' to std::array as well in the next CL (since we know it's size)?
Thanks for the great review feedback!!! https://codereview.webrtc.org/2644123002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/decimator_by_4.h (right): https://codereview.webrtc.org/2644123002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/decimator_by_4.h:29: void Decimate(rtc::ArrayView<const float> in, On 2017/02/07 14:23:13, aleloi wrote: > Were there plans for converting 'in' to std::array as well in the next CL (since > we know it's size)? Yes, we should definitely do that. It may, however, be in a cleanup CL after he next CL.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2644123002/#ps100001 (title: "Changed name of the unittest")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17035) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22471) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21216)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2644123002/#ps160001 (title: "Added missing includes")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17233) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17058) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22496) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21246)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) 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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17061) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22499) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21250)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2644123002/#ps180001 (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: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17063)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486558727318990,
"parent_rev": "d4ed7f59e4866d812cffa89a10feefabe1733bb2", "commit_rev":
"219208991b63b868d38d4c78fc3b04fa8370b636"}
Message was sent while issue was closed.
Description was changed from ========== Adding full initial version of delay estimation functionality in echo canceller 3 This CL adds code to the all the delay estimation functionality that is available for the first version of echo canceller 3. The code completes the class EchoPathDelayEstimator. Note that this code does not yet include any handling of clock-drift so there will be upcoming versions of this code. Also note that the CL includes some minor changes in other files for echo canceller 3. BUG=webrtc:6018 ========== to ========== Adding full initial version of delay estimation functionality in echo canceller 3 This CL adds code to the all the delay estimation functionality that is available for the first version of echo canceller 3. The code completes the class EchoPathDelayEstimator. Note that this code does not yet include any handling of clock-drift so there will be upcoming versions of this code. Also note that the CL includes some minor changes in other files for echo canceller 3. BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2644123002 Cr-Commit-Position: refs/heads/master@{#16489} Committed: https://chromium.googlesource.com/external/webrtc/+/219208991b63b868d38d4c78f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/219208991b63b868d38d4c78f... |
