FeedbackSignalAccumulator utility class for averaging feedback signals

content/browser/media/capture/time_weighted_average.h

Index: content/browser/media/capture/time_weighted_average.h
diff --git a/content/browser/media/capture/time_weighted_average.h b/content/browser/media/capture/time_weighted_average.h
new file mode 100644
index 0000000000000000000000000000000000000000..43f536b8e1b9a474f2e48fe3a789c5fee0514d14
--- /dev/null
+++ b/content/browser/media/capture/time_weighted_average.h
@@ -0,0 +1,73 @@
+// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_MEDIA_CAPTURE_TIME_WEIGHTED_AVERAGE_H_
+#define CONTENT_BROWSER_MEDIA_CAPTURE_TIME_WEIGHTED_AVERAGE_H_
+
+#include "base/time/time.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+// Utility class for maintaining an time-weighted moving average value of a

[hubbe, 2015/04/27 20:50:42]

I think time-weighted might be an accurate but confusing way to explain what
this class does. It might be better to just say that it handles irregular
updates correctly.

[miu, 2015/05/05 04:56:59]

Done.

+// metric value.

[hubbe, 2015/04/27 20:50:42]

It doesn't work with imperial values? :)

[miu, 2015/05/05 04:56:59]

Done.

+//
+// Usage note: Reset() must be called at least once before the first call to
+// Update().
+class CONTENT_EXPORT TimeWeightedAverage {
+ public:
+  // |time_constant| is the amount of time that must pass between two data
+  // points to move the average value halfway in-between.  Example: If
+  // |time_constant| is one second, then calling Reset(0.0, t=0s) and then
+  // Update(1.0, t=1s) will result in a moving average of 0.5.
+  explicit TimeWeightedAverage(base::TimeDelta time_constant);

[hubbe, 2015/04/27 20:50:42]

maybe call time_constant half_life instead?
Although, I'm confused, does this implement a moving average (windowed) or an
exponential decay function?

If this is a windowed average, then perhaps call it "window_time"?

[miu, 2015/05/05 04:56:59]

Done.  Yeah, half_life is a great name for this!  :)

Adjusted comments to eliminate "window" wordings.

+
+  // Erase all memory of historical values, re-starting with the given
+  // |starting_value|.
+  void Reset(double starting_value, base::TimeTicks timestamp);
+  base::TimeTicks last_reset_time() const { return last_reset_time_; }
+
+  // Update the average using the given |value| which was observed at the given
+  // |timestamp|.  If the timestamp was no more than one step out-of-order, the

[hubbe, 2015/04/27 20:50:42]

Wait, what's a step?

[miu, 2015/05/05 04:56:59]

Done.  Explained this further in class-level comment, and changed wording here
to be "one step out of chronological order" for clarity.

+  // update succeeds and this method returns true.  Otherwise the update has no
+  // effect and false is returned.
+  //
+  // Two or more updates at the same |timestamp| will have their values
+  // arithmetically averaged to form a single data point.  This single data
+  // point is then rolled into the moving average.
+  bool Update(double value, base::TimeTicks timestamp);
+
+  // Accessors to current state.
+  double current() const { return average_; }
+  base::TimeTicks latest_timestamp() const { return most_recent_timestamp_; }
+
+  // Convenience methods.
+  bool was_recently_updated(base::TimeTicks now) const {

[hubbe, 2015/04/27 20:50:42]

This seems like an odd function
I think "recent" needs to be decoupled from the decay rate.

[miu, 2015/05/05 04:56:59]

I got rid of this one.  I may or may not introduce along with code that uses it
in a later CL, with a more-proper name.

+    return (now - latest_timestamp()) <= time_constant_;
+  }
+  base::TimeDelta elapsed_since_reset() const {

[hubbe, 2015/04/27 20:50:42]

This also seems like an odd function.
From the name, I would have thought it would return TimeTicks::Now() -
last_reset_time().  If no updates have been given in a while, then this function
starts to lie...

[miu, 2015/05/05 04:56:59]

Removed this one too.  I may or may not introduce along with code that uses it
in a later CL, with a more-proper name.

+    return latest_timestamp() - last_reset_time();
+  }
+
+ private:
+  // Step the moving |average_| backward/forward.
+  void StepBackward(double last_value, base::TimeDelta elapsed);
+  void StepForward(double next_value, base::TimeDelta elapsed);
+
+  // Modify |most_recent_value_| to an unweigthed arithmetic average that
+  // includes |value|.
+  void MergeWithMostRecentDataPoint(double value);
+
+  const base::TimeDelta time_constant_;
+  base::TimeTicks last_reset_time_;
+  double average_;
+  double most_recent_value_;
+  int most_recent_count_;
+  base::TimeTicks most_recent_timestamp_;
+  base::TimeTicks second_most_recent_timestamp_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_CAPTURE_TIME_WEIGHTED_AVERAGE_H_