Adding full initial version of delay estimation functionality in echo canceller 3

webrtc/modules/audio_processing/aec3/down_sampler_4khz.cc

+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+#include "webrtc/modules/audio_processing/aec3/down_sampler_4khz.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/audio_processing/aec3/aec3_constants.h"
+#include "webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h"

[aleloi, 2017/01/27 15:37:47]

included i parent header.

[peah-webrtc, 2017/02/02 14:04:46]

Done.

+
+namespace webrtc {
+namespace {
+
+// [B,A] = butter(2,750/8000)
+const CascadedBiQuadFilter::BiQuadCoefficients kLowPassFilterCoefficients_8kHz =
+    {{0.0179f, 0.0357f, 0.0179f}, {-1.5879f, 0.6594f}};
+
+// [B,A] = butter(2,1500/16000)

[aleloi, 2017/01/27 15:37:47]

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.

[hlundin-webrtc, 2017/02/01 08:30:01]

I ask the same. The resulting coefficients are identical now, and I don't think
that was intended.

[peah-webrtc, 2017/02/02 14:04:46]

Oups! Yes, this became quite mixed up. Please see below.
Done.

[peah-webrtc, 2017/02/02 14:04:46]

Oups! Yes, this became quite mixed up. Please see below.
Done.

+const CascadedBiQuadFilter::BiQuadCoefficients
+    kLowPassFilterCoefficients_16kHz = {{0.0179f, 0.0357f, 0.0179f},
+                                        {-1.5879f, 0.6594f}};
+
+}  // namespace
+
+DownSampler4kHz::DownSampler4kHz(int sample_rate_hz)
+    : down_sampling_factor_(rtc::CheckedDivExact(kBlockSize, kSubBlockSize)),

[hlundin-webrtc, 2017/02/01 08:30:01]

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.

[peah-webrtc, 2017/02/02 14:04:46]

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.

+      low_pass_filter_(sample_rate_hz == 8000
+                           ? kLowPassFilterCoefficients_8kHz
+                           : kLowPassFilterCoefficients_16kHz,
+                       3) {

[aleloi, 2017/01/27 15:37:47]

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.

[peah-webrtc, 2017/02/02 14:04:46]

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?

+  RTC_DCHECK(ValidFullBandRate(sample_rate_hz));
+  RTC_DCHECK_EQ(4, down_sampling_factor_);

[aleloi, 2017/01/27 15:37:47]

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).

[hlundin-webrtc, 2017/02/01 08:30:01]

... but the name of the class would be misleading.

[peah-webrtc, 2017/02/02 14:04:46]

You are totally correct. I changed this now.

PTAL

[peah-webrtc, 2017/02/02 14:04:46]

Agree. PTAL

+}
+
+void DownSampler4kHz::DownSample(rtc::ArrayView<const float> in,
+                                 rtc::ArrayView<float> out) {
+  RTC_DCHECK_EQ(kBlockSize, in.size());
+  RTC_DCHECK_EQ(kSubBlockSize, out.size());
+  float x[kBlockSize];
+
+  // Limit the frequency content of the signal to avoid aliasing.
+  low_pass_filter_.Process(in, x);
+
+  // Downsample the signal.
+  for (size_t j = 0, k = 0; j < out.size(); ++j, k += down_sampling_factor_) {
+    RTC_DCHECK_GT(kBlockSize, k);
+    out[j] = x[k];
+  }
+}
+
+}  // namespace webrtc