Limit target encoder bitrate change frequency in the frame scheduler.

remoting/protocol/webrtc_frame_scheduler_simple.cc

Index: remoting/protocol/webrtc_frame_scheduler_simple.cc
diff --git a/remoting/protocol/webrtc_frame_scheduler_simple.cc b/remoting/protocol/webrtc_frame_scheduler_simple.cc
index b84f9db3228b75c26dbe79162fe5c2916e9ba5de..cecb01e01de5218722523eb086d3bdc40954867e 100644
--- a/remoting/protocol/webrtc_frame_scheduler_simple.cc
+++ b/remoting/protocol/webrtc_frame_scheduler_simple.cc
@@ -45,6 +45,11 @@ const int kBigFrameThresholdPixels = 300000;
 // encoded "big" frame may be too large to be delivered to the client quickly.
 const int kEstimatedBytesPerMegapixel = 100000;
 
+// Interval over which the bandwidth estimates is averaged to set target encoder
+// bitrate.
+constexpr base::TimeDelta kBandwidthAveragingInterval =
+    base::TimeDelta::FromSeconds(1);
+
 int64_t GetRegionArea(const webrtc::DesktopRegion& region) {
   int64_t result = 0;
   for (webrtc::DesktopRegion::Iterator r(region); !r.IsAtEnd(); r.Advance()) {
@@ -55,11 +60,52 @@ int64_t GetRegionArea(const webrtc::DesktopRegion& region) {
 
 }  // namespace
 
+WebrtcFrameSchedulerSimple::EncoderBitrateFilter::EncoderBitrateFilter() {}
+WebrtcFrameSchedulerSimple::EncoderBitrateFilter::~EncoderBitrateFilter() {}
+
+void WebrtcFrameSchedulerSimple::EncoderBitrateFilter::SetBandwidthEstimate(
+    int bandwidth_kbps,
+    base::TimeTicks now) {
+  bandwidth_samples_.push(std::make_pair(now, bandwidth_kbps));
+  bandwidth_samples_sum_ += bandwidth_kbps;
+
+  while (bandwidth_samples_.size() > 1 &&

[Jamie, 2016/12/09 23:39:18]

It took me a while to work out why you needed at least two elements for this
loop before I realized that you don't want to remove the one you just added. I
think it might be clearer to clean up the old entries (using !empty() as a
conditional, which is a lot more self-documenting) and then adding the new one
afterwards.

[Sergey Ulanov, 2016/12/10 01:22:57]

Done.

+         now - bandwidth_samples_.front().first > kBandwidthAveragingInterval) {
+    bandwidth_samples_sum_ -= bandwidth_samples_.front().second;
+    bandwidth_samples_.pop();
+  }
+}
+
+int WebrtcFrameSchedulerSimple::EncoderBitrateFilter::GetTargetBitrateKbps(
+    webrtc::DesktopSize size,
+    base::TimeTicks now) {
+  DCHECK(!bandwidth_samples_.empty());
+
+  // TODO(sergeyu): This logic is applicable only to VP8. Reconsider it for
+  // VP9.
+  int bandwidth_estimate = bandwidth_samples_sum_ / bandwidth_samples_.size();
+  int minimum_bitrate =
+      static_cast<int64_t>(kVp8MinimumTargetBitrateKbpsPerMegapixel) *
+      size.width() * size.height() / 1000000LL;
+  int target_bitrate = std::max(minimum_bitrate, bandwidth_estimate);
+
+  // Update encoder bitrate only when it changes by more than 30%. This is
+  // necessary because the encoder resets internal state it's reconfigured and

[Jamie, 2016/12/09 23:39:18]

s/it's/when it's/

[Sergey Ulanov, 2016/12/10 01:22:57]

Done.

+  // this causes visible drop in quality.
+  if (current_target_bitrate_ == 0 ||
+      std::abs(target_bitrate - current_target_bitrate_) >
+          current_target_bitrate_ / 3) {

[Jamie, 2016/12/09 23:39:18]

Add a named constant with rationale to explain why 33% was chosen.

[Sergey Ulanov, 2016/12/10 01:22:56]

Done.

+    current_target_bitrate_ = target_bitrate;
+  }
+  return current_target_bitrate_;
+}
+
 WebrtcFrameSchedulerSimple::WebrtcFrameSchedulerSimple()
     : pacing_bucket_(LeakyBucket::kUnlimitedDepth, 0),
       frame_processing_delay_us_(kStatsWindow),
       updated_region_area_(kStatsWindow),
       weak_factory_(this) {}
+
 WebrtcFrameSchedulerSimple::~WebrtcFrameSchedulerSimple() {}
 
 void WebrtcFrameSchedulerSimple::OnKeyFrameRequested() {
@@ -75,10 +121,11 @@ void WebrtcFrameSchedulerSimple::OnChannelParameters(int packet_loss,
   rtt_estimate_ = rtt;
 }
 
-void WebrtcFrameSchedulerSimple::OnTargetBitrateChanged(int bitrate_kbps) {
+void WebrtcFrameSchedulerSimple::OnTargetBitrateChanged(int bandwidth_kbps) {
   DCHECK(thread_checker_.CalledOnValidThread());
   base::TimeTicks now = base::TimeTicks::Now();
-  pacing_bucket_.UpdateRate(bitrate_kbps * 1000 / 8, now);
+  pacing_bucket_.UpdateRate(bandwidth_kbps * 1000 / 8, now);
+  encoder_bitrate_.SetBandwidthEstimate(bandwidth_kbps, now);
   ScheduleNextFrame(now);
 }
 
@@ -116,12 +163,8 @@ bool WebrtcFrameSchedulerSimple::GetEncoderFrameParams(
     return false;
   }
 
-  // TODO(sergeyu): This logic is applicable only to VP8. Reconsider it for VP9.
-  int minimum_bitrate =
-      static_cast<int64_t>(kVp8MinimumTargetBitrateKbpsPerMegapixel) *
-      frame.size().width() * frame.size().height() / 1000000LL;
   params_out->bitrate_kbps =
-      std::max(minimum_bitrate, pacing_bucket_.rate() * 8 / 1000);
+      encoder_bitrate_.GetTargetBitrateKbps(frame.size(), now);
 
   params_out->duration = kTargetFrameInterval;
   params_out->key_frame = key_frame_request_;