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..31c6eac6351f1fe6a0a5eda53383483e237833f3 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,91 @@ 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 can do here. 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 we
+  // get here and read the clock.
+  //
+  // 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.
+
+  // TODO(nisse): Don't read the clock here, instead let the caller
+  // pass in the current system time? Useful for testing, or if the
+  // application reads the system clock earlier.
+  int64_t diff_us = system_time_us - camera_time_us;
+
+  // We also try to detect if the camera timestamp actually is using
+  // the system monotonic clock (a common case, for cameras without
+  // builtin timestamping). In this case, we aim to keep the
+  // timestamps as if, i.e., set the offset to zero. In case the
+  // camera clock is drifting, and by chance the offset is crossing
+  // zero, this hack will only cause a small dent in the otherwise
+  // linear offset curve, temporarily forcing it closer to zero.
+  static const int64_t kDelayLimitUs = 50000;
+  if (diff_us > 0 && diff_us < kDelayLimitUs) {
+    diff_us = 0;
+  }

[stefan-webrtc, 2016/06/08 11:28:42]

Why do we have to do this?

[nisse-webrtc, 2016/06/09 10:02:58]

The above? I'd like the filtering to add jitter in the case the incoming
timestamps are already system time.

An alternative might be to let applications which use system time somehow bypass
the translation. Which sounds like the right thing, but I'm afraid it's not so
easy. The application might not really know, and if we start depending on having
system time, we might not want to trust the application to get it right.

[stefan-webrtc, 2016/06/09 11:59:09]

But if it's using system time, shouldn't diff_us be very very close to zero? The
jitter added should be super small after filtering. It seems like it will mostly
complicate the code with another threshold, and at the same time it may hide
bugs in the filtering since only a few cameras may actually run the filtering
code.

[nisse-webrtc, 2016/06/10 12:37:57]

I think you're right, it's unlikely to make any noticable difference. Deleting
it for now, we could add it back later if need arises.

+  // TODO(nisse): Do we need to detect jumps in the camera clock?
+  // E.g., if the camera is somehow reset mid-stream? We could check
+  // if abs(diff_us - offset_us) > 500ms or so, and in this case reset
+  // frames_seen_ to zero.

[stefan-webrtc, 2016/06/08 11:28:42]

I'd suggest using cusum change detection to detect this, as we do in
timestamp_extrapolator.cc. Look for
TimestampExtrapolator::DelayChangeDetection().

I do think that it would be a good idea to have detection like that as it isn't
unlikely that something could happen...

[nisse-webrtc, 2016/06/09 10:02:58]

I think cusum is overkill (and when possible, I try to avoid algorithms with
complicated thresholds). Do you think a simple comparison to a limit of a few
100 ms will be good enough? I'm adding that (even though I'd prefer an explicit
reset on capture start).

My thinking is that if we're more than 500 ms off, whatever the reason (camera
being reset, or application not getting scheduled), we're having some kind of
glitch anyway and resetting the translation shouldn't make things noticably
worse. While if we have a camera time jump less than 500 ms, converging
gradually over the next few hundred frames should also give a tolerable
experience.

A slightly different approach could be to remember the previous camera
timestamp. If the camera time is earlier than then previous time, or more than
0.5 s later, reset the filter, regardless of whether or not the current offset
happens to be close. Under the theory that this filter really shouldn't hold on
to historic data.

[stefan-webrtc, 2016/06/09 11:59:09]

I think the latter approach sounds pretty good then. As you say, the filter will
adapt to the change, so if it was small enough we should recover fairly quickly.

[nisse-webrtc, 2016/06/10 12:37:57]

I changed it to the second approach changed, i.e., onyl check for jumps in the
camera time.

+  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* time_us) {

[stefan-webrtc, 2016/06/08 11:28:42]

Should this be called something else? system_capture_time_us?

[nisse-webrtc, 2016/06/09 10:02:58]

Maybe. In some of the callers, I use the name translated_camera_time_us. Is that
better, or do you prefer system_capture_time_us?

[stefan-webrtc, 2016/06/09 11:59:09]

Both work for me.

+  if (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 +314,10 @@ bool VideoCapturer::AdaptFrame(int width,
     *crop_x = 0;
     *crop_y = 0;
   }
+
+  if (time_us)
+    *time_us = camera_time_us + offset_us_;
+
   return true;
 }
 
@@ -258,9 +331,16 @@ void VideoCapturer::OnFrameCaptured(VideoCapturer*,
   int crop_y;
 
   if (!AdaptFrame(captured_frame->width, captured_frame->height,
-                  captured_frame->time_stamp,
+                  captured_frame->time_stamp / rtc::kNumNanosecsPerMicrosec,
+                  // TODO(nisse): Disable use of the timestamp

[stefan-webrtc, 2016/06/08 11:28:42]

Should this be fixed now?

[nisse-webrtc, 2016/06/09 10:02:58]

The comment is perhaps badly phrased, I'm trying to make it clearer. The
translation *is* now disabled in this case. My plan is that as soon as this cl
is landed, Chrome should be updated to use this interface and purged from all
usage of VideoFrameFactory and SignalFrameCaptured. 

And at that point, this function becomes mostly irrelevant, and we should figure
out how to schedule its removal (together with VideoFrameFactory and
SignalFrameCaptured).

[stefan-webrtc, 2016/06/09 11:59:09]

Acknowledged.

+                  // translation when using the SignalCapturedFrame
+                  // interface. Enabling it breaks the
+                  // WebRtcVideoEngine2Test.PropagatesInputFrameTimestamp
+                  // test. Probably not worth the effort to fix,
+                  // instead, try to get rid of this method.
+                  0,
                   &out_width, &out_height,
-                  &crop_width, &crop_height, &crop_x, &crop_y)) {
+                  &crop_width, &crop_height, &crop_x, &crop_y, nullptr)) {
     return;
   }