Implement video frame acknowledgements in the chromoting protocol.

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/default_tick_clock.h"
 #include "base/time/time.h"
+#include "remoting/proto/video.pb.h"
 
 namespace {
 
 // Number of samples to average the most recent capture and encode time
 // over.
 const int kStatisticsWindow = 3;
 
 // The hard limit is 30fps or 33ms per recording cycle.
 const int64 kDefaultMinimumIntervalMs = 33;
 
 // Controls how much CPU time we can use for encode and capture.
 // Range of this value is between 0 to 1. 0 means using 0% of of all CPUs
 // 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
 
 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_(
           base::TimeDelta::FromMilliseconds(kDefaultMinimumIntervalMs)),
       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_);
 }
 
 CaptureScheduler::~CaptureScheduler() {
 }
 
 void CaptureScheduler::Start() {
   DCHECK(CalledOnValidThread());
 
   ScheduleNextCapture();
@@ skipping 13 matching lines @@
   }
 }
 
 void CaptureScheduler::OnCaptureCompleted() {
   DCHECK(CalledOnValidThread());
 
   capture_pending_ = false;
   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();
 }
 
 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;
 }
 
 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
   // fraction of CPU across all cores.
   base::TimeDelta delay =
       std::max(minimum_interval_,
                base::TimeDelta::FromMilliseconds(
                    (capture_time_.Average() + encode_time_.Average()) /
                    (kRecordingCpuConsumption * num_of_processors_)));
 
   // Account for the time that has passed since the last capture.
   delay = std::max(base::TimeDelta(), delay - (tick_clock_->NowTicks() -
                                                last_capture_started_time_));
 
   capture_timer_->Start(
       FROM_HERE, delay,
       base::Bind(&CaptureScheduler::CaptureNextFrame, base::Unretained(this)));
 }
 
 void CaptureScheduler::CaptureNextFrame() {
   DCHECK(CalledOnValidThread());
   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();
 }
 
 }  // namespace remoting