Refactor QualityScaler and MovingAverage

webrtc/modules/video_coding/utility/quality_scaler.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #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;
 static const int kMeasureSecondsDownscale = 5;
 static const int kFramedropPercentThreshold = 60;
 // Min width/height to downscale to, set to not go below QVGA, but with some
 // margin to permit "almost-QVGA" resolutions, such as QCIF.
 static const int kMinDownscaleDimension = 140;
 // Initial resolutions corresponding to a bitrate. Aa bit above their actual
 // values to permit near-VGA and near-QVGA resolutions to use the same
@@ skipping 12 matching lines @@
 const int QualityScaler::kBadVp8QpThreshold = 95;
 
 #if defined(WEBRTC_IOS)
 const int QualityScaler::kLowH264QpThreshold = 32;
 const int QualityScaler::kBadH264QpThreshold = 42;
 #else
 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,
                          int initial_bitrate_kbps,
                          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) {
     int init_num_pixels = width * height;
     if (initial_bitrate_kbps < kVgaBitrateThresholdKbps)
       init_num_pixels = kVgaNumPixels;
     if (initial_bitrate_kbps < kQvgaBitrateThresholdKbps)
       init_num_pixels = kQvgaNumPixels;
     while (width * height > init_num_pixels) {
       ++downscale_shift_;
       width /= 2;
       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);
+  if (target_res_.width != width || target_res_.height != height) {
+    UpdateTargetResolution(width, height);
+  }
 
-  // Update scale factor.
-  int avg_drop = 0;
-  int avg_qp = 0;
+  // 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;
+    }
 
-  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);
+    // 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;
+    }
   }
-  UpdateTargetResolution(width, height);
+  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;
+  }
 }
 
 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;
   rtc::scoped_refptr<I420Buffer> scaled_buffer =
       pool_.CreateBuffer(res.width, res.height);
 
   scaled_buffer->ScaleFrom(frame);
 
   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