Move usage of QualityScaler to ViEEncoder.

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 55cb274209565687ccf146477f6ff8928a85c6fd..e8f81deea112ddcca1dca952d28606a8708943d4 100644
--- a/webrtc/modules/video_coding/utility/quality_scaler.cc
+++ b/webrtc/modules/video_coding/utility/quality_scaler.cc
@@ -13,8 +13,11 @@
 #include <math.h>
 
 #include <algorithm>
+#include <memory>
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/base/task_queue.h"
 
 // TODO(kthelgason): Some versions of Android have issues with log2.
 // See https://code.google.com/p/android/issues/detail?id=212634 for details
@@ -27,20 +30,8 @@ namespace webrtc {
 namespace {
 // 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 kMeasureSeconds = 5;

[sprang_webrtc, 2016/10/27 13:43:48]

I think this is the first time I've seen seconds used as a unit in out code. I'm
afraid of change, so would like this to be in ms too :)

[kthelgason, 2016/10/28 13:20:15]

That sounds good to me. This is in seconds because the previous code had it
specified in seconds, but I can of course change it.

 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
-// mechanism.
-static const int kVgaBitrateThresholdKbps = 500;
-static const int kVgaNumPixels = 700 * 500;  // 640x480
-static const int kQvgaBitrateThresholdKbps = 250;
-static const int kQvgaNumPixels = 400 * 300;  // 320x240
-
 // QP scaling threshold defaults:
 static const int kLowH264QpThreshold = 24;
 static const int kHighH264QpThreshold = 37;
@@ -50,18 +41,58 @@ static const int kLowVp8QpThreshold = 29;
 static const int kHighVp8QpThreshold = 95;
 }  // namespace
 
-// 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(VideoCodecType codec_type,
-                         int initial_bitrate_kbps,
-                         int width,
-                         int height,
-                         int fps) {
-  int low = -1, high = -1;
+class QualityScaler::CheckQPTask : public rtc::QueuedTask {
+ public:
+  explicit CheckQPTask(QualityScaler* scaler) : scaler_(scaler) {
+    LOG(LS_INFO) << "Created CheckQPTask. Scheduling on queue...";
+    const auto q = rtc::TaskQueue::Current();

[magjed_webrtc, 2016/10/27 11:45:57]

nit: I would not use auto here and the style guide says: "Use auto to avoid type
names that are noisy, obvious, or unimportant - cases where the type doesn't aid
in clarity for the reader." and at least for me the return type of
rtc::TaskQueue::Current() is not obvious or unimportant, and the type
rtc::TaskQueue* is not noisy. For example, I was concerned if you were making an
unintentional copy-by-value of something expensive since you are not capturing
by-ref, so I had to look the specific type up in task_queue.h. Let's keep the
use of auto to the specific cases where it's allowed or encouraged according to
the style guide. For this case, you can also just inline q, i.e. write
rtc::TaskQueue::Current()->PostDelayedTask(...).

[kthelgason, 2016/10/28 13:20:15]

Done.

[magjed_webrtc, 2016/11/10 13:06:59]

It's not done (in the latest patch set at least).

+    q->PostDelayedTask(std::unique_ptr<rtc::QueuedTask>(this),
+                       scaler_->GetTimeoutMs());
+  }
+
+ private:
+  friend class QualityScaler;

[magjed_webrtc, 2016/10/27 11:45:57]

Why is QualityScaler a friend? Can't you just make Stop() public instead?

[kthelgason, 2016/10/28 13:20:15]

I think it's cleaner to enforce the fact that this method should only ever be
called by QualityScaler this way. Maybe that's stupid since the class is not
declared in a header, but that is my reasoning at least :)

[magjed_webrtc, 2016/10/28 15:56:00]

But the class is unusable without access to the Stop function and it's
impossible to kill it. I don't feel strongly about one way over the other so do
what you see fit.

+  void Stop() {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+    stop_ = true;
+  }

[sprang_webrtc, 2016/10/27 13:43:48]

nit: \n

[kthelgason, 2016/10/28 13:20:15]

Done.

+  bool Run() override {
+    RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+    if (stop_)
+      return true;  // TaskQueue will free this task.
+    scaler_->CheckQP();
+    rtc::TaskQueue::Current()->PostDelayedTask(
+        std::unique_ptr<rtc::QueuedTask>(this), scaler_->GetTimeoutMs());
+    return false;  // Retain the task in order to reuse it.
+  }
+
+  QualityScaler* const scaler_;
+  bool stop_ = false;
+  rtc::SequencedTaskChecker task_checker_;
+};
+
+QualityScaler::QualityScaler(ScalingObserverInterface* observer)
+    : observer_(observer),
+      check_qp_task_(nullptr),
+      fast_rampup_(true),
+      measure_interval_(kMeasureSecondsFastUpscale),
+      average_qp_(kMeasureSeconds * 30),
+      framedrop_percent_(kMeasureSeconds * 30) {
+  // Use a faster window for upscaling initially.
+  // This enables faster initial rampups without risking strong up-down
+  // behavior later.
+
+  RTC_DCHECK(observer_ != nullptr);
+  task_checker_.Detach();
+}
+
+QualityScaler::~QualityScaler() {
+  RTC_DCHECK(check_qp_task_ == nullptr) << "Must call Stop.";
+}
+
+void QualityScaler::Init(VideoCodecType codec_type) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  int low, high;

