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..9b5369b1a9dd2ecd2f2ee11e7889520f1015f9e5 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>();

[perkj_webrtc, 2017/02/13 08:48:03]

nit: isn the word target_pixel_count  here equally confusing?  Naming is hard
and I dont have a strong prefens either or. 

[sprang_webrtc, 2017/02/13 09:29:20]

I think it makes sense, since we want the source to return frames at a
resolution as close as possible to target_pixel_count, rather than "a step up
from X, regardless of the step size". Since we control the step size down, it
makes sense to also try to control the step size up, imo.

     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.

[perkj_webrtc, 2017/02/13 08:48:03]

nope, you have limited to be 4* times the number  of pixels.

[sprang_webrtc, 2017/02/13 09:29:20]

This whole comment is misleading. I'll put up CL to clarify it.

+    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)

[perkj_webrtc, 2017/02/13 08:48:03]

correct cl? Does this fix a bug?

[sprang_webrtc, 2017/02/13 09:29:20]

Yes, if fixes a bug but it only seems to be triggered by FakeEncoder, which
emits frames with qp = -1. It caused the test I added to fail because of
incorrect qp scaling. See earlier comment.

       quality_scaler_->ReportQP(qp);
   });
 
@@ -705,14 +706,21 @@ 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_) {

[perkj_webrtc, 2017/02/13 08:48:03]

Looks like a bug if last_frame_info_ = nullptr. How can we adapt unless a frame
have been received. 

[sprang_webrtc, 2017/02/13 09:29:20]

I think some test triggered that. I may want to figure out why and fix the test
+ DCHECK here instead.

     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_->mode_ == AdaptationRequest::Mode::kAdaptDown &&
+      current_pixel_count >= last_adaptation_request_->input_pixel_count_) {
+    // Don't request lower resolution if the current resolution is not lower
+    // than the last time we asked for the resolution to be lowered.
     return;
+  }
+  last_adaptation_request_.emplace(AdaptationRequest{
+      current_pixel_count, AdaptationRequest::Mode::kAdaptDown});
+
   switch (reason) {
     case kQuality:
       stats_proxy_->OnQualityRestrictedResolutionChanged(
@@ -725,8 +733,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 +745,23 @@ 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_->mode_ == AdaptationRequest::Mode::kAdaptUp &&
+      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, AdaptationRequest::Mode::kAdaptUp});
+
   switch (reason) {
     case kQuality:
       stats_proxy_->OnQualityRestrictedResolutionChanged(
@@ -759,8 +773,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.";