VideoAdapter: Drop frames based on actual fps instead of expected fps

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 9c3837c5729bc4b48531689c36ed2e0a9206afc7..f1d2f18c791c4db043ee437a6e482f34a043ecfd 100644
--- a/webrtc/media/base/videoadapter.cc
+++ b/webrtc/media/base/videoadapter.cc
@@ -118,22 +118,45 @@ VideoAdapter::VideoAdapter()
       adaption_changes_(0),
       previous_width_(0),
       previous_height_(0),
-      input_interval_(0),
-      interval_next_frame_(0),
+      previous_timestamp_ns_(0),

[nisse-webrtc, 2016/05/18 11:44:00]

Please make it an rtc::Optional, instead of using 0 as a magic value for unset.

[magjed_webrtc, 2016/05/18 16:35:22]

Done.

+      interval_ns_next_frame_(0),

[nisse-webrtc, 2016/05/18 11:44:00]

There should be a comment somewhere explaining the meaning of this variable. I
take it is time left until the next frame should be sent?

It might be clearer to replace both previous_timestamp_ns_ and
interval_ns_next_frame_ by a variable holding the absolute time of the next
frame.

nit: I think unit should be moved to the end of the name.

[magjed_webrtc, 2016/05/18 16:35:22]

Done.

       resolution_request_max_pixel_count_(std::numeric_limits<int>::max()),
       resolution_request_max_pixel_count_step_up_(0) {}
 
 VideoAdapter::~VideoAdapter() {}
 
-void VideoAdapter::SetExpectedInputFrameInterval(int64_t interval) {
-  // TODO(perkj): Consider measuring input frame rate instead.
-  // Frame rate typically varies depending on lighting.
+bool VideoAdapter::ShouldDrop(int64_t in_timestamp_ns) {

[nisse-webrtc, 2016/05/18 11:44:00]

I'd prefer if false meant that the frame should be dropped. Name could be
"KeepFrame".

[magjed_webrtc, 2016/05/18 16:35:23]

Done.

   rtc::CritScope cs(&critical_section_);
-  input_interval_ = interval;
+  if (!requested_format_ || requested_format_->interval == 0)
+    return false;
+
+  // Reset counters if it's the first frame, if input fps is higher than
+  // requested fps, or if timestamps are out of order. Always output the
+  // first frame after a reset.
+  const int64_t frame_delta_ns = (in_timestamp_ns - previous_timestamp_ns_);
+  if (previous_timestamp_ns_ == 0 || frame_delta_ns <= 0 ||
+      frame_delta_ns >= requested_format_->interval) {
+    interval_ns_next_frame_ = 0;
+    previous_timestamp_ns_ = in_timestamp_ns;
+    return false;
+  }
+
+  // Increment counters.
+  interval_ns_next_frame_ += frame_delta_ns;
+  previous_timestamp_ns_ = in_timestamp_ns;
+
+  // Drop if elapsed time is less then requested.
+  if (interval_ns_next_frame_ < requested_format_->interval)
+    return true;
+
+  // Time to output new frame.
+  interval_ns_next_frame_ -= requested_format_->interval;
+  return false;
 }
 
 void VideoAdapter::AdaptFrameResolution(int in_width,
                                         int in_height,
+                                        int64_t in_timestamp_ns,
                                         int* cropped_width,
                                         int* cropped_height,
                                         int* out_width,
@@ -150,25 +173,7 @@ void VideoAdapter::AdaptFrameResolution(int in_width,
   }
 
   // Drop the input frame if necessary.
-  bool should_drop = false;
-  if (max_pixel_count == 0) {
-    // Drop all frames as the output format is 0x0.
-    should_drop = true;
-  } 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_interval_;
-    if (interval_next_frame_ >= requested_format_->interval) {
-      interval_next_frame_ -= requested_format_->interval;
-      // Reset |interval_next_frame_| if it accumulates too much to avoid
-      // "catching up" behaviour.
-      if (interval_next_frame_ >= requested_format_->interval)
-        interval_next_frame_ = 0;
-    } else {
-      should_drop = true;
-    }
-  }
-  if (should_drop) {
+  if (max_pixel_count == 0 || ShouldDrop(in_timestamp_ns)) {
     // Show VAdapt log every 90 frames dropped. (3 seconds)
     if ((frames_in_ - frames_out_) % 90 == 0) {
       // TODO(fbarchard): Reduce to LS_VERBOSE when adapter info is not needed
@@ -179,7 +184,7 @@ void VideoAdapter::AdaptFrameResolution(int in_width,
                    << " Changes: " << adaption_changes_
                    << " Input: " << in_width
                    << "x" << in_height
-                   << " i" << input_interval_
+                   << " timestamp: " << in_timestamp_ns
                    << " Output: i"
                    << (requested_format_ ? requested_format_->interval : 0);
     }
@@ -238,7 +243,7 @@ void VideoAdapter::AdaptFrameResolution(int in_width,
     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_interval_
+                 << "x" << in_height
                  << " Scale: " << scale.numerator << "/" << scale.denominator
                  << " Output: " << *out_width << "x" << *out_height << " i"
                  << (requested_format_ ? requested_format_->interval : 0);
@@ -251,7 +256,8 @@ void VideoAdapter::AdaptFrameResolution(int in_width,
 void VideoAdapter::OnOutputFormatRequest(const VideoFormat& format) {
   rtc::CritScope cs(&critical_section_);
   requested_format_ = rtc::Optional<VideoFormat>(format);
-  interval_next_frame_ = 0;
+  previous_timestamp_ns_ = 0;
+  interval_ns_next_frame_ = 0;
 }
 
 void VideoAdapter::OnResolutionRequest(