Move usage of QualityScaler to ViEEncoder.

webrtc/modules/video_coding/utility/quality_scaler.h

 /*
  *  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.
  */
 
 #ifndef WEBRTC_MODULES_VIDEO_CODING_UTILITY_QUALITY_SCALER_H_
 #define WEBRTC_MODULES_VIDEO_CODING_UTILITY_QUALITY_SCALER_H_
 
+#include <utility>
+
 #include "webrtc/common_types.h"
+#include "webrtc/video_frame.h"

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

Remove this unused include as well as the i420_buffer_pool.h include.

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

Done.

+#include "webrtc/base/optional.h"
+#include "webrtc/base/sequenced_task_checker.h"
 #include "webrtc/common_video/include/i420_buffer_pool.h"
 #include "webrtc/modules/video_coding/utility/moving_average.h"
 
 namespace webrtc {
+
+// An interface for a class that receives scale up/down requests.
+class ScalingObserverInterface {
+ public:
+  enum ScaleReason { kQuality, kCpu };

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

prefer enum class

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

I get that strongly-typed enums have several benefits, however I'm using this
enum as an array index and thus the implicit integer conversion is actually
desired behaviour.
I can change that of course, if you think it's unsafe.

[sprang_webrtc, 2016/10/31 10:24:31]

I'd still go with an enum class, and be more explicit. Something like:
enum class ScaleReason : size_t { kQuality = 0, kCpu = 1 };
If you really want to use them as index then just static_cast<size_t>()

[sprang_webrtc, 2016/11/14 10:25:38]

No?

+  static const size_t ScaleReasonSize = 2;

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

nit: kScaleReasonSize or SCALE_REASON_SIZE

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

Done.

+  // Called to signal that we can handle larger frames.
+  virtual void ScaleUp(ScaleReason reason) = 0;
+  // Called to signal that encoder to scale down.
+  virtual void ScaleDown(ScaleReason reason) = 0;
+
+ protected:
+  virtual ~ScalingObserverInterface() {}
+};
+
+// QualityScaler runs asynchronously and monitors QP values of encoded frames.
+// It holds a reference to a ScalingObserverInterface implementation to signal
+// an intent to scale up or down.
 class QualityScaler {
  public:
-  struct Resolution {
-    int width;
-    int height;
+  typedef std::pair<int, int> QPThresholds;

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

I'd prefer a simple struct, just because threshold.high is more explicit than
threshold.second.

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

Done.

+  struct Settings {
+    Settings(bool on, int low, int high)
+        : enabled(on),
+          thresholds(rtc::Optional<QPThresholds>(QPThresholds(low, high))) {}
+    explicit Settings(bool on) : enabled(on) {}
+    const bool enabled;
+    const rtc::Optional<QPThresholds> thresholds;
   };
 
-  QualityScaler();
-  void Init(VideoCodecType codec_type,
-            int initial_bitrate_kbps,
-            int width,
-            int height,
-            int fps);
-  void Init(int low_qp_threshold,
-            int high_qp_threshold,
-            int initial_bitrate_kbps,
-            int width,
-            int height,
-            int fps);
-  void ReportFramerate(int framerate);
+  // Construct a QualityScaler with a given |observer|
+  explicit QualityScaler(ScalingObserverInterface* observer);
+  virtual ~QualityScaler();
+  // Init starts the quality scaler periodically checking what the average QP
+  // has been recently.
+  void Init(VideoCodecType codec_type);
+  // If specific thresholds are desired these can be supplied as |thresholds|.
+  void Init(QPThresholds thresholds);
+  // Stop must be called to stop the periodic task before QualityScaler is
+  // destroyed.
+  void Stop();
+  // Should be called each time the encoder drops a frame
+  void ReportDroppedFrame();
+  // Inform the QualityScaler of the last seen QP.
   void ReportQP(int qp);
-  void ReportDroppedFrame();
-  void OnEncodeFrame(int width, int height);
-  Resolution GetScaledResolution() const;
-  rtc::scoped_refptr<VideoFrameBuffer> GetScaledBuffer(
-      const rtc::scoped_refptr<VideoFrameBuffer>& frame);
-  int downscale_shift() const { return downscale_shift_; }
+
+  // This method declared virtual to help with testing.
+  virtual int64_t GetTimeoutMs();

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

nit: I don't think the name 'Timeout' is appropriate. Can you rename it
something like GetSamplingPeriodMs()?

As a sidenote, I think it would have been better to be able to pass in an
explicit TaskQueue to QualityScaler as an argument, so that we can pass in a
fake/mock TaskQueue when testing, so that the tests become completely
deterministic and we don't have to make virtual functions like this. That would
require TaskQueue to be an interface, which is not the case, so I think this is
an ok approach for now.

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

I agree that is a better approach. I didn't see a way to make it work though.
Changed the name of the method in line with your suggestion.

 
  private:
+  class CheckQPTask;
+  void CheckQP();
   void ClearSamples();
-  void ScaleUp();
-  void ScaleDown();
-  void UpdateTargetResolution(int width, int height);
+  void ReportQPLow();
+  void ReportQPHigh();
 
-  I420BufferPool pool_;
+  ScalingObserverInterface* observer_ GUARDED_BY(&task_checker_);

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

ScalingObserverInterface* const observer_

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

Done.

+  CheckQPTask* check_qp_task_ GUARDED_BY(&task_checker_);
+  rtc::SequencedTaskChecker task_checker_;
 
-  size_t num_samples_downscale_;
-  size_t num_samples_upscale_;
-  bool fast_rampup_;
-  MovingAverage average_qp_;
-  MovingAverage framedrop_percent_;
+  bool fast_rampup_ GUARDED_BY(&task_checker_);
+  int64_t measure_interval_ GUARDED_BY(&task_checker_);

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

nit: I would like to add a 'sec' suffix to the variable name to document the
time unit.

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

Done.

+  MovingAverage average_qp_ GUARDED_BY(&task_checker_);
+  MovingAverage framedrop_percent_ GUARDED_BY(&task_checker_);
 
-  int low_qp_threshold_;
-  int high_qp_threshold_;
-  Resolution target_res_;
+  int low_qp_threshold_ GUARDED_BY(&task_checker_);
+  int high_qp_threshold_ GUARDED_BY(&task_checker_);

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

Maybe a single QPThresholds member?

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

Done.

 
-  int downscale_shift_;
-  int maximum_shift_;
+  static const auto scale_reason_ = ScalingObserverInterface::kQuality;

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

Prefer not to use auto here.

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

Why? the type should be completely obvious from the rhs of the assignment. 

[sprang_webrtc, 2016/10/31 10:24:31]

If you change to an enum class, maybe. Otherwise some glancing at the code for
the first time will have to scroll up and read again to realize kQuality is a
part of ScaleReason.
I personally prefer using auto only for ridiculously long std container iterator
types and the like...

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

It's not allowed to use auto for non-local variables.

 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_VIDEO_CODING_UTILITY_QUALITY_SCALER_H_