Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher

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 a3b071ac00fb058cad52b3290d00d163550744df..4274d0ae680698f4bb671126349dd2054840d3eb 100644
--- a/webrtc/video/vie_encoder.cc
+++ b/webrtc/video/vie_encoder.cc
@@ -230,54 +230,50 @@ class ViEEncoder::VideoSourceProxy {
   bool RequestResolutionLowerThan(int pixel_count) {
     // Called on the encoder task queue.
     rtc::CritScope lock(&crit_);
-    if (!IsResolutionScalingEnabledLocked()) {
+    if (!source_ || !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 false;
     }
-    // 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.
+    // The input video frame size will have a resolution less than or equal to
+    // |max_pixel_count| depending on how the source can scale the frame size.
     const int pixels_wanted = (pixel_count * 3) / 5;
-    if (pixels_wanted < kMinPixelsPerFrame)
+    if (pixels_wanted < kMinPixelsPerFrame ||
+        pixels_wanted >= sink_wants_.max_pixel_count) {
       return false;
-
+    }
+    LOG(LS_INFO) << "Scaling down resolution.";

[sprang_webrtc, 2017/05/29 14:45:47]

Might be nice with the actual pixel counts in the log message too? Here and
below.

[åsapersson, 2017/05/30 13:57:31]

Done.

     sink_wants_.max_pixel_count = pixels_wanted;
     sink_wants_.target_pixel_count = rtc::Optional<int>();
-    if (source_)
-      source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
     return true;
   }
 
-  void RequestFramerateLowerThan(int framerate_fps) {
+  bool RequestFramerateLowerThan(int fps) {
     // Called on the encoder task queue.

[sprang_webrtc, 2017/05/29 14:45:47]

Can we add thread/sequence checkers for this maybe? here and elsewhere

[åsapersson, 2017/05/30 13:57:30]

I can have a look but maybe in a separate cl?

[sprang_webrtc, 2017/05/31 08:44:30]

Sure, maybe you can add it to one of the upcoming ones?

-    rtc::CritScope lock(&crit_);
-    if (!IsFramerateScalingEnabledLocked()) {
-      // 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 rate will be scaled down to 2/3 of input fps,
-    // rounding down.
-    const int framerate_wanted =
-        std::max(kMinFramerateFps, (framerate_fps * 2) / 3);
-    sink_wants_.max_framerate_fps = framerate_wanted;
-    if (source_)
-      source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    // The input video frame rate will be scaled down to 2/3, rounding down.
+    return RestrictFramerate((fps * 2) / 3);
   }
 
-  void RequestHigherResolutionThan(int pixel_count) {
+  bool RequestHigherResolutionThan(int pixel_count) {
+    // Called on the encoder task queue.
     rtc::CritScope lock(&crit_);
-    if (!IsResolutionScalingEnabledLocked()) {
+    if (!source_ || !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;
+      return false;
     }
+    int max_pixels_wanted = pixel_count;
+    if (max_pixels_wanted != std::numeric_limits<int>::max())
+      max_pixels_wanted = pixel_count * 4;
+
+    if (max_pixels_wanted <= sink_wants_.max_pixel_count)
+      return false;

[sprang_webrtc, 2017/05/29 14:45:47]

When would this happen? Should we reset sink_wants_.max_pixel_count in that
case?

[åsapersson, 2017/05/30 13:57:31]

In balanced mode (https://codereview.webrtc.org/2887303003/),
last_adaptation_request_ is not used. Equal pixel count can happen if
AdaptUp/AdaptDown is called frequently. The intention is for it to work similar
to last_adaptation_request_ (and maybe later remove last_adaptation_request_ if
possible).

[sprang_webrtc, 2017/05/31 08:44:31]

Ack. Maybe add comment about that?

 
-    if (pixel_count == std::numeric_limits<int>::max()) {
+    sink_wants_.max_pixel_count = max_pixels_wanted;
+    if (max_pixels_wanted == std::numeric_limits<int>::max()) {
       // Remove any constraints.
       sink_wants_.target_pixel_count.reset();
-      sink_wants_.max_pixel_count = std::numeric_limits<int>::max();
     } else {
       // On step down we request at most 3/5 the pixel count of the previous
       // resolution, so in order to take "one step up" we request a resolution
@@ -287,31 +283,52 @@ class ViEEncoder::VideoSourceProxy {
       // most four time the current number of pixels.
       sink_wants_.target_pixel_count =
           rtc::Optional<int>((pixel_count * 5) / 3);
-      sink_wants_.max_pixel_count = pixel_count * 4;
     }
-    if (source_)
-      source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    LOG(LS_INFO) << "Scaling up resolution.";
+    source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    return true;
+  }
+
+  bool RequestHigherFramerateThan(int fps) {
+    // Called on the encoder task queue.
+    // The input frame rate will be scaled up to the last step, with rounding.
+    int framerate_wanted = fps;
+    if (fps != std::numeric_limits<int>::max())
+      framerate_wanted = (fps * 3) / 2;
+
+    return IncreaseFramerate(framerate_wanted);
   }
 
-  void RequestHigherFramerateThan(int framerate_fps) {
+  bool RestrictFramerate(int fps) {
     // Called on the encoder task queue.
     rtc::CritScope lock(&crit_);
-    if (!IsFramerateScalingEnabledLocked()) {
-      // This can happen since |degradation_preference_| is set on libjingle's
-      // worker thread but the adaptation is done on the encoder task queue.
-      return;
-    }
-    if (framerate_fps == std::numeric_limits<int>::max()) {
-      // Remove any restrains.
-      sink_wants_.max_framerate_fps = std::numeric_limits<int>::max();
-    } else {
-      // The input video frame rate will be scaled up to the last step, with
-      // rounding.
-      const int framerate_wanted = (framerate_fps * 3) / 2;
-      sink_wants_.max_framerate_fps = framerate_wanted;
-    }
-    if (source_)
-      source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    if (!source_ || !IsFramerateScalingEnabledLocked())
+      return false;
+
+    const int fps_wanted = std::max(kMinFramerateFps, fps);
+    if (fps_wanted >= sink_wants_.max_framerate_fps)
+      return false;
+
+    LOG(LS_INFO) << "Scaling down framerate: " << fps_wanted;
+    sink_wants_.max_framerate_fps = fps_wanted;
+    source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    return true;
+  }
+
+  bool IncreaseFramerate(int fps) {
+    // Called on the encoder task queue.
+    rtc::CritScope lock(&crit_);
+    if (!source_ || !IsFramerateScalingEnabledLocked())
+      return false;
+
+    const int fps_wanted = std::max(kMinFramerateFps, fps);
+    if (fps_wanted <= sink_wants_.max_framerate_fps)
+      return false;
+
+    LOG(LS_INFO) << "Scaling up framerate: " << fps_wanted;
+    sink_wants_.max_framerate_fps = fps_wanted;
+    source_->AddOrUpdateSink(vie_encoder_, GetActiveSinkWants());
+    return true;
   }
 
  private:
@@ -855,17 +872,19 @@ void ViEEncoder::AdaptDown(AdaptReason reason) {
     case VideoSendStream::DegradationPreference::kBalanced:
       FALLTHROUGH();
     case VideoSendStream::DegradationPreference::kMaintainFramerate:
+      // Scale down resolution.
       if (!source_proxy_->RequestResolutionLowerThan(
               adaptation_request.input_pixel_count_)) {
         return;
       }
-      LOG(LS_INFO) << "Scaling down resolution.";
       GetAdaptCounter().IncrementResolution(reason, 1);
       break;
     case VideoSendStream::DegradationPreference::kMaintainResolution:
-      source_proxy_->RequestFramerateLowerThan(
-          adaptation_request.framerate_fps_);
-      LOG(LS_INFO) << "Scaling down framerate.";
+      // Scale down framerate.
+      if (!source_proxy_->RequestFramerateLowerThan(
+              adaptation_request.framerate_fps_)) {
+        return;
+      }
       GetAdaptCounter().IncrementFramerate(reason, 1);
       break;
     case VideoSendStream::DegradationPreference::kDegradationDisabled:
@@ -920,30 +939,30 @@ void ViEEncoder::AdaptUp(AdaptReason reason) {
   switch (degradation_preference_) {
     case VideoSendStream::DegradationPreference::kBalanced:
       FALLTHROUGH();
-    case VideoSendStream::DegradationPreference::kMaintainFramerate:
-      if (adapt_counter.TotalCount() == 1) {
+    case VideoSendStream::DegradationPreference::kMaintainFramerate: {
+      // Scale up resolution.
+      int pixel_count = adaptation_request.input_pixel_count_;
+      if (adapt_counter.ResolutionCount() == 1) {
         LOG(LS_INFO) << "Removing resolution down-scaling setting.";
-        source_proxy_->RequestHigherResolutionThan(
-            std::numeric_limits<int>::max());
-      } else {
-        source_proxy_->RequestHigherResolutionThan(
-            adaptation_request.input_pixel_count_);
-        LOG(LS_INFO) << "Scaling up resolution.";
+        pixel_count = std::numeric_limits<int>::max();
       }
+      if (!source_proxy_->RequestHigherResolutionThan(pixel_count))
+        return;
       GetAdaptCounter().IncrementResolution(reason, -1);
       break;
-    case VideoSendStream::DegradationPreference::kMaintainResolution:
-      if (adapt_counter.TotalCount() == 1) {
+    }
+    case VideoSendStream::DegradationPreference::kMaintainResolution: {
+      // Scale up framerate.
+      int fps = adaptation_request.framerate_fps_;
+      if (adapt_counter.FramerateCount() == 1) {
         LOG(LS_INFO) << "Removing framerate down-scaling setting.";
-        source_proxy_->RequestHigherFramerateThan(
-            std::numeric_limits<int>::max());
-      } else {
-        source_proxy_->RequestHigherFramerateThan(
-            adaptation_request.framerate_fps_);
-        LOG(LS_INFO) << "Scaling up framerate.";
+        fps = std::numeric_limits<int>::max();
       }
+      if (!source_proxy_->RequestHigherFramerateThan(fps))
+        return;
       GetAdaptCounter().IncrementFramerate(reason, -1);
       break;
+    }
     case VideoSendStream::DegradationPreference::kDegradationDisabled:
       RTC_NOTREACHED();
   }