Adding fraction of filter divergence in AEC metrics.

webrtc/modules/audio_processing/utility/mean_calculator.cc

+/*
+ *  Copyright 2015 The WebRTC Project Authors. All rights reserved.

[peah-webrtc, 2016/03/14 09:12:53]

The year should be 2016

[minyue-webrtc, 2016/03/14 14:51:42]

Yes

+ *
+ *  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/utility/mean_calculator.h"
+
+namespace webrtc {
+
+MeanCalculator::MeanCalculator(size_t window_length)
+    : window_length_(window_length),
+      buffer_(new float[window_length_]),
+      head_(0),
+      full_(false),
+      sum_(0.0f) {
+}
+
+MeanCalculator::~MeanCalculator() = default;

[peah-webrtc, 2016/03/14 09:12:53]

Is the destructor needed?

[minyue-webrtc, 2016/03/14 14:51:42]

deleted

[peah-webrtc, 2016/03/15 09:18:40]

No, it is just commented out.

+
+// Add one sample to the sequence.
+void MeanCalculator::AddSample(float sample) {
+  rtc::CritScope cs(&crit_);
+  if (full_)
+    sum_ -= buffer_[head_];

[peah-webrtc, 2016/03/14 09:12:53]

This is not guaranteed to give the correct sum. Actually, a sum of only positive
values may yield a negative value when updated in this way.

[minyue-webrtc, 2016/03/14 14:51:42]

I was also worried about the numerical accuracy. I think, as a mean calculator,
some numerical error should be tolerable. But I think we can use Kahan
Algorithm: https://en.wikipedia.org/wiki/Kahan_summation_algorithm

[peah-webrtc, 2016/03/15 09:18:40]

I don't agree that as a mean calculator an error is tolerable. It depends on the
intended usage of the mean. For instance, for the AEC that numerical error would
be fatal.

Since this is a general class, I think the numerical error must be clearly
specified.

As it is now, it is fully possible to have a mean that is negative although all
the values used when computing the mean are positive. And it is not at all clear
from the class that that might happen.

[peah-webrtc, 2016/03/15 09:18:40]

The computation of a sliding window arithmetic mean for floating point values is
not straightforward, and since the current usage of this class actually does not
require a mean of floating point values (as it currently uses integers), I think
you should postpone the implementation of this class.

+  sum_ += sample;
+  buffer_[head_] = sample;
+  head_ = (head_ + 1) % window_length_;
+  if (!full_ && head_ == 0)
+    full_ = true;
+}
+
+// Get the mean of the latest samples. Returns the mean if it is available,
+// otherwise null, which happens when the added samples have not fully filled
+// the window.
+rtc::Optional<float> MeanCalculator::GetMean() {
+  rtc::CritScope cs(&crit_);
+  if (IsWindowFull()) {

[peah-webrtc, 2016/03/14 09:12:53]

I think it is better to use full_ directly instead of call the IsWindowFull
function as the function call requires acquiring the lock twice.

[minyue-webrtc, 2016/03/14 14:51:42]

I agree

[peah-webrtc, 2016/03/15 09:18:40]

Acknowledged.

+    return rtc::Optional<float>(sum_ / window_length_);
+  } else {
+    return rtc::Optional<float>();
+  }
+}
+
+// Flush all samples added.
+void MeanCalculator::Flush() {
+  rtc::CritScope cs(&crit_);
+  head_ = 0;
+  full_ = false;
+  sum_ = 0.0f;
+}
+
+// Determines if the window is full. This is a quick way of checking if the
+// mean is ready.
+bool MeanCalculator::IsWindowFull() {
+  rtc::CritScope cs(&crit_);
+  return full_;
+}
+
+}  // namespace webrtc