Move usage of QualityScaler to ViEEncoder.

webrtc/video/vie_encoder.h

 /*
  *  Copyright (c) 2012 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_VIDEO_VIE_ENCODER_H_
 #define WEBRTC_VIDEO_VIE_ENCODER_H_
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/sequenced_task_checker.h"
 #include "webrtc/base/task_queue.h"
 #include "webrtc/call.h"
 #include "webrtc/common_types.h"
 #include "webrtc/common_video/rotation.h"
 #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"
 #include "webrtc/video/overuse_frame_detector.h"
 #include "webrtc/video_encoder.h"
 #include "webrtc/video_send_stream.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 
 class ProcessThread;
 class SendStatisticsProxy;
 
 // VieEncoder represent a video encoder that accepts raw video frames as input
 // and produces an encoded bit stream.
 // Usage:
 //  Instantiate.
 //  Call SetSink.
 //  Call SetSource.
 //  Call ConfigureEncoder with the codec settings.
 //  Call Stop() when done.
 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.
   class EncoderSink : public EncodedImageCallback {
    public:
     virtual void OnEncoderConfigurationChanged(
         std::vector<VideoStream> streams,
         int min_transmit_bitrate_bps) = 0;
   };
 
@@ skipping 39 matching lines @@
   // virtual to test EncoderStateFeedback with mocks.
   virtual void OnReceivedIntraFrameRequest(size_t stream_index);
   virtual void OnReceivedSLI(uint8_t picture_id);
   virtual void OnReceivedRPSI(uint64_t picture_id);
 
   void OnBitrateUpdated(uint32_t bitrate_bps,
                         uint8_t fraction_lost,
                         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;
   class EncodeTask;
   class VideoSourceProxy;
 
   struct VideoFrameInfo {
     VideoFrameInfo(int width,
                    int height,
                    VideoRotation rotation,
@@ skipping 35 matching lines @@
   rtc::Event shutdown_event_;
 
   const uint32_t number_of_cores_;
 
   const std::unique_ptr<VideoSourceProxy> source_proxy_;
   EncoderSink* sink_;
   const VideoSendStream::Config::EncoderSettings settings_;
   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_;
   ProcessThread* module_process_thread_;
   rtc::ThreadChecker module_process_thread_checker_;
   // |thread_checker_| checks that public methods that are related to lifetime
   // of ViEEncoder are called on the same thread.
   rtc::ThreadChecker thread_checker_;
 
   VideoEncoderConfig encoder_config_ ACCESS_ON(&encoder_queue_);
@@ skipping 11 matching lines @@
   size_t max_data_payload_length_ ACCESS_ON(&encoder_queue_);
   uint32_t last_observed_bitrate_bps_ ACCESS_ON(&encoder_queue_);
   bool encoder_paused_and_dropped_frame_ ACCESS_ON(&encoder_queue_);
   bool has_received_sli_ ACCESS_ON(&encoder_queue_);
   uint8_t picture_id_sli_ ACCESS_ON(&encoder_queue_);
   bool has_received_rpsi_ ACCESS_ON(&encoder_queue_);
   uint64_t picture_id_rpsi_ ACCESS_ON(&encoder_queue_);
   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_);
   // Resolution has been requested to be less than |max_pixel_count_|.
   rtc::Optional<int> max_pixel_count_;  // ACCESS_ON(&encoder_queue_);
   // Resolution has been requested to higher than |max_pixel_count_step_up_|.
   rtc::Optional<int> max_pixel_count_step_up_;  // ACCESS_ON(&encoder_queue_);
 
   rtc::RaceChecker incoming_frame_race_checker_;
   Atomic32 posted_frames_waiting_for_encode_;
   // Used to make sure incoming time stamp is increasing for every frame.
   int64_t last_captured_timestamp_ GUARDED_BY(incoming_frame_race_checker_);
   // Delta used for translating between NTP and internal timestamps.
   const int64_t delta_ntp_internal_ms_;
 
   int64_t last_frame_log_ms_ GUARDED_BY(incoming_frame_race_checker_);
   int captured_frame_count_ ACCESS_ON(&encoder_queue_);
   int dropped_frame_count_ ACCESS_ON(&encoder_queue_);
 
   // All public methods are proxied to |encoder_queue_|. It must must be
   // destroyed first to make sure no tasks are run that use other members.
   rtc::TaskQueue encoder_queue_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(ViEEncoder);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_VIDEO_VIE_ENCODER_H_