Adding fraction of filter divergence in AEC metrics.

webrtc/modules/audio_processing/utility/mean_calculator.h

+/*
+ *  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, thanks

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

Acknowledged.

+ *
+ *  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.
+ */
+
+#ifndef WEBRTC_MODULES_AUDIO_PROCESSING_UTILITY_MEAN_CALCULATOR_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_UTILITY_MEAN_CALCULATOR_H_
+

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

Until there are more uses of this class, I think it should not be placed in
utilities and instead be put into the aec folder and be tailormade to exactly
what it is needed there. As it is now, the only usage is in aec_core.cc where it
is used to store float values of 1.0 or 0.0 which I think makes it motivated to
write the code to exactly look at that.

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

As mentioned in an earlier reply, the reason of writing it this way is indeed to
make more use of it.  not in this CL though WDYT?

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

I think that it then is better to delay the introduction of the class until next
time it is needed , as the functionality in the class is not that well suited
for how it is used (to compute the mean of integers).

[minyue-webrtc, 2016/04/03 21:42:05]

Ok, sure. I will remove it

+#include <memory>
+#include <stddef.h>
+
+#include "webrtc/base/criticalsection.h"
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/base/optional.h"
+
+namespace webrtc {
+
+// MeanCalculator calculates the mean of a sample sequence of a certain length.
+class MeanCalculator {

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

In the code, this class is currently only used to store the values 1.0f and
0.0f. I think the class then should be replaced by an int/bool array wrapper.

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

replied in earlier comments

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

Acknowledged.

+ public:
+  explicit MeanCalculator(size_t window_length);

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

The name of this class is a bit too vague I think. It provides a  sliding window
mean computation over an array of values and it would be nice if it reflects
that. 

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

I am very bad at naming, do you have any suggestion?

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

What about SlidingWindowAverage?

+  ~MeanCalculator();
+
+  // Add one sample to the sequence.
+  void AddSample(float sample);
+
+  // 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> GetMean();
+
+  // Flush all samples added.
+  void Flush();

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

What about Clear() ?

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

I'd buy it.

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

Acknowledged.

+
+  // Determines if the window is full. This is a quick way of checking if the
+  // mean is ready.
+  bool IsWindowFull();

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

The return of an optional and this function provides redundant functionality. I
think one of them should be removed.

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

Should be declared const

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

I think that using GetMean() to determine IsWindowFull() is a bit too much. Not
only because it creates an Option, but also because the name does not reflect
this functionality.

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

Yes, thanks.

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

It totally depends on how it is to be used. Since neither of the methods is
actually yet used in the code (apart from in the unit test) I can only guess on
the intended usage so probably it would be more clear if you add usage of these
in the code.

In my mind, what I think you intend to do is something like:
A) rtc::Optional<float> mean = calc.GetMean();
   if (mean) {
     reportNewMean(*mean);
   }

As I see it, with IsWindowFull() that could instead be used as
B) if (calc.IsWindowFull()) {
     rtc::Optional<float> mean = calc.GetMean();
     RTC_DCHECK(mean);
     reportNewMean(*mean);
   }


That is the usage that I foresee and then I think both A) and B) makes sense,
but in either of these only one of the functionalities are used.

Therefore, as I don't think it makes sense to add functionality to the class
before it is really needed, I think the IsWindowFull() method should be removed.

+
+ private:
+  rtc::CriticalSection crit_;

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

Why is a lock required in this class? It is not currently accessed in a
concurrent manner, right?

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

There is no trouble on the current use case of it. But it needs to protect
itself on other future use cases.

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

The problem with a lock is that it adds a cost and I don't think that we should
add locks if there is no strong need for that. Furthermore, even though I agree
with you that there is no risk with the current code, locks are always risky in
terms of deadlocks.

Another argument against a lock is that it encourages using this in a
multi-threaded manner and that is something that we definitely should avoid.

Until there is a clear need for a lock I see no reason to add that.

+  size_t window_length_ GUARDED_BY(crit_);

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

This variable can be removed if you use a vector (be replaced by .size()).

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

Doesn't size() give the actual number of values added, rather than capacity. And
there seems no way to define and return capacity for vector<>, right?

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

The vector should be preallocated to the size of the vector, i.e., it should not
grow for each of the values that are added. Then the size reflects exactly what
window_length_ does.

(The capacity of the vector can be assessed by the capacity() method)

+  std::unique_ptr<float[]> buffer_ GUARDED_BY(crit_);

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

Please use a vector instead of a dynamically allocated float array. Then you get
the niceness of the vector class while still having the ability to dynamically
allocate the data.

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

ok.

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

Acknowledged.

+  size_t head_ GUARDED_BY(crit_);
+  bool full_ GUARDED_BY(crit_);
+  float sum_ GUARDED_BY(crit_);
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(MeanCalculator);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_UTILITY_MEAN_CALCULATOR_H