FeedbackSignalAccumulator utility class for averaging feedback signals

content/browser/media/capture/time_weighted_average.h

+// 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 exponentially-decaying average value of a
+// metric whose updates occur at undetermined time intervals and are mostly in
+// chronological order.  If updates occur more than one stop out-of-sequence,

[hubbe, 2015/05/06 17:54:49]

"one stop"
How much is that?
Is the "step size" related to half_life, or is it 1.0?

It seems odd for this class to care about chronological order.
Without knowing what the use case looks like, I would think that a DCHECK()
would be a better way to handle out-of-order values.

[miu, 2015/05/09 22:08:21]

As discussed face-to-face, I removed all the out-of-order foo.

+// they are ignored.  For example, updates for t=1,2,3,4 or t=1,2,4,3 are all
+// accounted for in the average as if they had arrived in-order, but the update
+// for t=2 in the sequence t=1,3,4,2 will be ignored.
+//
+// Usage note: Reset() must be called at least once before the first call to
+// Update().
+class CONTENT_EXPORT TimeWeightedAverage {
+ public:
+  // |half_life| is the amount of time that must pass between two data points to
+  // move the average value halfway in-between.  Example: If |half_life| 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 half_life);

[hubbe, 2015/05/06 17:54:49]

Actually, half_life might not be the right name, because it implies decay.
(Which I thought this class was doing last time I read this file.)

I'm not sure if there is a better name though. Maybe crossover time or something
could work...

[miu, 2015/05/09 22:08:21]

IMO, it works really well.  It's describing the half-life of its "memory" of
prior updates.

+
+  // 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 is no more than one step out of
+  // chronological order, the 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

[hubbe, 2015/05/06 17:54:49]

This kind of seems like an odd thing to do. Does this mean that

   Update(5, t=1.0);
   Update(10, t=1.0);

will affect the output differently from:

  Update(5, t=1.0);
  Update(10, t=1.0+epsilon);

?

Or will they do the same thing?
(If they do the same thing, this comment is redundant.)

[miu, 2015/05/09 22:08:21]

As discussed face-to-face, I really don't want to take the arithmetic average of
all values at the same timestamp.  Instead, I want the max.  This made the code
much simpler, and was the right thing to do.

+  // arithmetically averaged to form a single data point.  This single data
+  // point is then rolled into the 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_; }
+
+ private:
+  // Step the computation of |average_| backward/forward.
+  void Undo(double last_value, base::TimeDelta elapsed);
+  void Apply(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 half_life_;

[hubbe, 2015/05/06 17:54:49]

These variables needs comments too.
In particular, I have no idea what most_recent_count_ does.

[miu, 2015/05/09 22:08:21]

Done.

+  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_