Implement timestamp translation/filter in VideoCapturer.

webrtc/media/base/videocapturer.cc

Index: webrtc/media/base/videocapturer.cc
diff --git a/webrtc/media/base/videocapturer.cc b/webrtc/media/base/videocapturer.cc
index 96a605585509220b12875ecf993fc5010fe5e1cc..619483787fe780a7f4abc095b4dce0ccbc3c808c 100644
--- a/webrtc/media/base/videocapturer.cc
+++ b/webrtc/media/base/videocapturer.cc
@@ -59,7 +59,8 @@ bool CapturedFrame::GetDataSize(uint32_t* size) const {
 /////////////////////////////////////////////////////////////////////
 // Implementation of class VideoCapturer
 /////////////////////////////////////////////////////////////////////
-VideoCapturer::VideoCapturer() : apply_rotation_(false) {
+VideoCapturer::VideoCapturer()
+    : apply_rotation_(false), frames_seen_(0), offset_us_(0) {
   thread_checker_.DetachFromThread();
   Construct();
 }
@@ -214,23 +215,88 @@ void VideoCapturer::OnSinkWantsChanged(const rtc::VideoSinkWants& wants) {
   }
 }
 
+void VideoCapturer::UpdateOffset(
+    int64_t camera_time_us, int64_t system_time_us) {
+  // Estimate the offset between system monotonic time and the capture
+  // time from the camera. The camera is assumed to provide more
+  // accurate timestamps than we get from the system time. But the
+  // camera may use its own free-running clock with a large offset and
+  // a small drift compared to the system clock. So the model is
+  // basically
+  //
+  //   y_k = c_0 + c_1 x_k + v_k
+  //
+  // where x_k is the camera timestamp, believed to be accurate in its
+  // own scale. y_k is our reading of the system clock. v_k is the
+  // measurement noise, i.e., the delay from frame capture until the
+  // system clock was read.
+  //
+  // It's possible to do (weighted) least-squares estimation of both
+  // c_0 and c_1. Then we get the constants as c_1 = Cov(x,y) /
+  // Var(x), and c_0 = mean(y) - c_1 mean(x). Substituting this c_0,
+  // we can rearrange the model as
+  //
+  //   y_k = mean(y) + (x_k - mean(x)) + (c_1 - 1) (x_k - mean(x)) + v_k
+  //
+  // Now if we use a weighted average which gradually forgets old
+  // values, x_k - mean(x) is bounded, of the same order as the time
+  // constant (and close to constant for a steady frame rate). In
+  // addition, the frequency error |c_1 - 1| should be small. Cameras
+  // with a frequency error up to 3000 ppm (3 ms drift per second)
+  // have been observed, but frequency errors below 100 ppm could be
+  // expected of any cheap crystal.
+  //
+  // Bottom line is that we ignore the c_1 term, and use only the estimator
+  //
+  //    x_k + mean(y-x)
+  //
+  // where mean is plain averaging for initial samples, followed by
+  // exponential averaging.
+
+  if (frames_seen_) {
+    // Reset the filter whenever there is a jump in camera timestamps.
+    // This happens both when capture is restarted, and if the camera
+    // clock for any reason is reset.
+    static const int64_t kResetLimitUs = 300000;
+
+    if (camera_time_us < prev_camera_time_us_
+        || camera_time_us > prev_camera_time_us_ + kResetLimitUs) {

[stefan-webrtc, 2016/06/10 13:13:31]

Hm, what if we have only 1 fps input? I think we need to be comparing
camera_time_us - prev_camera_time_us_ to system_time_us - prev_system_time_us_.

[nisse-webrtc, 2016/06/13 06:52:49]

Then we'll ignore the camera timestamp and use only system time. I'm thinking
that high accuracy for the frame timestamps is beneficial mainly for high-fps
video, so I wouldn't expect it to be a problem. Or do you think jitter on the
system time reading would cause any noticable problem in a 1 fps (or 2 fps)
video stream?
If we want to avoid resetting the filter in the case that (1) there are largish
intervals between frames, and (2) the camera time still is consistent with no
jumps,  then I think it's better to go back to comparing 

  std::abs(diff_us - offset_us_)

like in an earlier patch set. Then we don't need any additional state just for
the reset logic. (And to repeat, I would prefer to not have any automatic reset
logic here, and instead do a reset on StartCapturing. But that seems tricky to
do with the current organization of the interface).

[stefan-webrtc, 2016/06/13 08:56:01]

This is probably true, but it seems a bit ugly to me to have an arbitrary
threshold when we no longer care about the capture timestamp and instead reset
the filter on every frame.
Ok, maybe the abs diff is better then. I'm fine with that.

[nisse-webrtc, 2016/06/13 09:33:04]

I've changed the reset logic back. Also added a log message when reset happens.

+      frames_seen_ = 0;
+    }
+  }
+  prev_camera_time_us_ = camera_time_us;
+
+  // The input for averaging, y_k - x_k in the above notation.
+  int64_t diff_us = system_time_us - camera_time_us;
+
+  static const unsigned kWindowSize = 100;
+  if (frames_seen_ < kWindowSize) {
+    ++frames_seen_;
+  }
+  offset_us_ += (diff_us - offset_us_) / frames_seen_;
+}
+
 bool VideoCapturer::AdaptFrame(int width,
                                int height,
-                               // TODO(nisse): Switch to us unit.
-                               int64_t capture_time_ns,
+                               int64_t camera_time_us,
+                               int64_t system_time_us,
                                int* out_width,
                                int* out_height,
                                int* crop_width,
                                int* crop_height,
                                int* crop_x,
-                               int* crop_y) {
+                               int* crop_y,
+                               int64_t* translated_camera_time_us) {
+  if (translated_camera_time_us)
+    UpdateOffset(camera_time_us, system_time_us);
+
   if (!broadcaster_.frame_wanted()) {
     return false;
   }
 
   if (enable_video_adapter_ && !IsScreencast()) {
     if (!video_adapter_.AdaptFrameResolution(
-            width, height, capture_time_ns,
+            width, height, camera_time_us * rtc::kNumNanosecsPerMicrosec,
             crop_width, crop_height, out_width, out_height)) {
       // VideoAdapter dropped the frame.
       return false;
@@ -245,6 +311,10 @@ bool VideoCapturer::AdaptFrame(int width,
     *crop_x = 0;
     *crop_y = 0;
   }
+
+  if (translated_camera_time_us)
+    *translated_camera_time_us = camera_time_us + offset_us_;
+
   return true;
 }
 
@@ -257,10 +327,17 @@ void VideoCapturer::OnFrameCaptured(VideoCapturer*,
   int crop_x;
   int crop_y;
 
+  // TODO(nisse): We don't do timestamp translation on this input
+  // path. It seems straight-forward to enable translation, but that
+  // breaks the WebRtcVideoEngine2Test.PropagatesInputFrameTimestamp
+  // test. Probably not worth the effort to fix, instead, try to
+  // delete or refactor all code using VideoFrameFactory and
+  // SignalCapturedFrame.
   if (!AdaptFrame(captured_frame->width, captured_frame->height,
-                  captured_frame->time_stamp,
+                  captured_frame->time_stamp / rtc::kNumNanosecsPerMicrosec,
+                  0,
                   &out_width, &out_height,
-                  &crop_width, &crop_height, &crop_x, &crop_y)) {
+                  &crop_width, &crop_height, &crop_x, &crop_y, nullptr)) {
     return;
   }