Gather history of capture and encode time determine next recoring delay

remoting/host/capture_scheduler.cc

+// 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/capture_scheduler.h"
+
+#include <algorithm>
+
+#include "base/logging.h"
+#include "base/sys_info.h"
+#include "base/time.h"
+
+namespace {
+
+// Window for averaging statistics on encode and capture time.
+// This constant controls how many recorded instances are averaged.

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

Please word this more clearly.  Intuitively one would expect the window to
specify the time interval over which values are averaged, but this is actually
the number of samples over which to average.

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

Done.

+const int kStatisticsWindow = 3;

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

Why average over three previous samples?  Why not just consider the most recent
capture & encode; that way you can almost guarantee the CPU usage you require -
if one frame encodes very quickly because not much has changed then you'll delay
very little.

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

My intention is to have a relatively steady frame rate. If we choose this based
on the last sample the frame rate may changes quite a lot.

+
+// The hard limit is 20fps or 50ms per recording cycle.
+const int64 kMinimumRecordingDelay = 50;

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

This still seems unnecessarily low, given that this class should end up
rate-limiting appropriately based on the time taken to capture and encode.
Arguably, this should also be a parameter to the scheduler, not a hard-coded
constant.

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

You have a different opinion than Sergey here, he suggested to go lower to 20.
Anyway I don't think it's good to change this value now unless we solved the
flow control problems.

I was hoping that we can extend this class to do stuff like key press interval
to predict the best next capture start time, I would like to keep all the
parameters in this class so it's easier to experiment with parameters or other
algorithm (it will be in "some" class anyway, better keep it more searchable).

[Wez, 2011/10/31 22:05:53]

Sorry, I meant that this value would lead to a low frame rate...

+
+// Controls how much CPU time we can use for encode and capture.
+// This is capped at 50%.

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

I'd suggest rewording to comment to something describing what the range 0.0-1.0
mean, i.e. allow no CPU usage to allowing continuous use of an entire core.  I
think it'd be simpler to have this as a ratio of total CPU usage across all
cores.

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

Yes it's a ratio across all corres already.

+const double kRecordingCpuConsumption = 0.5;
+
+}  // namespace
+
+namespace remoting {
+
+CaptureScheduler::CaptureScheduler()
+    : num_of_processors_(base::SysInfo::NumberOfProcessors()),

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

nit: This means we don't cope properly with dynamic changes in the number of
available cores...

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

Done.

+      capture_time_(kStatisticsWindow),
+      encode_time_(kStatisticsWindow) {
+  DCHECK(num_of_processors_);
+}
+
+CaptureScheduler::~CaptureScheduler() {
+}
+
+base::TimeDelta CaptureScheduler::NextCaptureDelay() {
+  // The algorithm only uses CPU time spent in capture, encode and

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

typo: "... capture, encode and..." -> "... capture and encode, and the ..."

+  // number of processors to determine next capture time.

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

nit: processors -> cores?
nit: "determine next capture time" -> "choose the next capture interval"

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

I think we should follow the naming of base::SysInfo::NumberOfProcessors().
done.

+  // We want to make sure time spent in capture and encode doesn't
+  // exceed a predefined value, i.e. kRecordingCpuConsumption.

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

nit: This comment seems to reiterate the basic function of this component?

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

Done.

+  double delay =
+      (capture_time_.Average() + encode_time_.Average()) /

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

Note that the encode time will vary considerably depending on how much has
changed on-screen, whereas the capture time average only will on platforms where
we're using update notifications.  Where we're polling for changes capture time
will be pretty constant.  Also bear in mind that capture time may have
side-effects, e.g. if while capturing no other process can be rendering.

+      kRecordingCpuConsumption / num_of_processors_;

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

It's clearer to write this formula as:

  delay = (capture + encode) / (consumption * cores);

This formula treats kRecordingCpuConsumption as a proportion of CPU usage across
all cores, not a single core as was previously discussed.  Usage across all
cores makes more sense, I think, but you should make sure it's clear that that's
what the proportion means.

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

Done.

+
+  if (delay < kMinimumRecordingDelay)
+    return base::TimeDelta::FromMilliseconds(kMinimumRecordingDelay);
+  return base::TimeDelta::FromMilliseconds(delay);
+}
+
+void CaptureScheduler::RecordCaptureTime(base::TimeDelta capture_time) {
+  capture_time_.Record(capture_time.InMilliseconds());
+}
+
+void CaptureScheduler::RecordEncodeTime(base::TimeDelta encode_time) {
+  encode_time_.Record(encode_time.InMilliseconds());
+}
+
+}  // namespace remoting