Added a new echo likelihood stat that reports the maximum value from a previous time period.

webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h

+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.

[hlundin-webrtc, 2017/01/13 09:06:22]

2017

[ivoc, 2017/01/13 12:12:29]

Oops, good point.

+ *
+ *  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_ECHO_DETECTOR_LIKELIHOOD_AGGREGATOR_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_DETECTOR_LIKELIHOOD_AGGREGATOR_H_
+
+#include <vector>
+
+namespace webrtc {
+
+// This class keeps a sliding window of past echo likelihood values, and can
+// calculate the maximum.
+class LikelihoodAggregator {

[the sun, 2017/01/13 12:15:57]

Rename to e.g. MovingMax or SlidingMax to
1. Avoid the need for the above comment.
2. Indicate what it actually does
3. ...which may allow it to be reused in other contexts
because really, the class has nothing to do with echo likelihood values - it
just keeps track of the max among the last in a series of floats...

[ivoc, 2017/01/13 13:24:23]

Great suggestion, done.

+ public:
+  explicit LikelihoodAggregator(size_t window_size);

[hlundin-webrtc, 2017/01/13 09:06:22]

What is the unit of the window size? Please, append the unit with the variable
name.

[ivoc, 2017/01/13 12:12:29]

There's one likelihood value per frame (10 ms of audio). I changed this to
window_size_frames.

[the sun, 2017/01/13 12:15:57]

How does that matter here?

[hlundin-webrtc, 2017/01/13 13:38:32]

Maybe it doesn't. I was biased by our offline discussions about window lengths
in seconds. Now that we are looking at samples, I agree that it is less
relevant.

+  ~LikelihoodAggregator();
+
+  // Add a new likelihood to the buffer. This will automatically remove the
+  // oldest likelihood value from the buffer.
+  void Update(float value);

[the sun, 2017/01/13 12:15:57]

Update() to me sounds like it would do some computation, but the computation is
actually done in max(). How about renaming this to Add() (which would make part
of the comment unnecessary) and max() to GetMax() or CalcMax()?

[ivoc, 2017/01/13 13:24:23]

In the new implementation it actually does a bit more computation (although
still amortized O(1)), is it okay to keep the name as is?

+  // Return the maximum likelihood value in the buffer.
+  float max() const;
+  // Fill the buffer with zeros.
+  void Clear();
+
+ private:
+  // The most recent echo likelihood values are stored in a rotating buffer.
+  std::vector<float> recent_values_;

[the sun, 2017/01/13 12:15:57]

Given that the size is 1000 elements in this use case, I think you've made the
right call simply searching through the vector when the max is retrieved
(insertion is what needs to be efficient).

[ivoc, 2017/01/13 13:24:23]

I initially thought so too, but it has now come to my attention that some apps
poll GetStats() rather frequently (roughly once per 100 ms), which would mean
that the Max() function should be efficient as well.

+  // Where to insert the next element in the rotating buffer.
+  int insertion_index_ = 0;

[hlundin-webrtc, 2017/01/13 09:06:22]

size_t

[ivoc, 2017/01/13 12:12:29]

Done.

+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_DETECTOR_LIKELIHOOD_AGGREGATOR_H_