VideoAdapter: Add cropping based on OnOutputFormatRequest()

webrtc/media/base/videoadapter.cc

Index: webrtc/media/base/videoadapter.cc
diff --git a/webrtc/media/base/videoadapter.cc b/webrtc/media/base/videoadapter.cc
index 797a876f2b4049c38049bf1477b5ecb80819e022..28d5988badcdf18b123f20daf4eb2e9e43bd1408 100644
--- a/webrtc/media/base/videoadapter.cc
+++ b/webrtc/media/base/videoadapter.cc
@@ -30,16 +30,14 @@ const float kScaleFactors[] = {
     3.f / 16.f,  // 3/16 scale.
 };
 
-float FindScaleLessThanOrEqual(int width,
-                               int height,
+float FindScaleLessThanOrEqual(int input_num_pixels,
                                int target_num_pixels,
                                int* resulting_number_of_pixels) {
   float best_distance = std::numeric_limits<float>::max();
   float best_scale = 0.0f;  // Default to 0 if nothing matches.
-  float pixels = width * height;
   float best_number_of_pixels = 0.0f;
   for (const auto& scale : kScaleFactors) {
-    float test_num_pixels = pixels * scale * scale;
+    float test_num_pixels = input_num_pixels * scale * scale;
     float diff = target_num_pixels - test_num_pixels;
     if (diff < 0) {
       continue;
@@ -59,16 +57,15 @@ float FindScaleLessThanOrEqual(int width,
   return best_scale;
 }
 
-float FindScaleLargerThan(int width,
-                          int height,
+float FindScaleLargerThan(int input_num_pixels,
                           int target_num_pixels,
                           int* resulting_number_of_pixels) {
   float best_distance = std::numeric_limits<float>::max();
   float best_scale = 1.f;  // Default to unscaled if nothing matches.
-  float pixels = width * height;
-  float best_number_of_pixels = pixels;  // Default to input number of pixels.
+  // Default to input number of pixels.
+  float best_number_of_pixels = input_num_pixels;
   for (const auto& scale : kScaleFactors) {
-    float test_num_pixels = pixels * scale * scale;
+    float test_num_pixels = input_num_pixels * scale * scale;
     float diff = test_num_pixels - target_num_pixels;
     if (diff <= 0) {
       break;
@@ -84,21 +81,36 @@ float FindScaleLargerThan(int width,
   return best_scale;
 }
 
+float FindScale(int input_num_pixels,
+                int max_pixel_count_step_up,
+                int max_pixel_count) {
+  // Try scale just above |max_pixel_count_step_up_|.
+  if (max_pixel_count_step_up > 0) {
+    int resulting_pixel_count;
+    const float scale = FindScaleLargerThan(
+        input_num_pixels, max_pixel_count_step_up, &resulting_pixel_count);
+    if (resulting_pixel_count <= max_pixel_count)
+      return scale;
+  }
+  // Return largest scale below |max_pixel_count|.
+  return FindScaleLessThanOrEqual(input_num_pixels, max_pixel_count, nullptr);
+}
+
 }  // namespace
 
 namespace cricket {
 
 VideoAdapter::VideoAdapter()
-    : output_num_pixels_(std::numeric_limits<int>::max()),
-      frames_in_(0),
+    : frames_in_(0),
       frames_out_(0),
       frames_scaled_(0),
       adaption_changes_(0),
       previous_width_(0),
       previous_height_(0),
+      input_interval_(0),
       interval_next_frame_(0),
-      format_request_max_pixel_count_(std::numeric_limits<int>::max()),
-      resolution_request_max_pixel_count_(std::numeric_limits<int>::max()) {}
+      resolution_request_max_pixel_count_(std::numeric_limits<int>::max()),
+      resolution_request_max_pixel_count_step_up_(0) {}
 
 VideoAdapter::~VideoAdapter() {}
 
@@ -106,57 +118,39 @@ void VideoAdapter::SetExpectedInputFrameInterval(int64_t interval) {
   // TODO(perkj): Consider measuring input frame rate instead.
   // Frame rate typically varies depending on lighting.
   rtc::CritScope cs(&critical_section_);
-  input_format_.interval = interval;
-}
-
-void VideoAdapter::SetInputFormat(const VideoFormat& format) {
-  bool is_resolution_change = (input_format().width != format.width ||
-                               input_format().height != format.height);
-  int64_t old_input_interval = input_format_.interval;
-  input_format_ = format;
-  output_format_.interval =
-      std::max(output_format_.interval, input_format_.interval);
-  if (old_input_interval != input_format_.interval) {
-    LOG(LS_INFO) << "VAdapt input interval changed from "
-      << old_input_interval << " to " << input_format_.interval;
-  }
-  if (is_resolution_change) {
-    // Trigger the adaptation logic again, to potentially reset the adaptation
-    // state for things like view requests that may not longer be capping
-    // output (or may now cap output).
-    Adapt(std::min(format_request_max_pixel_count_,
-                   resolution_request_max_pixel_count_),
-          0);
-  }
+  input_interval_ = interval;
 }
 
-const VideoFormat& VideoAdapter::input_format() const {
-  rtc::CritScope cs(&critical_section_);
-  return input_format_;
-}
-
-VideoFormat VideoAdapter::AdaptFrameResolution(int in_width, int in_height) {
+void VideoAdapter::AdaptFrameResolution(int in_width,
+                                        int in_height,
+                                        int* cropped_width,
+                                        int* cropped_height,
+                                        int* out_width,
+                                        int* out_height) {
   rtc::CritScope cs(&critical_section_);
   ++frames_in_;
 
-  SetInputFormat(VideoFormat(
-      in_width, in_height, input_format_.interval, input_format_.fourcc));
+  // The max output pixel count is the minimum of the requests from
+  // OnOutputFormatRequest and OnResolutionRequest.
+  int max_pixel_count = resolution_request_max_pixel_count_;
+  if (requested_format_) {
+    max_pixel_count = std::min(
+        max_pixel_count, requested_format_->width * requested_format_->height);
+  }
 
   // Drop the input frame if necessary.
   bool should_drop = false;
-  if (!output_num_pixels_) {
+  if (max_pixel_count == 0) {
     // Drop all frames as the output format is 0x0.
     should_drop = true;
-  } else {
+  } else if (requested_format_ && requested_format_->interval > 0) {
     // Drop some frames based on input fps and output fps.
     // Normally output fps is less than input fps.
-    interval_next_frame_ += input_format_.interval;
-    if (output_format_.interval > 0) {
-      if (interval_next_frame_ >= output_format_.interval) {
-        interval_next_frame_ %= output_format_.interval;
-      } else {
-        should_drop = true;
-      }
+    interval_next_frame_ += input_interval_;
+    if (interval_next_frame_ >= requested_format_->interval) {
+      interval_next_frame_ %= requested_format_->interval;

[nisse-webrtc, 2016/05/13 07:58:19]

If it is ensured that input_interval < requested_format_->interval (which is the
case where dropping frames make sense, right?), the expensive %= can be replaced
by -=. So I'd suggest

1. Change the outer check to "requested_format_->interval > input_interval".

2. Change %= to -=.

3. Reset interval_next_frame to zero whenever requested_format_->interval is
changed. 

This is a *very* minor performance improvement (order of 50 cycles per frame),
but I also think it makes the logic a bit clearer.

[magjed_webrtc, 2016/05/13 09:12:59]

1) I think in the near future, we want to remove the fixed |input_interval_| and
calculate it on a per-frame basis from the frame timestamps instead, and then
that check doesn't make sense.
2) I think '%=' is more robust than '-=', because then we know for sure that we
will never accumulate a big value.
3) Done.

[nisse-webrtc, 2016/05/13 11:47:06]

I see. Don't bother changing that now, then.
I still don't quite see that % makes sense. For robustness, I think

if (next >= interval) {
  next -= interval;
  if (next >= interval)
    next = 0;
}

would be more reasonable. In the case that the result of the first subtraction
is too large, that means that we could have produced additional in-between
frames, and the remainder from %= represents the time since the latest of those
in-between frame. But since the in-between frames are non-existing, that value
isn't very useful.
Good.

[magjed_webrtc, 2016/05/13 14:22:28]

Done.

+    } else {
+      should_drop = true;
     }
   }
   if (should_drop) {
@@ -170,48 +164,71 @@ VideoFormat VideoAdapter::AdaptFrameResolution(int in_width, int in_height) {
                    << " Changes: " << adaption_changes_
                    << " Input: " << in_width
                    << "x" << in_height
-                   << " i" << input_format_.interval
-                   << " Output: i" << output_format_.interval;
+                   << " i" << input_interval_
+                   << " Output: i"
+                   << (requested_format_ ? requested_format_->interval : 0);
     }
 
-    return VideoFormat();  // Drop frame.
+    // Drop frame.
+    *cropped_width = 0;
+    *cropped_height = 0;
+    *out_width = 0;
+    *out_height = 0;
+    return;
   }
 
-  const float scale = FindScaleLessThanOrEqual(in_width, in_height,
-                                               output_num_pixels_, nullptr);
-  const int output_width = static_cast<int>(in_width * scale + .5f);
-  const int output_height = static_cast<int>(in_height * scale + .5f);
+  // Calculate how the input should be cropped.
+  if (!requested_format_ ||
+      requested_format_->width == 0 || requested_format_->height == 0) {
+    *cropped_width = in_width;
+    *cropped_height = in_height;
+  } else {
+    // Adjust |requested_format_| orientation to match input.
+    if ((in_width > in_height) !=
+        (requested_format_->width > requested_format_->height)) {
+      std::swap(requested_format_->width, requested_format_->height);
+    }
+    const float requested_aspect =

[nisse-webrtc, 2016/05/13 07:58:19]

I liked my integer-only crop logic in 

https://codereview.webrtc.org/1960073002/patch/160001/170005

I.e., something like

// Size of the used portion of the input frame.
crop_width = in_width;
crop_height = in_height;

if (... requested format specified...) {

  // Crop the input frame, if needed to preserve aspect ratio.
  int aspect_diff = crop_width * dst_height -
                    crop_height * dst_width;
  if (aspect_diff > 0) {
    // Horizontal crop.
    crop_width -= aspect_diff / dst_height;
  } else if (aspect_diff < 0) {
    // Vertical crop.
    crop_height -= (-aspect_diff) / dst_width;
  }
}

(My cl used long for aspect_diff, to allow width and height beyond 16 bits. But
since max_pixel_count is a plain int anyway, I guess a plain int should be good
enough here too).

[magjed_webrtc, 2016/05/13 09:12:59]

Your interger-only crop is clever, but I don't see the point. This code is not
performance critical and your code is longer, and IMO less intuitive.

[nisse-webrtc, 2016/05/13 11:47:06]

Ok.

+        requested_format_->width /
+        static_cast<float>(requested_format_->height);
+    *cropped_width =
+        std::min(in_width, static_cast<int>(in_height * requested_aspect));
+    *cropped_height =
+        std::min(in_height, static_cast<int>(in_width / requested_aspect));
+  }
+
+  // Find best scale factor.
+  const float scale =
+      FindScale(*cropped_width * *cropped_height,
+                resolution_request_max_pixel_count_step_up_, max_pixel_count);
+
+  // Calculate final output size.
+  *out_width = static_cast<int>(*cropped_width * scale + .5f);

[nisse-webrtc, 2016/05/13 07:58:19]

But now I'm getting confused... I thought the requested format above was the
intended output format. I'm trying to understand how this is supposed to work,
let me think aloud.

It might be a bit more natural to switch things around: Start with the requested
format, compare to max pixels constraints etc, and decide on the output format.

And after the output size is decided, compute cropping parameters. 

But then you pass the cropped size as input to the FindScale function. The
purpose FindScale is to get a nice scaling ratio, right? But if so, then
computing the output size with rounding seems bad, since any rounding error can
destroy the ratio, changing it from, say, 0.25 to 0.2501.

To get things right, I'm afraid we'd need integer logic, and either
unconditionally force crop size to be multiple of 16 , or tweak the cropping
after the scale is chosen. And in either case, potentially resulting in a small
error in aspect ratio.

Say we have the scale 3/16, and crop size 680. The out size is 127.5, say we
round it down to 127 (or up to 128, neither is any good). Then input size for an
exact scaling ratio would be 677.3333, which is no good. We need to round the
output size down to a multiple of 3, i.e., 126. 

Thus, to get exact 3/16 scaling for a crop size close to 680, we'd have to
select a crop size of 672 and an output size of 126. Next larger possible choice
is crop size 688, output size 129.

I think this ought to be fixed, maybe in this cl?

[magjed_webrtc, 2016/05/13 09:12:59]

I need to base the scale factor on the number of cropped input pixels, not on
the requested format, that's why I'm starting with cropping.

Yes, the output size is rounded and can theoretically destroy the optimized path
in libyuv::Scale. This was the case before my CL as well. I think it's very
unlikely given normal resolutions, and it's not directly catastrophically when
it fails, it will just be slightly slower. For textures it doesn't matter at
all.

To get a perfect scale factor, we need to align both cropped width and height to
the denominator of the scale factor. I can fix it, but I think it's an overkill.

+  *out_height = static_cast<int>(*cropped_height * scale + .5f);
 
   ++frames_out_;
   if (scale != 1)
     ++frames_scaled_;
 
-  if (previous_width_ && (previous_width_ != output_width ||
-                          previous_height_ != output_height)) {
+  if (previous_width_ && (previous_width_ != *out_width ||
+                          previous_height_ != *out_height)) {
     ++adaption_changes_;
     LOG(LS_INFO) << "Frame size changed: scaled " << frames_scaled_ << " / out "
                  << frames_out_ << " / in " << frames_in_
                  << " Changes: " << adaption_changes_ << " Input: " << in_width
-                 << "x" << in_height << " i" << input_format_.interval
-                 << " Scale: " << scale << " Output: " << output_width << "x"
-                 << output_height << " i" << output_format_.interval;
+                 << "x" << in_height << " i" << input_interval_
+                 << " Scale: " << scale << " Output: " << *out_width << "x"
+                 << *out_height << " i"
+                 << (requested_format_ ? requested_format_->interval : 0);
   }
 
-  output_format_.width = output_width;
-  output_format_.height = output_height;
-  previous_width_ = output_width;
-  previous_height_ = output_height;
-
-  return output_format_;
+  previous_width_ = *out_width;
+  previous_height_ = *out_height;
 }
 
 void VideoAdapter::OnOutputFormatRequest(const VideoFormat& format) {
   rtc::CritScope cs(&critical_section_);
-  format_request_max_pixel_count_ = format.width * format.height;
-  output_format_.interval = format.interval;
-  Adapt(std::min(format_request_max_pixel_count_,
-                 resolution_request_max_pixel_count_),
-        0);
+  requested_format_ = rtc::Optional<VideoFormat>(format);
 }
 
 void VideoAdapter::OnResolutionRequest(
@@ -220,44 +237,8 @@ void VideoAdapter::OnResolutionRequest(
   rtc::CritScope cs(&critical_section_);
   resolution_request_max_pixel_count_ =
       max_pixel_count.value_or(std::numeric_limits<int>::max());
-  Adapt(std::min(format_request_max_pixel_count_,
-                 resolution_request_max_pixel_count_),
-        max_pixel_count_step_up.value_or(0));
-}
-
-bool VideoAdapter::Adapt(int max_num_pixels, int max_pixel_count_step_up) {
-  float scale_lower =
-      FindScaleLessThanOrEqual(input_format_.width, input_format_.height,
-                               max_num_pixels, &max_num_pixels);
-  float scale_upper =
-      max_pixel_count_step_up > 0
-          ? FindScaleLargerThan(input_format_.width, input_format_.height,
-                                max_pixel_count_step_up,
-                                &max_pixel_count_step_up)
-          : 1.f;
-
-  bool use_max_pixel_count_step_up =
-      max_pixel_count_step_up > 0 && max_num_pixels > max_pixel_count_step_up;
-
-  int old_num_pixels = output_num_pixels_;
-  output_num_pixels_ =
-      use_max_pixel_count_step_up ? max_pixel_count_step_up : max_num_pixels;
-  // Log the new size.
-  float scale = use_max_pixel_count_step_up ? scale_upper : scale_lower;
-  int new_width = static_cast<int>(input_format_.width * scale + .5f);
-  int new_height = static_cast<int>(input_format_.height * scale + .5f);
-
-  bool changed = output_num_pixels_ != old_num_pixels;
-  LOG(LS_INFO) << "OnResolutionRequest: "
-               << " Max pixels: " << max_num_pixels
-               << " Max pixels step up: " << max_pixel_count_step_up
-               << " Output Pixels: " << output_num_pixels_
-               << " Input: " << input_format_.width << "x"
-               << input_format_.height << " Scale: " << scale
-               << " Resolution: " << new_width << "x" << new_height
-               << " Changed: " << (changed ? "true" : "false");
-
-  return changed;
+  resolution_request_max_pixel_count_step_up_ =
+      max_pixel_count_step_up.value_or(0);
 }
 
 }  // namespace cricket