Implement video frame acknowledgements in the chromoting protocol.

remoting/host/capture_scheduler.cc

Index: remoting/host/capture_scheduler.cc
diff --git a/remoting/host/capture_scheduler.cc b/remoting/host/capture_scheduler.cc
index ccd141ddcdc85f8c5cb8c1916fe41f12a17a4d06..3a6a9ab667aabc9020fcf42bd00b6c1a4ad8c36c 100644
--- a/remoting/host/capture_scheduler.cc
+++ b/remoting/host/capture_scheduler.cc
@@ -10,6 +10,7 @@
 #include "base/sys_info.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
+#include "remoting/proto/video.pb.h"
 
 namespace {
 
@@ -25,8 +26,13 @@ const int64 kDefaultMinimumIntervalMs = 33;
 // available while 1 means using 100% of all CPUs available.
 const double kRecordingCpuConsumption = 0.5;
 
+// Maximum number of captured frames in the encoding queue. Currently capturer
+// implementations do not allow to keep more than 2 DesktopFrame objects.
+static const int kMaxFramesInEncodingQueue = 2;
+
 // Maximum number of frames that can be processed simultaneously.
-static const int kMaxPendingFrames = 2;
+static const int kMaxPendingFrames = 4;
+static const int kMaxPendingFramesWithoutAcks = 2;
 
 }  // namespace
 
@@ -35,6 +41,7 @@ namespace remoting {
 // We assume that the number of available cores is constant.
 CaptureScheduler::CaptureScheduler(const base::Closure& capture_closure)
     : capture_closure_(capture_closure),
+      acks_supported_(false),
       tick_clock_(new base::DefaultTickClock()),
       capture_timer_(new base::Timer(false, false)),
       minimum_interval_(
@@ -42,9 +49,12 @@ CaptureScheduler::CaptureScheduler(const base::Closure& capture_closure)
       num_of_processors_(base::SysInfo::NumberOfProcessors()),
       capture_time_(kStatisticsWindow),
       encode_time_(kStatisticsWindow),
-      pending_frames_(0),
+      num_encoding_frames_(0),
+      num_sending_frames_(0),
+      num_unacknowledged_frames_(0),
       capture_pending_(false),
-      is_paused_(false) {
+      is_paused_(false),
+      next_frame_id_(0) {
   DCHECK(num_of_processors_);
 }
 
@@ -78,23 +88,45 @@ void CaptureScheduler::OnCaptureCompleted() {
   capture_time_.Record(
       (tick_clock_->NowTicks() - last_capture_started_time_).InMilliseconds());
 
+  ++num_encoding_frames_;
+
+  ScheduleNextCapture();
+}
+
+void CaptureScheduler::OnFrameEncoded(VideoPacket* packet) {
+  DCHECK(CalledOnValidThread());
+
+  // Set packet_id for the outgoing packet.
+  packet->set_frame_id(next_frame_id_);

[Wez, 2015/02/11 02:22:55]

Do we want to do that if ACKs are disabled?

[Sergey Ulanov, 2015/02/17 19:37:06]

Yes. In the current version of this CL protocol version stays the same. The idea
is that the host always sets this field to indicate to the client that it
expects ACKs. And then it assumes that the client doesn't support ACKs until it
receives the first ACK.

+  ++next_frame_id_;
+
+  // Update internal stats.
+  encode_time_.Record(packet->encode_time_ms());
+
+  --num_encoding_frames_;
+  ++num_sending_frames_;
+  ++num_unacknowledged_frames_;

[Wez, 2015/02/11 02:22:55]

This will grow unbounded if ACKs aren't enabled, right?

[Sergey Ulanov, 2015/02/17 19:37:06]

Right. And then it's ignored in ScheduleNextCapture(), unless we know that
client supports ACKs.

+
   ScheduleNextCapture();
 }
 
 void CaptureScheduler::OnFrameSent() {
   DCHECK(CalledOnValidThread());
 
-  // Decrement the pending capture count.
-  pending_frames_--;
-  DCHECK_GE(pending_frames_, 0);
+  --num_sending_frames_;
+  DCHECK_GE(num_sending_frames_, 0);
 
   ScheduleNextCapture();
 }
 
-void CaptureScheduler::OnFrameEncoded(base::TimeDelta encode_time) {
+void CaptureScheduler::ProcessVideoAck(scoped_ptr<VideoAck> video_ack) {
   DCHECK(CalledOnValidThread());
 
-  encode_time_.Record(encode_time.InMilliseconds());
+  acks_supported_ = true;

[Wez, 2015/02/11 02:22:55]

It seems strange to have this depend upon whether we ever receive an ACK, rather
than on the version of the protocol that the client chose.

[Sergey Ulanov, 2015/02/17 19:37:06]

See my comment in OnFrameEncoded(). It makes this CL simpler. Added a comment
here. If you are against this approach then I will add new video channel version
back.

[Wez, 2015/02/21 03:12:02]

It makes the CL simpler but arguably makes the resulting code more complex. :P

I think this approach is OK, though, provided we have a bug & TODO for cleaning
out the ACK-less paths once everything has been updated.

[Sergey Ulanov, 2015/02/23 17:35:38]

Opened crbug.com/460963

+
+  --num_unacknowledged_frames_;
+  DCHECK_GE(num_unacknowledged_frames_, 0);
+
   ScheduleNextCapture();
 }
 
@@ -102,9 +134,11 @@ void CaptureScheduler::SetTickClockForTest(
     scoped_ptr<base::TickClock> tick_clock) {
   tick_clock_ = tick_clock.Pass();
 }
+
 void CaptureScheduler::SetTimerForTest(scoped_ptr<base::Timer> timer) {
   capture_timer_ = timer.Pass();
 }
+
 void CaptureScheduler::SetNumOfProcessorsForTest(int num_of_processors) {
   num_of_processors_ = num_of_processors;
 }
@@ -112,8 +146,20 @@ void CaptureScheduler::SetNumOfProcessorsForTest(int num_of_processors) {
 void CaptureScheduler::ScheduleNextCapture() {
   DCHECK(CalledOnValidThread());
 
-  if (is_paused_ || pending_frames_ >= kMaxPendingFrames || capture_pending_)
+  if (is_paused_ || capture_pending_ ||
+      num_encoding_frames_ >= kMaxFramesInEncodingQueue) {
     return;
+  }
+
+  if (acks_supported_) {
+    if (num_encoding_frames_ + num_unacknowledged_frames_ >= kMaxPendingFrames)
+      return;
+  } else {
+    if (num_encoding_frames_ + num_sending_frames_ >=
+        kMaxPendingFramesWithoutAcks) {
+      return;
+    }
+  }
 
   // Delay by an amount chosen such that if capture and encode times
   // continue to follow the averages, then we'll consume the target
@@ -138,9 +184,6 @@ void CaptureScheduler::CaptureNextFrame() {
   DCHECK(!is_paused_);
   DCHECK(!capture_pending_);
 
-  pending_frames_++;
-  DCHECK_LE(pending_frames_, kMaxPendingFrames);
-
   capture_pending_ = true;
   last_capture_started_time_ = tick_clock_->NowTicks();
   capture_closure_.Run();