[magjed_webrtc, 2016/10/27 11:45:57]

nit: You should only have one declaration per line.

[kthelgason, 2016/10/28 13:20:15]

Done.

   switch (codec_type) {
     case kVideoCodecH264:
       low = kLowH264QpThreshold;
@@ -74,141 +105,87 @@ void QualityScaler::Init(VideoCodecType codec_type,
     default:
       RTC_NOTREACHED() << "Invalid codec type for QualityScaler.";
   }
-  Init(low, high, initial_bitrate_kbps, width, height, fps);
+  Init(QPThresholds(low, high));
 }
 
-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;
-  fast_rampup_ = true;
-  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;
-    }
-  }
-  UpdateTargetResolution(init_width, init_height);
-  ReportFramerate(fps);
+void QualityScaler::Init(QPThresholds thresholds) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  low_qp_threshold_ = thresholds.first;
+  high_qp_threshold_ = thresholds.second;
+  if (check_qp_task_ == nullptr)
+    check_qp_task_ = new CheckQPTask(this);
 }
 
-// Report framerate(fps) to estimate # of samples.
-void QualityScaler::ReportFramerate(int framerate) {
-  // Use a faster window for upscaling initially.
-  // This enables faster initial rampups without risking strong up-down
-  // behavior later.
-  num_samples_upscale_ = framerate * (fast_rampup_ ? kMeasureSecondsFastUpscale
-                                                   : 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::Stop() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  if (check_qp_task_ != nullptr) {
+    check_qp_task_->Stop();
+    check_qp_task_ = nullptr;
+  }
 }
 
-void QualityScaler::ReportQP(int qp) {
-  framedrop_percent_.AddSample(0);
-  average_qp_.AddSample(qp);
+int64_t QualityScaler::GetTimeoutMs() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  return measure_interval_ * 1000;
 }
 
 void QualityScaler::ReportDroppedFrame() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
   framedrop_percent_.AddSample(100);
 }
 
-void QualityScaler::OnEncodeFrame(int width, int height) {
+void QualityScaler::ReportQP(int qp) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  framedrop_percent_.AddSample(0);
+  average_qp_.AddSample(qp);
+}
+
+void QualityScaler::CheckQP() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
   // Should be set through InitEncode -> Should be set by now.
   RTC_DCHECK_GE(low_qp_threshold_, 0);
