Gather history of capture and encode time determine next recoring delay

remoting/host/recording_rate_regulator.cc

Index: remoting/host/recording_rate_regulator.cc
diff --git a/remoting/host/recording_rate_regulator.cc b/remoting/host/recording_rate_regulator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ad6e85a893e48f5eceb979d698cdcf88473830a5
--- /dev/null
+++ b/remoting/host/recording_rate_regulator.cc
@@ -0,0 +1,84 @@
+// Copyright (c) 2011 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.
+
+#include "remoting/host/recording_rate_regulator.h"
+
+#include <algorithm>
+
+#include "base/sys_info.h"
+#include "base/time.h"
+
+namespace {
+
+// Window for averaging statistics on encode and capture time.
+const int kStatisticsWindow = 3;

[Wez, 2011/10/20 00:52:15]

What units is this in?

[Alpha Left Google, 2011/10/20 14:18:20]

There's no unit, just a divider.

[Wez, 2011/10/25 00:05:37]

Surely it's the time window over which the encode and capture statistics are
averaged, in which case it has time units?

[Alpha Left Google, 2011/10/31 18:47:24]

It's not a time window since it's not used for rate but a window to average
encode and capture time over multiple iterations. This value means the
iterations we take to average these values.

+
+// The hard limit is 20fps or 50ms per recording cycle. Faster than this
+// value causes significant slow down on the system.

[Wez, 2011/10/20 00:52:15]

That's true of current systems; why would it be true in future?

[Alpha Left Google, 2011/10/20 14:18:20]

I'm just gonna remove this comment.

+const base::TimeDelta kMinimumRecordingDelay =

[Sergey Ulanov, 2011/10/20 00:17:00]

This should be int64. Complex types are not allowed for statics, and it probably
requires static initializer.

[Alpha Left Google, 2011/10/31 18:47:24]

Done.

+    base::TimeDelta::FromMilliseconds(50);

[Sergey Ulanov, 2011/10/20 00:17:00]

IMO this is to high. It means that we cannot reach latency less than 50ms. maybe
20ms.

[Alpha Left Google, 2011/10/20 14:18:20]

It's not just whether hardware can handle it.. but also bandwidth too, given the
difficulty we have now I'd like to keep this until we get a better understand of
network problems.

[Sergey Ulanov, 2011/10/20 17:19:11]

I don't think this is related to bandwidth. We currently account for limited
bandwidth by limiting number of in-flight frames (kMaxRecordings).

[Wez, 2011/10/25 00:05:37]

Limiting the number of in-flight frames will restrict the frame rate on LFNs,
though.

I think Sergey's right; the lower bound should represent our ideal target
frame-rate and the capture throttling should rduce that down to a rate that
doesn't saturate the machine.

[Alpha Left Google, 2011/10/31 18:47:24]

Yes I agree that ideally this should be the ideal case we want to achieve but
given that we have problems with the rate controlling, doing so will just pump
more data to the send buffer to the stack which is not a good move until the
flow control problem is solved.

That being said, this change is about improving the situations due to system
load, I don't want to introduce more uncertainties.

+
+// This value is the ratio of processing time (i.e. encode and capture)
+// in a recording cycle vs duration of a recording cycle. In other words
+// this factor determines how much time we can spend on processing in
+// each recording cycle. A smaller value will give the system more idle
+// time and results in a lower recording frequency.
+const double kRecordingDelayFactor = 0.5;

[Wez, 2011/10/20 00:52:15]

Isn't that just a percentage processor usage, in effect?

[Alpha Left Google, 2011/10/31 18:47:24]

Done.

+
+}  // namespace
+
+namespace remoting {
+
+RecordingRateRegulator::RecordingRateRegulator()
+    : num_of_processors_(base::SysInfo::NumberOfProcessors()),
+      capture_time_(kStatisticsWindow),
+      encode_time_(kStatisticsWindow) {
+}
+
+RecordingRateRegulator::~RecordingRateRegulator() {
+}
+
+base::TimeDelta RecordingRateRegulator::NextRecordingDelay() {
+  // In general we use the history of capture time and encode time
+  // to determine next recording delay so that these two heavy
+  // operations don't saturate the system.
+  // The algorithm for choosing the next recording delay depends
+  // on number of processors and current system load.
+  // More specifically this is seperated into three cases:

[Wez, 2011/10/25 00:05:37]

nit: separated

[Alpha Left Google, 2011/10/31 18:47:24]

Done.

+  // - 1 Processor
+  //   Sum the average encoding time and average capture time and
+  //   divide this by a predefined factor.
+  // - 2 Processor
+  //   Choose the minimum between the average encoding time and
+  //   average capture time and divide this number by a predefined
+  //   factor. The reason is that one processor can be saturated
+  //   but not both processors.
+  // - 3 or more processors.
+  //   Operate as fast as possible. Even if two processors are
+  //   saturated there's still one left for doing other things.

[Wez, 2011/10/20 00:52:15]

We have the special-case for (2)?

Your scheme seems to assume that for 3 or more processors it's OK to use 2 of
them entirely, and that for 2 it's OK to use one entirely, in effect.

Why not just limit on the total %age usage across all cores, by summing the
encode and capture times, dividing down by the number of cores, and basing the
delay on that?

[Alpha Left Google, 2011/10/31 18:47:24]

I changed the whole logic.

+  base::TimeDelta delay;
+  if (num_of_processors_ == 1) {
+    delay = base::TimeDelta::FromMilliseconds(
+        (capture_time_.Average() + encode_time_.Average()) /
+        kRecordingDelayFactor);
+  } else if (num_of_processors_ == 2) {
+    delay = base::TimeDelta::FromMilliseconds(
+        std::min(capture_time_.Average(), encode_time_.Average()) /
+        kRecordingDelayFactor);
+  }
+
+  if (delay < kMinimumRecordingDelay)
+    return kMinimumRecordingDelay;
+  return delay;
+}
+
+void RecordingRateRegulator::RecordCaptureTime(int64 ms) {
+  capture_time_.Record(ms);
+}
+
+void RecordingRateRegulator::RecordEncodeTime(int64 ms) {
+  encode_time_.Record(ms);
+}
+
+}  // namespace remoting