Refactor QualityScaler and MovingAverage

webrtc/modules/video_coding/utility/quality_scaler.cc

Index: webrtc/modules/video_coding/utility/quality_scaler.cc
diff --git a/webrtc/modules/video_coding/utility/quality_scaler.cc b/webrtc/modules/video_coding/utility/quality_scaler.cc
index c509e843642f2237e94fe1727749479a9523faa7..d008187e981d8b4c5dc0be48674f18c22eb60450 100644
--- a/webrtc/modules/video_coding/utility/quality_scaler.cc
+++ b/webrtc/modules/video_coding/utility/quality_scaler.cc
@@ -10,10 +10,12 @@
 
 #include "webrtc/modules/video_coding/utility/quality_scaler.h"
 
+#include <algorithm>
+#include <cmath>
+
 namespace webrtc {
 
 namespace {
-static const int kMinFps = 5;
 // Threshold constant used until first downscale (to permit fast rampup).
 static const int kMeasureSecondsFastUpscale = 2;
 static const int kMeasureSecondsUpscale = 5;
@@ -46,7 +48,11 @@ const int QualityScaler::kLowH264QpThreshold = 24;
 const int QualityScaler::kBadH264QpThreshold = 37;
 #endif
 
-QualityScaler::QualityScaler() : low_qp_threshold_(-1) {}
+// Default values. Should immediately get set to something more sensible.
+QualityScaler::QualityScaler()
+    : average_qp_(kMeasureSecondsUpscale * 30),
+      framedrop_percent_(kMeasureSecondsUpscale * 30),
+      low_qp_threshold_(-1) {}
 
 void QualityScaler::Init(int low_qp_threshold,
                          int high_qp_threshold,
@@ -54,14 +60,15 @@ void QualityScaler::Init(int low_qp_threshold,
                          int width,
                          int height,
                          int fps) {
-  ClearSamples();
   low_qp_threshold_ = low_qp_threshold;
   high_qp_threshold_ = high_qp_threshold;
   downscale_shift_ = 0;
-  // Use a faster window for upscaling initially (but be more graceful later).
-  // This enables faster initial rampups without risking strong up-down
-  // behavior later.
-  measure_seconds_upscale_ = kMeasureSecondsFastUpscale;
+
+  fast_rampup_ = true;
+
+  ClearSamples();

[sakal, 2016/09/07 09:47:21]

This is not necessary since ReportFramerate will clear the samples anyway,
right?

+  ReportFramerate(fps);
+
   const int init_width = width;
   const int init_height = height;
   if (initial_bitrate_kbps > 0) {
@@ -76,61 +83,98 @@ void QualityScaler::Init(int low_qp_threshold,
       height /= 2;
     }
   }
-
-  // Zero out width/height so they can be checked against inside
-  // UpdateTargetResolution.
-  res_.width = res_.height = 0;
   UpdateTargetResolution(init_width, init_height);
   ReportFramerate(fps);
 }
 
 // Report framerate(fps) to estimate # of samples.
 void QualityScaler::ReportFramerate(int framerate) {
-  framerate_ = framerate;
-  UpdateSampleCounts();
+  // Use a faster window for upscaling initially.
+  // This enables faster initial rampups without risking strong up-down
+  // behavior later.
+  if (fast_rampup_)
+    num_samples_upscale_ = framerate * kMeasureSecondsFastUpscale;

[magjed_webrtc, 2016/09/07 11:55:16]

nit: I would like to write this as a single assignment of num_samples_upscale_
using ternary to select FastUpscale vs Upscale.

+  else
+    num_samples_upscale_ = framerate * kMeasureSecondsUpscale;
+  num_samples_downscale_ = framerate * kMeasureSecondsDownscale;
+
+  average_qp_ =
+      MovingAverage(std::max(num_samples_upscale_, num_samples_downscale_));
+  framedrop_percent_ =
+      MovingAverage(std::max(num_samples_upscale_, num_samples_downscale_));
 }
 
 void QualityScaler::ReportQP(int qp) {
+  frames_seen_++;
   framedrop_percent_.AddSample(0);
-  average_qp_downscale_.AddSample(qp);
-  average_qp_upscale_.AddSample(qp);
+  average_qp_.AddSample(qp);
 }
 
 void QualityScaler::ReportDroppedFrame() {
+  frames_seen_++;
   framedrop_percent_.AddSample(100);
 }
 
 void QualityScaler::OnEncodeFrame(int width, int height) {
   // Should be set through InitEncode -> Should be set by now.
   RTC_DCHECK_GE(low_qp_threshold_, 0);
-  RTC_DCHECK_GT(num_samples_upscale_, 0u);
-  RTC_DCHECK_GT(num_samples_downscale_, 0u);
-
-  // Update scale factor.
-  int avg_drop = 0;
-  int avg_qp = 0;
-
-  if ((framedrop_percent_.GetAverage(num_samples_downscale_, &avg_drop) &&
-       avg_drop >= kFramedropPercentThreshold) ||
-      (average_qp_downscale_.GetAverage(num_samples_downscale_, &avg_qp) &&
-       avg_qp > high_qp_threshold_)) {
-    AdjustScale(false);
-  } else if (average_qp_upscale_.GetAverage(num_samples_upscale_, &avg_qp) &&
-             avg_qp <= low_qp_threshold_) {
-    AdjustScale(true);
+  if (target_res_.width != width || target_res_.height != height) {
+    UpdateTargetResolution(width, height);
+  }
+
+  // Check if we should scale down due to high frame drop.
+  if (frames_seen_ >= num_samples_downscale_) {

[magjed_webrtc, 2016/09/07 11:55:16]

Do we really need this check now that we have rtc::Optional as a return value of
GetAverage? I expected this code to be:
const rtc::Optional<int> drop_rate =
    framedrop_percent_.GetAverage(num_samples_downscale_);
const rtc::Optional<int> avg_qp_down =
    average_qp_.GetAverage(num_samples_downscale_);
if ((drop_rate && *drop_rate >= kFramedropPercentThreshold) ||
    (avg_qp_down && *avg_qp_down > high_qp_threshold_)) {
  ScaleDown();
  return;
}

[kthelgason, 2016/09/07 12:31:46]

Yeah, you're right that it's not necessary. I thought it might be better to be
explicit about when we try to scale up,  but looking at it now it's probably
clear just from the usage of MovingAverage here. Definitely not worth the added
code anyway. 

+    const auto drop_rate =
+        framedrop_percent_.GetAverage(num_samples_downscale_);
+    if (drop_rate && *drop_rate >= kFramedropPercentThreshold) {
+      ScaleDown();
+      return;
+    }
+
+    // Check if we should scale up or down based on QP.

[sakal, 2016/09/07 09:47:21]

We never scale up here.

[kthelgason, 2016/09/07 12:31:46]

The comment applies to the following two branches, at least that's how it was
intended. If it's not clear I will fix it.

+    const auto avg_qp_down = average_qp_.GetAverage(num_samples_downscale_);
+    if (avg_qp_down && *avg_qp_down > high_qp_threshold_) {
+      ScaleDown();
+      return;
+    }
+  }
+  if (frames_seen_ >= num_samples_upscale_) {

[magjed_webrtc, 2016/09/07 11:55:16]

ditto: the if-statement seems redundant.

+    const auto avg_qp_up = average_qp_.GetAverage(num_samples_upscale_);
+    if (avg_qp_up && *avg_qp_up <= low_qp_threshold_) {
+      // QP has been low. We want to try a higher resolution.
+      ScaleUp();
+      return;
+    }
+  }
+}
+
+void QualityScaler::ScaleUp() {
+  downscale_shift_ = std::max(0, downscale_shift_ - 1);
+  ClearSamples();
+}
+
+void QualityScaler::ScaleDown() {
+  downscale_shift_ = std::min(maximum_shift_, downscale_shift_ + 1);
+  ClearSamples();
+  // If we've scaled down, wait longer before scaling up again.
+  if (fast_rampup_) {
+    fast_rampup_ = false;
+    num_samples_upscale_ = (num_samples_upscale_ / kMeasureSecondsFastUpscale) *
+                           kMeasureSecondsUpscale;
   }
-  UpdateTargetResolution(width, height);
 }
 
 QualityScaler::Resolution QualityScaler::GetScaledResolution() const {
-  return res_;
+  const int frame_width = target_res_.width >> downscale_shift_;
+  const int frame_height = target_res_.height >> downscale_shift_;
+  return Resolution{frame_width, frame_height};
 }
 
 rtc::scoped_refptr<VideoFrameBuffer> QualityScaler::GetScaledBuffer(
     const rtc::scoped_refptr<VideoFrameBuffer>& frame) {
   Resolution res = GetScaledResolution();
-  int src_width = frame->width();
-  int src_height = frame->height();
+  const int src_width = frame->width();
+  const int src_height = frame->height();
 
   if (res.width == src_width && res.height == src_height)
     return frame;
@@ -142,50 +186,21 @@ rtc::scoped_refptr<VideoFrameBuffer> QualityScaler::GetScaledBuffer(
   return scaled_buffer;
 }
 
-void QualityScaler::UpdateTargetResolution(int frame_width, int frame_height) {
-  RTC_DCHECK_GE(downscale_shift_, 0);
-  int shifts_performed = 0;
-  for (int shift = downscale_shift_;
-       shift > 0 && (frame_width / 2 >= kMinDownscaleDimension) &&
-       (frame_height / 2 >= kMinDownscaleDimension);
-       --shift, ++shifts_performed) {
-    frame_width /= 2;
-    frame_height /= 2;
+void QualityScaler::UpdateTargetResolution(int width, int height) {
+  if (width < kMinDownscaleDimension || height < kMinDownscaleDimension) {
+    maximum_shift_ = 0;
+  } else {
+    maximum_shift_ = static_cast<int>(
+        std::log2(std::min(width, height) / kMinDownscaleDimension));
   }
-  // Clamp to number of shifts actually performed to not be stuck trying to
-  // scale way beyond QVGA.
-  downscale_shift_ = shifts_performed;
-  if (res_.width == frame_width && res_.height == frame_height) {
-    // No reset done/needed, using same resolution.
-    return;
-  }
-  res_.width = frame_width;
-  res_.height = frame_height;
-  ClearSamples();
+  target_res_ = Resolution{width, height};
 }
 
 void QualityScaler::ClearSamples() {
   framedrop_percent_.Reset();
-  average_qp_downscale_.Reset();
-  average_qp_upscale_.Reset();
+  average_qp_.Reset();
+  frames_seen_ = 0;
 }
 
-void QualityScaler::UpdateSampleCounts() {
-  num_samples_downscale_ = static_cast<size_t>(
-      kMeasureSecondsDownscale * (framerate_ < kMinFps ? kMinFps : framerate_));
-  num_samples_upscale_ = static_cast<size_t>(
-      measure_seconds_upscale_ * (framerate_ < kMinFps ? kMinFps : framerate_));
-}
-
-void QualityScaler::AdjustScale(bool up) {
-  downscale_shift_ += up ? -1 : 1;
-  if (downscale_shift_ < 0)
-    downscale_shift_ = 0;
-  if (!up) {
-    // First downscale hit, start using a slower threshold for going up.
-    measure_seconds_upscale_ = kMeasureSecondsUpscale;
-    UpdateSampleCounts();
-  }
-}
 
 }  // namespace webrtc