More reliable keep-alive video packets.

Previously the host was not sending empty keep alive messages when
video stream is paused or when it's blocked on the encoder, this
triggers reconnect too often.
Refactored keep-alive logic in VideoScheduler so now it always
sends keep-alive messages when there is no other activity on the
stream.

BUG=376528,375568

Review URL: https://codereview.chromium.org/292373002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272790 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 58 insertions, 23 deletions

diff --git a/remoting/host/video_scheduler.cc b/remoting/host/video_scheduler.cc
index 7fc1983..a346d04 100644
--- a/remoting/host/video_scheduler.cc
+++ b/remoting/host/video_scheduler.cc
@@ -30,6 +30,10 @@ namespace remoting {
 // TODO(hclam): Move this value to CaptureScheduler.
 static const int kMaxPendingFrames = 2;
 
+// Interval between empty keep-alive frames. These frames are sent only
+// when there are no real video frames being sent.
+static const int kKeepAlivePacketIntervalMs = 500;
+
 VideoScheduler::VideoScheduler(
     scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> encode_task_runner,
@@ -68,16 +72,22 @@ void VideoScheduler::OnCaptureCompleted(webrtc::DesktopFrame* frame) {
 
   capture_pending_ = false;
 
+  if (!frame) {
+    LOG(ERROR) << "Capture failed.";
+    return;
+  }
+
   scoped_ptr<webrtc::DesktopFrame> owned_frame(frame);
 
-  if (frame) {
-    scheduler_.RecordCaptureTime(
-        base::TimeDelta::FromMilliseconds(frame->capture_time_ms()));
-  }
+  scheduler_.RecordCaptureTime(
+      base::TimeDelta::FromMilliseconds(frame->capture_time_ms()));
 
-  encode_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::EncodeFrame, this,
-                            base::Passed(&owned_frame), sequence_number_));
+  // Encode and send only non-empty frames.
+  if (!frame->updated_region().is_empty()) {
+    encode_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&VideoScheduler::EncodeFrame, this,
+                              base::Passed(&owned_frame), sequence_number_));
+  }
 
   // If a frame was skipped, try to capture it again.
   if (did_skip_frame_) {
@@ -123,8 +133,10 @@ void VideoScheduler::Stop() {
   cursor_stub_ = NULL;
   video_stub_ = NULL;
 
-  capture_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&VideoScheduler::StopOnCaptureThread, this));
+  keep_alive_timer_.reset();
+
+  capture_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&VideoScheduler::StopOnCaptureThread, this));
 }
 
 void VideoScheduler::Pause(bool pause) {
@@ -172,6 +184,9 @@ void VideoScheduler::StartOnCaptureThread() {
   capturer_->Start(this);
 
   capture_timer_.reset(new base::OneShotTimer<VideoScheduler>());
+  keep_alive_timer_.reset(new base::DelayTimer<VideoScheduler>(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(kKeepAlivePacketIntervalMs),
+      this, &VideoScheduler::SendKeepAlivePacket));
 
   // Capture first frame immedately.
   CaptureNextFrame();
@@ -249,19 +264,39 @@ void VideoScheduler::SendVideoPacket(scoped_ptr<VideoPacket> packet) {
     return;
 
   video_stub_->ProcessVideoPacket(
-      packet.Pass(), base::Bind(&VideoScheduler::VideoFrameSentCallback, this));
+      packet.Pass(), base::Bind(&VideoScheduler::OnVideoPacketSent, this));
 }
 
-void VideoScheduler::VideoFrameSentCallback() {
+void VideoScheduler::OnVideoPacketSent() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
   if (!video_stub_)
     return;
 
+  keep_alive_timer_->Reset();
+
   capture_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VideoScheduler::FrameCaptureCompleted, this));
 }
 
+void VideoScheduler::SendKeepAlivePacket() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  if (!video_stub_)
+    return;
+
+  video_stub_->ProcessVideoPacket(
+      scoped_ptr<VideoPacket>(new VideoPacket()),
+      base::Bind(&VideoScheduler::OnKeepAlivePacketSent, this));
+}
+
+void VideoScheduler::OnKeepAlivePacketSent() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  if (keep_alive_timer_)
+    keep_alive_timer_->Reset();
+}
+
 void VideoScheduler::SendCursorShape(
     scoped_ptr<protocol::CursorShapeInfo> cursor_shape) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
@@ -278,17 +313,7 @@ void VideoScheduler::EncodeFrame(
     scoped_ptr<webrtc::DesktopFrame> frame,
     int64 sequence_number) {
   DCHECK(encode_task_runner_->BelongsToCurrentThread());
-
-  // If there is nothing to encode then send an empty keep-alive packet.
-  if (!frame || frame->updated_region().is_empty()) {
-    scoped_ptr<VideoPacket> packet(new VideoPacket());
-    packet->set_client_sequence_number(sequence_number);
-    network_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&VideoScheduler::SendVideoPacket, this,
-                              base::Passed(&packet)));
-    capture_task_runner_->DeleteSoon(FROM_HERE, frame.release());
-    return;
-  }
+  DCHECK(!frame->updated_region().is_empty());
 
   scoped_ptr<VideoPacket> packet = encoder_->Encode(*frame);
   packet->set_client_sequence_number(sequence_number);
diff --git a/remoting/host/video_scheduler.h b/remoting/host/video_scheduler.h
index f2eabd9..94fc795 100644
--- a/remoting/host/video_scheduler.h
+++ b/remoting/host/video_scheduler.h
@@ -140,7 +140,13 @@ class VideoScheduler : public base::RefCountedThreadSafe<VideoScheduler>,
 
   // Callback passed to |video_stub_| for the last packet in each frame, to
   // rate-limit frame captures to network throughput.
-  void VideoFrameSentCallback();
+  void OnVideoPacketSent();
+
+  // Called by |keep_alive_timer_|.
+  void SendKeepAlivePacket();
+
+  // Callback for |video_stub_| called after a keep-alive packet is sent.
+  void OnKeepAlivePacketSent();
 
   // Send updated cursor shape to client.
   void SendCursorShape(scoped_ptr<protocol::CursorShapeInfo> cursor_shape);
@@ -173,6 +179,10 @@ class VideoScheduler : public base::RefCountedThreadSafe<VideoScheduler>,
   // Timer used to schedule CaptureNextFrame().
   scoped_ptr<base::OneShotTimer<VideoScheduler> > capture_timer_;
 
+  // Timer used to ensure that we send empty keep-alive frames to the client
+  // even when the video stream is paused or encoder is busy.
+  scoped_ptr<base::DelayTimer<VideoScheduler> > keep_alive_timer_;
+
   // The number of frames being processed, i.e. frames that we are currently
   // capturing, encoding or sending. The value is capped at 2 to minimize
   // latency.