Implement video frame acknowledgements in the chromoting protocol.

remoting/host/capture_scheduler.h

Index: remoting/host/capture_scheduler.h
diff --git a/remoting/host/capture_scheduler.h b/remoting/host/capture_scheduler.h
index b3136e78d84d88f5b526b830dd095893b10de7e9..161ffda86da63c042046c67c8431f83dd529d227 100644
--- a/remoting/host/capture_scheduler.h
+++ b/remoting/host/capture_scheduler.h
@@ -2,10 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// This class chooses a capture interval so as to limit CPU usage to not exceed
-// a specified %age. It bases this on the CPU usage of recent capture and encode
-// operations, and on the number of available CPUs.
-
 #ifndef REMOTING_HOST_CAPTURE_SCHEDULER_H_
 #define REMOTING_HOST_CAPTURE_SCHEDULER_H_
 
@@ -15,14 +11,27 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "remoting/base/running_average.h"
+#include "remoting/protocol/video_feedback_stub.h"
 
 namespace remoting {
 
+class VideoPacket;
+
 // CaptureScheduler is used by the VideoFramePump to schedule frame capturer,
 // taking into account capture delay, encoder delay, network bandwidth, etc.
-class CaptureScheduler : public base::NonThreadSafe {
+//
+// This class chooses a capture interval so as to limit CPU usage to not exceed
+// a specified percentage. It bases this on the CPU usage of recent capture and
+// encode operations, and on the number of available CPUs.
+//
+// CaptureScheduler also implements VideoFeedbackStub to receive frame
+// acknowledgments from the client. If the client supports acknowledgments
+// then it limits total number of captured frames that haven't been
+// acknowledged (currently the limit is set to 4 frames). Otherwise it limits

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

Explain this frame-count - e.g. is it chosen to allow some pipelining while
maintaining no more than 132ms latency (132ms = 4 * 33ms, which seems like an
awful lot of latency to allow)

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

This limit is for full roundtrip, including ACK message delivery, while we care
about latency only in one direction. So 132ms will be observed in the case when
you have low-bandwidth network with 0 latency, which is not realistic. I removed
this comment from here and added comments in capture_scheduler.cc about this
choice.

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

Comment still seems to be here?

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

Sorry. Cleaned up the comment now.

+// total number of frames in the encoding and sending queues to 2.
+class CaptureScheduler : public base::NonThreadSafe,
+                         public protocol::VideoFeedbackStub {
  public:
-  // |capture_closure| is called every time a new frame needs to be captured.
   explicit CaptureScheduler(const base::Closure& capture_closure);
   ~CaptureScheduler();
 
@@ -35,12 +44,16 @@ class CaptureScheduler : public base::NonThreadSafe {
   // Notifies the scheduler that a capture has been completed.
   void OnCaptureCompleted();
 
-  // Notifies the scheduler that a frame has been encoded.
-  void OnFrameEncoded(base::TimeDelta encode_time);
+  // Notifies the scheduler that a frame has been encoded. The scheduler can
+  // change |packet| if necessary, e.g. set |frame_id|.
+  void OnFrameEncoded(VideoPacket* packet);
 
-  // Notifies the scheduler that a frame has been sent.
+  // Notifies the scheduler that a frame hams been sent.
   void OnFrameSent();
 
+  // VideoFeedbackStub interface.
+  void ProcessVideoAck(scoped_ptr<VideoAck> video_ack) override;
+
   // Sets minimum interval between frames.
   void set_minimum_interval(base::TimeDelta minimum_interval) {
     minimum_interval_ = minimum_interval;
@@ -63,6 +76,9 @@ class CaptureScheduler : public base::NonThreadSafe {
 
   base::Closure capture_closure_;
 
+  // Set to true if the connection supports video frame acknowledgments.
+  bool acks_supported_;
+
   scoped_ptr<base::TickClock> tick_clock_;
 
   // Timer used to schedule CaptureNextFrame().
@@ -76,8 +92,14 @@ class CaptureScheduler : public base::NonThreadSafe {
   RunningAverage capture_time_;
   RunningAverage encode_time_;
 
-  // Total number of pending frames that are being captured, encoded or sent.
-  int pending_frames_;
+  // Number of frames pending encoding.
+  int num_encoding_frames_;
+
+  // Number of frames in the sending queue.
+  int num_sending_frames_;
+
+  // Number of outgoing frames for which we haven't received an acknowledgment.
+  int num_unacknowledged_frames_;
 
   // Set to true when capture is pending.
   bool capture_pending_;
@@ -87,6 +109,8 @@ class CaptureScheduler : public base::NonThreadSafe {
 
   bool is_paused_;
 
+  uint32_t next_frame_id_;

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

nit: Explain this member!

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

Done.

+
   DISALLOW_COPY_AND_ASSIGN(CaptureScheduler);
 };