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

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 4ee6b8f1277f4db78b43f910cd645125cba42b55..7d0268923488ac8882827d2bf6c3e749fe07cccd 100644
--- a/webrtc/video/vie_encoder.cc
+++ b/webrtc/video/vie_encoder.cc
@@ -189,7 +189,7 @@ class ViEEncoder::VideoSourceProxy {
     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_);
   }
@@ -204,9 +204,10 @@ class ViEEncoder::VideoSourceProxy {
     }
     // 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_);
   }
@@ -616,7 +617,7 @@ EncodedImageCallback::Result ViEEncoder::OnEncodedImage(
   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);
   });
 
@@ -705,14 +706,20 @@ void ViEEncoder::OnBitrateUpdated(uint32_t bitrate_bps,
 
 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(
@@ -725,8 +732,6 @@ void ViEEncoder::AdaptDown(AdaptReason reason) {
       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.";
@@ -739,15 +744,22 @@ void ViEEncoder::AdaptDown(AdaptReason reason) {
 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(
@@ -759,8 +771,6 @@ void ViEEncoder::AdaptUp(AdaptReason 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.";