Move usage of QualityScaler to ViEEncoder.

webrtc/video/vie_encoder.h

Index: webrtc/video/vie_encoder.h
diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h
index 6c77823a7b2285c13129ee5191221d084a7bb14b..4aeea89aabba07552ce1993a284a9522e5a69f30 100644
--- a/webrtc/video/vie_encoder.h
+++ b/webrtc/video/vie_encoder.h
@@ -25,6 +25,7 @@
 #include "webrtc/media/base/videosinkinterface.h"
 #include "webrtc/modules/video_coding/include/video_coding_defines.h"
 #include "webrtc/modules/video_coding/utility/simulcast_rate_allocator.h"
+#include "webrtc/modules/video_coding/utility/quality_scaler.h"
 #include "webrtc/modules/video_coding/video_coding_impl.h"
 #include "webrtc/modules/video_processing/include/video_processing.h"
 #include "webrtc/system_wrappers/include/atomic32.h"
@@ -49,7 +50,7 @@ class SendStatisticsProxy;
 class ViEEncoder : public rtc::VideoSinkInterface<VideoFrame>,
                    public EncodedImageCallback,
                    public VCMSendStatisticsCallback,
-                   public CpuOveruseObserver {
+                   public ScalingObserverInterface {
  public:
   // Interface for receiving encoded video frames and notifications about
   // configuration changes.
@@ -109,14 +110,14 @@ class ViEEncoder : public rtc::VideoSinkInterface<VideoFrame>,
                         int64_t round_trip_time_ms);
 
  protected:
-  // Used for testing. For example the |CpuOveruseObserver| methods must be
-  // called on |encoder_queue_|.
+  // Used for testing. For example the |ScalingObserverInterface| methods must
+  // be called on |encoder_queue_|.
   rtc::TaskQueue* encoder_queue() { return &encoder_queue_; }
 
-  // webrtc::CpuOveruseObserver implementation.
+  // webrtc::ScalingObserverInterface implementation.
   // These methods are protected for easier testing.
-  void OveruseDetected() override;
-  void NormalUsage() override;
+  void ScaleUp(ScaleReason reason) override;

[perkj_webrtc, 2016/10/26 15:53:50]

This seems racy to me. I think we want something like have something like
ScalyUp(ScaleReason reason, int pixel_count) so that we can now what the
decision is based on. 
ie- cpu say go down but quality say go up. Then you may only go up if both CPU
adapter and Quality scaler agree.  And to agree you will want to now what
resolution the callback is based on.

The logic for choosing what to do when there are different resolution wants
exist in VideoBroadcaster and can be copied from there. 

I am not convinced that a common interface for OveruseFrameDetector and the
quality scaler is needed. But ok. 

+  void ScaleDown(ScaleReason reason) override;
 
  private:
   class ConfigureEncoderTask;
@@ -172,8 +173,9 @@ class ViEEncoder : public rtc::VideoSinkInterface<VideoFrame>,
   const VideoCodecType codec_type_;
 
   vcm::VideoSender video_sender_ ACCESS_ON(&encoder_queue_);
-  bool disable_resolution_scaling_ ACCESS_ON(&encoder_queue_);
+  bool enable_quality_scaling_ ACCESS_ON(&encoder_queue_);

[magjed_webrtc, 2016/10/26 14:28:37]

I think you misunderstood me about the RAII change. The changes I wanted from
patch set 10 was this:
* Make QualityScaler::Init be the ctor, i.e. QualityScaler::QualityScaler.
* Make QualityScaler::Stop be the dtor, i.e .QualityScaler::~QualityScaler.
* Remove disable_quality_scaling_ and replace:
  void ViEEncoder::Stop() {
  - if (!disable_quality_scaling_)
  -   quality_scaler_.Stop();
  + quality_scaler_ = nullptr;
  and
  EncodedImageCallback::Result ViEEncoder::OnEncodedImage() {
  - if (!disable_quality_scaling_)
  -   quality_scaler_.ReportQP(encoded_image.qp_);
  +  if (quality_scaler_)
  +    quality_scaler_->ReportQP(encoded_image.qp_);
* Replace
  - quality_scaler_.Init(...);
  + quality_scaler_.reset(new QualityScaler(...));

[kthelgason, 2016/10/26 19:02:50]

So the issue with doing what you describe comes down to task queue usage. In
order for this to work without needing to guard access to members with a lock
all methods must be called sequentially. If the dtor is responsible for
deinitialisation we can't make that guarantee.

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

I don't follow, for me it looks perfectly fine to remove QualityScaler::Stop()
and move that code to QualityScaler::~QualityScaler() instead. All methods will
still be called sequentially on the task queue, including the dtor. Can you
elaborate why this would be a problem?

   OveruseFrameDetector overuse_detector_ ACCESS_ON(&encoder_queue_);
+  std::unique_ptr<QualityScaler> quality_scaler_ ACCESS_ON(&encoder_queue_);
 
   SendStatisticsProxy* const stats_proxy_;
   rtc::VideoSinkInterface<VideoFrame>* const pre_encode_callback_;
@@ -205,7 +207,7 @@ class ViEEncoder : public rtc::VideoSinkInterface<VideoFrame>,
   Clock* const clock_;
   // Counter used for deciding if the video resolution is currently
   // restricted by CPU usage. It is reset if |source_| is changed.
-  int cpu_restricted_counter_;  // ACCESS_ON(&encoder_queue_);
+  int scale_counter_[ScaleReasonSize] = {0};  // ACCESS_ON(&encoder_queue_);
 
   int last_frame_width_;   //  ACCESS_ON(&encoder_queue_);
   int last_frame_height_;  //  ACCESS_ON(&encoder_queue_);