-  if (target_res_.width != width || target_res_.height != height) {
-    UpdateTargetResolution(width, height);
-  }
-
+  LOG(LS_INFO) << "Checking if average QP exceeds threshold";
   // Check if we should scale down due to high frame drop.
-  const auto drop_rate = framedrop_percent_.GetAverage(num_samples_downscale_);
+  const auto drop_rate = framedrop_percent_.GetAverage();
   if (drop_rate && *drop_rate >= kFramedropPercentThreshold) {
-    ScaleDown();
+    ReportQPHigh();
     return;
   }
 
   // Check if we should scale up or down based on QP.
-  const auto avg_qp_down = average_qp_.GetAverage(num_samples_downscale_);
-  if (avg_qp_down && *avg_qp_down > high_qp_threshold_) {
-    ScaleDown();
+  const auto avg_qp = average_qp_.GetAverage();

[sprang_webrtc, 2016/10/27 13:43:48]

Would prefer the actual type here.

[kthelgason, 2016/10/28 13:20:15]

I'm not sure I agree. The type is rtc::Optional<int>, and typing that out does
not add much clarity, at least for me. But my preference is always towards more
auto, not less. Feel free to reiterate this comment if you feel strongly about
it.

[magjed_webrtc, 2016/10/28 15:56:00]

+1. rtc::Optional<int> adds clarity for me. It's not possible to know the type
from the local context and it's necessary to look up the declaration of
GetAverage. The type declarations add more clarity for me than comments. Please
only use auto in these cases according to the style guide:
* (Encouraged) For iterators and other long/cluttery type names, particularly
when the type is clear from context (calls to find, begin, or end for instance).
* (Allowed) When the type is clear from local context (in the same expression or
within a few lines). Initialization of a pointer or smart pointer with calls to
new commonly falls into this category, as does use of auto in a range-based loop
over a container whose type is spelled out nearby.
* (Allowed) When the type doesn't matter because it isn't being used for
anything other than equality comparison.
* (Encouraged) When iterating over a map with a range-based loop (because it is
often assumed that the correct type is pair<KeyType,ValueType> whereas it is
actually pair<const KeyType,ValueType>). This is particularly well paired with
local key and value aliases for .first and .second (often const-ref).

+  if (avg_qp && *avg_qp > high_qp_threshold_) {
+    ReportQPHigh();
     return;
   }
-  const auto avg_qp_up = average_qp_.GetAverage(num_samples_upscale_);
-  if (avg_qp_up && *avg_qp_up <= low_qp_threshold_) {
+  if (avg_qp && *avg_qp <= low_qp_threshold_) {
     // QP has been low. We want to try a higher resolution.
-    ScaleUp();
+    ReportQPLow();
     return;
   }
 }
 
-void QualityScaler::ScaleUp() {
-  downscale_shift_ = std::max(0, downscale_shift_ - 1);
+void QualityScaler::ReportQPLow() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  LOG(LS_INFO) << "QP has been low, asking for higher resolution.";
   ClearSamples();
+  observer_->ScaleUp(scale_reason_);
 }
 
-void QualityScaler::ScaleDown() {
-  downscale_shift_ = std::min(maximum_shift_, downscale_shift_ + 1);
+void QualityScaler::ReportQPHigh() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
+  LOG(LS_INFO) << "QP has been high , asking for lower resolution.";
   ClearSamples();
+  observer_->ScaleDown(scale_reason_);
   // 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 {
-  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();
-  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 width, int height) {
-  if (width < kMinDownscaleDimension || height < kMinDownscaleDimension) {
-    maximum_shift_ = 0;
-  } else {
-    maximum_shift_ = static_cast<int>(
-        log2(std::min(width, height) / kMinDownscaleDimension));
+    measure_interval_ = kMeasureSeconds;
   }
-  target_res_ = Resolution{width, height};
 }
 
 void QualityScaler::ClearSamples() {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
   framedrop_percent_.Reset();
   average_qp_.Reset();
 }