Gather history of capture and encode time determine next recoring delay

remoting/host/screen_recorder.cc

Index: remoting/host/screen_recorder.cc
diff --git a/remoting/host/screen_recorder.cc b/remoting/host/screen_recorder.cc
index 98266e3cf02bf768fe5cd5ec1f5eca3e2fc39a76..ab49427e0b87874b8649cc147765f11121803b15 100644
--- a/remoting/host/screen_recorder.cc
+++ b/remoting/host/screen_recorder.cc
@@ -11,6 +11,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop_proxy.h"
 #include "base/stl_util.h"
+#include "base/sys_info.h"
 #include "base/task.h"
 #include "base/time.h"
 #include "remoting/base/capture_data.h"
@@ -25,14 +26,7 @@ using remoting::protocol::ConnectionToClient;
 
 namespace remoting {
 
-// By default we capture 20 times a second. This number is obtained by
-// experiment to provide good latency.
-static const double kDefaultCaptureRate = 20.0;
-
 // Maximum number of frames that can be processed similtaneously.
-// TODO(sergeyu): Should this be set to 1? Or should we change
-// dynamically depending on how fast network and CPU are? Experement
-// with it.
 static const int kMaxRecordings = 2;

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

Can we move this to rate controller class? E.g. you can pass |recordings_| to
NextRecordingDelay(), and it would return -1 if |recordings_| is to high and we
need to wait for one next frame to be processed.

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

I think it'll be better to do it separately. It's not a requirement to this
improvement.

[Sergey Ulanov, 2011/10/31 20:17:18]

add TODO for this?

 
 ScreenRecorder::ScreenRecorder(
@@ -49,13 +43,18 @@ ScreenRecorder::ScreenRecorder(
       is_recording_(false),
       network_stopped_(false),
       encoder_stopped_(false),
+      max_recordings_(kMaxRecordings),
       recordings_(0),
       frame_skipped_(false),
-      max_rate_(kDefaultCaptureRate),
       sequence_number_(0) {
   DCHECK(capture_loop_);
   DCHECK(encode_loop_);
   DCHECK(network_loop_);
+
+  // If the system has only one processor there's no use to have simutaneous

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

typo: "simutaneous"

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

Done.

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

Done.

+  // recording sessions, it will just create excessive system load.

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

The rate-limiting should result in this, surely?

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

Actually max_recordings_ should be 1 here. My bad here.

+  if (base::SysInfo::NumberOfProcessors() < 2)
+    max_recordings_ = 2;

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

Don't get it. kMaxRecording = 2, so max_recordings_ is set to 2 ether way.

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

I set it to 1, mistake here.

 }
 
 ScreenRecorder::~ScreenRecorder() {
@@ -84,11 +83,6 @@ void ScreenRecorder::Stop(const base::Closure& done_task) {
       &ScreenRecorder::DoStopOnNetworkThread, this, done_task));
 }
 
-void ScreenRecorder::SetMaxRate(double rate) {
-  capture_loop_->PostTask(
-      FROM_HERE, NewRunnableMethod(this, &ScreenRecorder::DoSetMaxRate, rate));
-}
-
 void ScreenRecorder::AddConnection(
     scoped_refptr<ConnectionToClient> connection) {
   capture_loop_->PostTask(
@@ -158,27 +152,13 @@ void ScreenRecorder::DoStart() {
   DoCapture();
 }
 
-void ScreenRecorder::DoSetMaxRate(double max_rate) {
-  DCHECK_EQ(capture_loop_, MessageLoop::current());
-
-  // TODO(hclam): Should also check for small epsilon.
-  DCHECK_GT(max_rate, 0.0) << "Rate is too small.";
-
-  max_rate_ = max_rate;
-
-  // Restart the timer with the new rate.
-  if (is_recording_) {
-    capture_timer_.Stop();
-    StartCaptureTimer();
-  }
-}
-
 void ScreenRecorder::StartCaptureTimer() {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
-  base::TimeDelta interval = base::TimeDelta::FromMilliseconds(
-      static_cast<int>(base::Time::kMillisecondsPerSecond / max_rate_));
-  capture_timer_.Start(FROM_HERE, interval, this, &ScreenRecorder::DoCapture);
+  capture_timer_.Start(FROM_HERE,
+                       regulator_.NextRecordingDelay(),
+                       this,
+                       &ScreenRecorder::DoCapture);
 }
 
 void ScreenRecorder::DoCapture() {
@@ -186,19 +166,24 @@ void ScreenRecorder::DoCapture() {
   // Make sure we have at most two oustanding recordings. We can simply return
   // if we can't make a capture now, the next capture will be started by the
   // end of an encode operation.
-  if (recordings_ >= kMaxRecordings || !is_recording_) {
+  if (recordings_ >= max_recordings_ || !is_recording_) {
     frame_skipped_ = true;
     return;
   }
 
-  if (frame_skipped_) {
+  if (frame_skipped_)
     frame_skipped_ = false;
-    capture_timer_.Reset();
-  }
 
   // At this point we are going to perform one capture so save the current time.
   ++recordings_;
-  DCHECK_LE(recordings_, kMaxRecordings);
+  DCHECK_LE(recordings_, max_recordings_);
+
+  // Before doing a capture schedule for the next one.
+  capture_timer_.Stop();

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

Do we need this? DoCapture() is supposed to be called by the timer, so the timer
should not be running at this point because it is OneShotTimer.

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

We shouldn't hit this case but there's no harm doing this.

+  capture_timer_.Start(FROM_HERE,
+                       regulator_.NextRecordingDelay(),
+                       this,
+                       &ScreenRecorder::DoCapture);
 
   // And finally perform one capture.
   capture_start_time_ = base::Time::Now();
@@ -217,6 +202,7 @@ void ScreenRecorder::CaptureDoneCallback(
     int capture_time = static_cast<int>(
         (base::Time::Now() - capture_start_time_).InMilliseconds());
     capture_data->set_capture_time_ms(capture_time);
+    regulator_.RecordCaptureTime(capture_time);
 
     // The best way to get this value is by binding the sequence number to
     // the callback when calling CaptureInvalidRects(). However the callback
@@ -384,6 +370,7 @@ void ScreenRecorder::EncodedDataAvailableCallback(VideoPacket* packet) {
     int encode_time = static_cast<int>(
         (base::Time::Now() - encode_start_time_).InMilliseconds());
     packet->set_encode_time_ms(encode_time);
+    regulator_.RecordEncodeTime(encode_time);
   }
 
   network_loop_->PostTask(