Change rtc::VideoSinkWants to have target and a max pixel count

webrtc/video/vie_encoder.cc

 /*
  *  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.
  */
 
@@ skipping 171 matching lines @@
       // task 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.
     const int pixels_wanted = (pixel_count * 3) / 5;
     if (pixels_wanted < kMinPixelsPerFrame)
       return;
     sink_wants_.max_pixel_count = rtc::Optional<int>(pixels_wanted);
-    sink_wants_.max_pixel_count_step_up = rtc::Optional<int>();
+    sink_wants_.target_pixel_count = rtc::Optional<int>();
     if (source_)
       source_->AddOrUpdateSink(vie_encoder_, sink_wants_);
   }
 
   void RequestHigherResolutionThan(int pixel_count) {
     rtc::CritScope lock(&crit_);
     if (!IsResolutionScalingEnabledLocked()) {
       // This can happen since |degradation_preference_| is set on
       // libjingle's 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);
+    // how the source can scale the input frame size. We still cap the step up
+    // to be at most twice the number of pixels.
+    sink_wants_.target_pixel_count = rtc::Optional<int>((pixel_count * 5) / 3);
+    sink_wants_.max_pixel_count = rtc::Optional<int>(pixel_count * 4);
     if (source_)
       source_->AddOrUpdateSink(vie_encoder_, sink_wants_);
   }
 
  private:
   bool IsResolutionScalingEnabledLocked() const
       EXCLUSIVE_LOCKS_REQUIRED(&crit_) {
     return degradation_preference_ !=
            DegradationPreference::kMaintainResolution;
   }
@@ skipping 389 matching lines @@
 
   EncodedImageCallback::Result result =
       sink_->OnEncodedImage(encoded_image, codec_specific_info, fragmentation);
 
   int64_t time_sent_us = rtc::TimeMicros();
   uint32_t timestamp = encoded_image._timeStamp;
   const int qp = encoded_image.qp_;
   encoder_queue_.PostTask([this, timestamp, time_sent_us, qp] {
     RTC_DCHECK_RUN_ON(&encoder_queue_);
     overuse_detector_.FrameSent(timestamp, time_sent_us);
-    if (quality_scaler_)
+    if (quality_scaler_ && qp >= 0)

[nisse-webrtc, 2017/02/06 08:43:19]

Is this fix related to the other changes?

[sprang_webrtc, 2017/02/06 10:34:02]

This is to prevent the quality scaler from using -1 values (indicating not
available) when calculating the average qp. It resulted in triggering surprising
adaptation requests with the fake encoder used in the test I added.
I'm pretty sure we don't want this behavior from potential external decoders
that don't report qp as well.
+kthelgason can maybe comment on this?

[kthelgason, 2017/02/06 13:31:05]

SGTM. As part of the quality scaler work I tried to make sure all encoders
populate |encoded_frame.qp| but I didn't consider the test encoders and external
implementations. I think this is good to have as a safeguard.

[sprang_webrtc, 2017/02/06 13:49:53]

Acknowledged.

       quality_scaler_->ReportQP(qp);
   });
 
   return result;
 }
 
 void ViEEncoder::OnDroppedFrame() {
   encoder_queue_.PostTask([this] {
     RTC_DCHECK_RUN_ON(&encoder_queue_);
     if (quality_scaler_)
@@ skipping 68 matching lines @@
 
   if (stats_proxy_ && video_suspension_changed) {
     LOG(LS_INFO) << "Video suspend state changed to: "
                  << (video_is_suspended ? "suspended" : "not suspended");
     stats_proxy_->OnSuspendChange(video_is_suspended);
   }
 }
 
 void ViEEncoder::AdaptDown(AdaptReason reason) {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
-  if (degradation_preference_ != DegradationPreference::kBalanced)
+  if (degradation_preference_ != DegradationPreference::kBalanced ||
+      !last_frame_info_) {
     return;
-  // Request lower resolution if the current resolution is lower than last time
-  // we asked for the resolution to be lowered.
-  int current_pixel_count =
-      last_frame_info_ ? last_frame_info_->pixel_count() : 0;
-  if (max_pixel_count_ && current_pixel_count >= *max_pixel_count_)
+  }
+  int current_pixel_count = last_frame_info_->pixel_count();
+  if (last_adaptation_request_ && last_adaptation_request_->adapt_down_ &&
+      current_pixel_count >= last_adaptation_request_->input_pixel_count_) {
+    // Don't request lower resolution if the current resolution is not lower

[nisse-webrtc, 2017/02/06 08:43:19]

When do you expect this to be true? When the source hasn't yet acted on the
previous request, or if it isn't able to produce a smaller resolution?

Might work slightly differently for steps up and down, because max_pixel_count
has higher precedence. 

For step down, it might be clearer to write the test as something like

if (current_pixel_count > wants.max_pixel.count.value_or(MAXINT))
  /* No further action until source honors our max_pixel_count setting */
  return;

Corresponding logic for step up (forther below) is more fuzzy. 

I think current version is ok, but perhaps you could have a second look to see
if the logic can be made clearer.

[sprang_webrtc, 2017/02/06 10:34:02]

When the source hasn't yet acted on the previous request, so we don't want to
request a new step needlessly. Looking direcly at the current max in the sink
wants can maybe slightly simplify this, but would require exposure of that
struct in VideoSourceProxy. Not sure if that's worth it?

I tried to keep the behavior as close as possible to the previous. The old
implementation stored the max and max_step values just to compare to the current
for this purpose. It only populated one value at a time though so it could infer
the direction of the step from that.

+    // than the last time we asked for the resolution to be lowered.
     return;
+  }
+  last_adaptation_request_.emplace(
+      AdaptationRequest{current_pixel_count, true});
+
   switch (reason) {
     case kQuality:
       stats_proxy_->OnQualityRestrictedResolutionChanged(
           scale_counter_[reason] + 1);
       break;
     case kCpu:
       if (scale_counter_[reason] >= kMaxCpuDowngrades)
         return;
       // Update stats accordingly.
       stats_proxy_->OnCpuRestrictedResolutionChanged(true);
       break;
   }
-  max_pixel_count_ = rtc::Optional<int>(current_pixel_count);
-  max_pixel_count_step_up_ = rtc::Optional<int>();
   ++scale_counter_[reason];
   source_proxy_->RequestResolutionLowerThan(current_pixel_count);
   LOG(LS_INFO) << "Scaling down resolution.";
   for (size_t i = 0; i < kScaleReasonSize; ++i) {
     LOG(LS_INFO) << "Scaled " << scale_counter_[i]
                  << " times for reason: " << (i ? "cpu" : "quality");
   }
 }
 
 void ViEEncoder::AdaptUp(AdaptReason reason) {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
   if (scale_counter_[reason] == 0 ||
-      degradation_preference_ != DegradationPreference::kBalanced) {
+      degradation_preference_ != DegradationPreference::kBalanced ||
+      !last_frame_info_) {
     return;
   }
-  // Only scale if resolution is higher than last time
-  // we requested higher resolution.
-  int current_pixel_count =
-      last_frame_info_ ? last_frame_info_->pixel_count() : 0;
-  if (current_pixel_count <= max_pixel_count_step_up_.value_or(0))
+  // Only scale if resolution is higher than last time we requested higher
+  // resolution.
+  int current_pixel_count = last_frame_info_->pixel_count();
+  if (last_adaptation_request_ && !last_adaptation_request_->adapt_down_ &&
+      current_pixel_count <= last_adaptation_request_->input_pixel_count_) {
+    // Don't request higher resolution if the current resolution is not higher
+    // than the last time we asked for the resolution to be higher.
     return;
+  }
+  last_adaptation_request_.emplace(
+      AdaptationRequest{current_pixel_count, false});
+
   switch (reason) {
     case kQuality:
       stats_proxy_->OnQualityRestrictedResolutionChanged(
           scale_counter_[reason] - 1);
       break;
     case kCpu:
       // Update stats accordingly.
       stats_proxy_->OnCpuRestrictedResolutionChanged(scale_counter_[reason] >
                                                      1);
       break;
   }
-  max_pixel_count_ = rtc::Optional<int>();
-  max_pixel_count_step_up_ = rtc::Optional<int>(current_pixel_count);
   --scale_counter_[reason];
   source_proxy_->RequestHigherResolutionThan(current_pixel_count);
   LOG(LS_INFO) << "Scaling up resolution.";
   for (size_t i = 0; i < kScaleReasonSize; ++i) {
     LOG(LS_INFO) << "Scaled " << scale_counter_[i]
                  << " times for reason: " << (i ? "cpu" : "quality");
   }
 }
 
 }  // namespace webrtc