Let ViEEncoder express resolution requests as Sinkwants

webrtc/video/vie_encoder.cc

Index: webrtc/video/vie_encoder.cc
diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc
index 95a6f5a18c9f54a8139292fcfd05167cfdc8af3c..671c24032f7ac63e267ecc28d1d7f42d5009ed76 100644
--- a/webrtc/video/vie_encoder.cc
+++ b/webrtc/video/vie_encoder.cc
@@ -226,6 +226,9 @@ class ViEEncoder::EncodeTask : public rtc::QueuedTask {
   bool Run() override {
     RTC_DCHECK_RUN_ON(&vie_encoder_->encoder_queue_);
     RTC_DCHECK_GT(vie_encoder_->posted_frames_waiting_for_encode_.Value(), 0);
+    vie_encoder_->stats_proxy_->OnIncomingFrame(
+        frame_.width(), frame_.height(),
+        vie_encoder_->cpu_restricted_counter_ != 0);
     ++vie_encoder_->captured_frame_count_;
     if (--vie_encoder_->posted_frames_waiting_for_encode_ == 0) {
       vie_encoder_->EncodeVideoFrame(frame_, time_when_posted_ms_);
@@ -253,21 +256,29 @@ class ViEEncoder::EncodeTask : public rtc::QueuedTask {
 };
 
 // VideoSourceProxy is responsible ensuring thread safety between calls to
-// ViEEncoder::SetSource that will happen on libjingles worker thread when a
+// ViEEncoder::SetSource that will happen on libjingle's worker thread when a
 // video capturer is connected to the encoder and the encoder task queue
 // (encoder_queue_) where the encoder reports its VideoSinkWants.
 class ViEEncoder::VideoSourceProxy {
  public:
   explicit VideoSourceProxy(ViEEncoder* vie_encoder)
-      : vie_encoder_(vie_encoder), source_(nullptr) {}
+      : vie_encoder_(vie_encoder),
+        resolution_scaling_disabled_(false),
+        source_(nullptr) {}
 
-  void SetSource(rtc::VideoSourceInterface<VideoFrame>* source) {
+  void SetSource(rtc::VideoSourceInterface<VideoFrame>* source,
+                 bool disable_resolution_scaling) {

[åsapersson, 2016/10/24 07:54:06]

change to enable_resolution_scaling?

[perkj_webrtc, 2016/10/26 16:40:17]

now use preferences

+    // Called on libjingle's worker thread.
     RTC_DCHECK_CALLED_SEQUENTIALLY(&main_checker_);
     rtc::VideoSourceInterface<VideoFrame>* old_source = nullptr;
+    rtc::VideoSinkWants wants;
     {
       rtc::CritScope lock(&crit_);
       old_source = source_;
       source_ = source;
+      resolution_scaling_disabled_ = disable_resolution_scaling;
+      wants = disable_resolution_scaling ? disabled_scaling_sink_wants_
+                                         : sink_wants_;
     }
 
     if (old_source != source && old_source != nullptr) {
@@ -278,16 +289,63 @@ class ViEEncoder::VideoSourceProxy {
       return;
     }
 
-    // TODO(perkj): Let VideoSourceProxy implement LoadObserver and truly send
-    // CPU load as sink wants.
-    rtc::VideoSinkWants wants;
     source->AddOrUpdateSink(vie_encoder_, wants);
   }
 
+  void SetWantsRotationApplied(bool rotation_applied) {
+    rtc::CritScope lock(&crit_);
+    sink_wants_.rotation_applied = rotation_applied;
+    disabled_scaling_sink_wants_.rotation_applied = rotation_applied;
+    if (source_) {
+      source_->AddOrUpdateSink(vie_encoder_, resolution_scaling_disabled_
+                                                 ? disabled_scaling_sink_wants_
+                                                 : sink_wants_);
+    }
+  }
+
+  void RequestResolutionLowerThan(int pixel_count) {
+    // Called on the encoder task queue.
+    rtc::CritScope lock(&crit_);
+    if (resolution_scaling_disabled_) {
+      // This can happen since |resolution_scaling_disabled_| is set on
+      // libjingle's worker thread but the adaptation is done on the encoder
+      // task

[åsapersson, 2016/10/24 07:54:06]

move comment up one line

[perkj_webrtc, 2016/10/26 16:40:17]

Done.

+      // queue.
+      return;
+    }
+    // The input video frame size will have a resolution with less than or
+    // equal to |max_pixel_count| depending on how the source can scale the
+    // input frame size.
+    sink_wants_.max_pixel_count = rtc::Optional<int>((pixel_count * 3) / 5);
+    sink_wants_.max_pixel_count_step_up = rtc::Optional<int>();
+    if (source_)
+      source_->AddOrUpdateSink(vie_encoder_, sink_wants_);
+  }
+
+  void RequestHigherResolutionThan(int pixel_count) {
+    rtc::CritScope lock(&crit_);
+    if (resolution_scaling_disabled_) {
+      // This can happen since |resolution_scaling_disabled_| is set on
+      // libjingles worker thread but the adaptation is done on the encoder task
+      // queue.
+      return;
+    }
+    // The input video frame size will have a resolution with "one step up"
+    // pixels than |max_pixel_count_step_up| where "one step up" depends on
+    // how the source can scale the input frame size.
+    sink_wants_.max_pixel_count = rtc::Optional<int>();
+    sink_wants_.max_pixel_count_step_up = rtc::Optional<int>(pixel_count);
+    if (source_)
+      source_->AddOrUpdateSink(vie_encoder_, sink_wants_);
+  }
+
  private:
   rtc::CriticalSection crit_;
   rtc::SequencedTaskChecker main_checker_;
-  ViEEncoder* vie_encoder_;
+  ViEEncoder* const vie_encoder_;
+  rtc::VideoSinkWants sink_wants_ GUARDED_BY(&crit_);
+  rtc::VideoSinkWants disabled_scaling_sink_wants_ GUARDED_BY(&crit_);
+  bool resolution_scaling_disabled_ GUARDED_BY(&crit_);

[åsapersson, 2016/10/24 07:54:06]

change to resolution_scaling_enabled_?

[perkj_webrtc, 2016/10/26 16:40:17]

degradation_preference_

   rtc::VideoSourceInterface<VideoFrame>* source_ GUARDED_BY(&crit_);
 
   RTC_DISALLOW_COPY_AND_ASSIGN(VideoSourceProxy);
@@ -297,7 +355,6 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
                        SendStatisticsProxy* stats_proxy,
                        const VideoSendStream::Config::EncoderSettings& settings,
                        rtc::VideoSinkInterface<VideoFrame>* pre_encode_callback,
-                       LoadObserver* overuse_callback,
                        EncodedFrameObserver* encoder_timing)
     : shutdown_event_(true /* manual_reset */, false),
       number_of_cores_(number_of_cores),
@@ -306,14 +363,9 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       settings_(settings),
       codec_type_(PayloadNameToCodecType(settings.payload_name)),
       video_sender_(Clock::GetRealTimeClock(), this, this),
-      overuse_detector_(Clock::GetRealTimeClock(),
-                        GetCpuOveruseOptions(settings.full_overuse_time),
-                        this,
-                        encoder_timing,
-                        stats_proxy),
-      load_observer_(overuse_callback),
       stats_proxy_(stats_proxy),
       pre_encode_callback_(pre_encode_callback),
+      encoder_timing_(encoder_timing),
       module_process_thread_(nullptr),
       pending_encoder_reconfiguration_(false),
       encoder_start_bitrate_bps_(0),
@@ -325,6 +377,9 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       has_received_rpsi_(false),
       picture_id_rpsi_(0),
       clock_(Clock::GetRealTimeClock()),
+      cpu_restricted_counter_(0),
+      last_frame_width_(0),
+      last_frame_height_(0),
       last_captured_timestamp_(0),
       delta_ntp_internal_ms_(clock_->CurrentNtpInMilliseconds() -
                              clock_->TimeInMilliseconds()),
@@ -336,7 +391,6 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
     RTC_DCHECK_RUN_ON(&encoder_queue_);
     video_sender_.RegisterExternalEncoder(
         settings_.encoder, settings_.payload_type, settings_.internal_source);
-    overuse_detector_.StartCheckForOveruse();
   });
 }
 
@@ -348,12 +402,12 @@ ViEEncoder::~ViEEncoder() {
 
 void ViEEncoder::Stop() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  source_proxy_->SetSource(nullptr);
+  source_proxy_->SetSource(nullptr, true /* disable_resolution_scaling*/);
   encoder_queue_.PostTask([this] {
     RTC_DCHECK_RUN_ON(&encoder_queue_);
     video_sender_.RegisterExternalEncoder(nullptr, settings_.payload_type,
                                           false);
-    overuse_detector_.StopCheckForOveruse();
+    overuse_detector_.reset();
     shutdown_event_.Set();
   });
 
@@ -373,12 +427,27 @@ void ViEEncoder::DeRegisterProcessThread() {
   module_process_thread_->DeRegisterModule(&video_sender_);
 }
 
-void ViEEncoder::SetSource(rtc::VideoSourceInterface<VideoFrame>* source) {
+void ViEEncoder::SetSource(rtc::VideoSourceInterface<VideoFrame>* source,
+                           bool disable_resolution_scaling) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  source_proxy_->SetSource(source);
+  source_proxy_->SetSource(source, disable_resolution_scaling);
+  encoder_queue_.PostTask([this, disable_resolution_scaling] {
+    RTC_DCHECK_RUN_ON(&encoder_queue_);
+    if (disable_resolution_scaling) {
+      overuse_detector_.reset(nullptr);

[åsapersson, 2016/10/24 07:54:06]

Note that the encode time is reported to SendStatisticsProxy via
OveruseFrameDetector::EncodedFrameTimeMeasured. A change might be needed to
still get encode time stats when resolution scaling is disabled (e.g. for
screenshare).  

[perkj_webrtc, 2016/10/26 16:40:17]

Thanks for pointing this out. I did this change since Kari pointed out that we
dont need to call the overuse detector at all when when it is off.  But Erik
also mentioned that he want to change the overuseframedetector to also support
signaling that frame rate should be lowered.  So I will revert these changes and
always call the detector for now and only ignore the result. 

+    } else {
+      overuse_detector_.reset(new OveruseFrameDetector(
+          clock_, GetCpuOveruseOptions(settings_.full_overuse_time), this,
+          stats_proxy_));
+      overuse_detector_->StartCheckForOveruse();
+    }
+    stats_proxy_->SetCpuRestrictedResolution(!disable_resolution_scaling &&
+                                             cpu_restricted_counter_ != 0);

[åsapersson, 2016/10/24 07:54:06]

Is SetCpuRestrictedResolution needed? Looks like when cpu_restricted_counter_ is
updated it is reported via OnCpuRestrictedResolutionChanged.

[perkj_webrtc, 2016/10/26 16:40:17]

I think it is needed but it is really up to you. What if the source or
degradation preferences are changed? I assume that we then should set the stats
for weather or not we are currently CPU limited accordingly independently of
what the overuseframedetector say.

The reason there is both SetCpuRestricted and OnCpuRestrictedResolutionChanged
is that OnCpuRestrictedResolutionChanged also update the counter of the number
of times the resolution has been requested to change. That counter is not
updated if you for example switch to screen share and back.

+  });
 }
 
-void ViEEncoder::SetSink(EncoderSink* sink) {
+void ViEEncoder::SetSink(EncoderSink* sink, bool rotation_applied) {
+  source_proxy_->SetWantsRotationApplied(rotation_applied);
   encoder_queue_.PostTask([this, sink] {
     RTC_DCHECK_RUN_ON(&encoder_queue_);
     sink_ = sink;
@@ -463,8 +532,6 @@ void ViEEncoder::ReconfigureEncoder() {
 
 void ViEEncoder::OnFrame(const VideoFrame& video_frame) {
   RTC_DCHECK_RUNS_SERIALIZED(&incoming_frame_race_checker_);
-  stats_proxy_->OnIncomingFrame(video_frame.width(), video_frame.height());
-
   VideoFrame incoming_frame = video_frame;
 
   // Local time in webrtc time base.
@@ -538,6 +605,7 @@ void ViEEncoder::TraceFrameDropEnd() {
 void ViEEncoder::EncodeVideoFrame(const VideoFrame& video_frame,
                                   int64_t time_when_posted_in_ms) {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
+
   if (pre_encode_callback_)
     pre_encode_callback_->OnFrame(video_frame);
 
@@ -565,10 +633,15 @@ void ViEEncoder::EncodeVideoFrame(const VideoFrame& video_frame,
   }
   TraceFrameDropEnd();
 
+  last_frame_height_ = video_frame.height();
+  last_frame_width_ = video_frame.width();
+
   TRACE_EVENT_ASYNC_STEP0("webrtc", "Video", video_frame.render_time_ms(),
                           "Encode");
 
-  overuse_detector_.FrameCaptured(video_frame, time_when_posted_in_ms);
+  if (overuse_detector_) {
+    overuse_detector_->FrameCaptured(video_frame, time_when_posted_in_ms);
+  }
 
   if (codec_type_ == webrtc::kVideoCodecVP8) {
     webrtc::CodecSpecificInfo codec_specific_info;
@@ -611,15 +684,30 @@ EncodedImageCallback::Result ViEEncoder::OnEncodedImage(
     stats_proxy_->OnSendEncodedImage(encoded_image, codec_specific_info);
   }
 
+  if (encoder_timing_) {
+    encoder_timing_->OnEncodeTiming(
+        encoded_image.ntp_time_ms_,
+        clock_->TimeInMilliseconds() - encoded_image.capture_time_ms_);
+  }
+
   EncodedImageCallback::Result result =
       sink_->OnEncodedImage(encoded_image, codec_specific_info, fragmentation);
 
   int64_t time_sent = clock_->TimeInMilliseconds();
   uint32_t timestamp = encoded_image._timeStamp;
-  encoder_queue_.PostTask([this, timestamp, time_sent] {
+
+  auto call_overuse_frame_sent = [this, timestamp, time_sent] {
     RTC_DCHECK_RUN_ON(&encoder_queue_);
-    overuse_detector_.FrameSent(timestamp, time_sent);
-  });
+    if (overuse_detector_) {
+      overuse_detector_->FrameSent(timestamp, time_sent);
+    }
+  };
+  if (encoder_queue_.IsCurrent()) {
+    call_overuse_frame_sent();

[kthelgason, 2016/10/19 12:06:44]

Is this contortion really saving us anything? Seems much simpler to just always
post it to the queue.

[perkj_webrtc, 2016/10/26 16:40:17]

Done.

+  } else {
+    encoder_queue_.PostTask(call_overuse_frame_sent);
+  }
+
   return result;
 }
 
@@ -697,17 +785,43 @@ void ViEEncoder::OnBitrateUpdated(uint32_t bitrate_bps,
 
 void ViEEncoder::OveruseDetected() {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
-  // TODO(perkj): When ViEEncoder inherit rtc::VideoSink instead of
-  // VideoCaptureInput |load_observer_| should be removed and overuse be
-  // expressed as rtc::VideoSinkWants instead.
-  if (load_observer_)
-    load_observer_->OnLoadUpdate(LoadObserver::kOveruse);
+  LOG(LS_INFO) << "OveruseDetected usage";

[kthelgason, 2016/10/19 12:06:44]

This is a weird log message! Perhaps change this to something more descriptive.

[perkj_webrtc, 2016/10/26 16:40:17]

Done.

+  if (cpu_restricted_counter_ >= kMaxCpuDowngrades) {
+    return;
+  }
+  // Request lower resolution if the current resolution is lower than last time
+  // we asked for the resolution to be lowered.
+  // Update stats accordingly.
+  int current_pixel_count = last_frame_height_ * last_frame_width_;
+  if (current_pixel_count <
+      max_pixel_count_.value_or(current_pixel_count + 1)) {

[kthelgason, 2016/10/19 12:06:44]

I think this would read better as (max_pixel_count_ && current_pixel_count <
*max_pixel_count_)

The current_pixel_count + 1 looks like a workaround to me

Actually, it could just be an int initialized to MAX_INT. That seems simpler,
and easier to code.

[perkj_webrtc, 2016/10/26 16:40:17]

Done.

I prefer optional instead of a magic value though to indicate that it is not
set.

+    max_pixel_count_ = rtc::Optional<int>(current_pixel_count);
+    max_pixel_count_step_up_ = rtc::Optional<int>();

[kthelgason, 2016/10/19 12:06:44]

Same here, this could just be an int initialized to 0.

[perkj_webrtc, 2016/10/26 16:40:17]

dito

+    stats_proxy_->OnCpuRestrictedResolutionChanged(true);
+    ++cpu_restricted_counter_;
+    source_proxy_->RequestResolutionLowerThan(current_pixel_count);
+  }
 }
 
 void ViEEncoder::NormalUsage() {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
-  if (load_observer_)
-    load_observer_->OnLoadUpdate(LoadObserver::kUnderuse);
+  int current_pixel_count = last_frame_height_ * last_frame_width_;
+  // Request higher resolution if we are cpu restricted and the the current
+  // resolution is higher than last time we requested higher resolution.
+  // Update stats accordingly.
+  if (cpu_restricted_counter_ > 0 &&
+      current_pixel_count > max_pixel_count_step_up_.value_or(0)) {
+    max_pixel_count_ = rtc::Optional<int>();
+    max_pixel_count_step_up_ = rtc::Optional<int>(current_pixel_count);
+    --cpu_restricted_counter_;
+    stats_proxy_->OnCpuRestrictedResolutionChanged(cpu_restricted_counter_ > 0);
+    source_proxy_->RequestHigherResolutionThan(current_pixel_count);
+  }
+}
+
+bool ViEEncoder::OveruseFrameDetectionEnabled() const {
+  RTC_DCHECK_RUN_ON(&encoder_queue_);
+  return overuse_detector_ != nullptr;
 }
 
 }  // namespace webrtc