New method TimestampAligner::TranslateTimestamp

webrtc/base/timestampaligner.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <limits>
+
 #include "webrtc/base/logging.h"
 #include "webrtc/base/timestampaligner.h"
 
 namespace rtc {
 
-TimestampAligner::TimestampAligner() : frames_seen_(0), offset_us_(0) {}
+TimestampAligner::TimestampAligner()
+    : frames_seen_(0),
+      offset_us_(0),
+      clip_bias_us_(0),
+      prev_translated_time_us_(std::numeric_limits<int64_t>::min()) {}
 
 TimestampAligner::~TimestampAligner() {}
 
+int64_t TimestampAligner::TranslateTimestamp(int64_t camera_time_us,
+                                             int64_t system_time_us) {
+  return ClipTimestamp(
+      camera_time_us + UpdateOffset(camera_time_us, system_time_us),
+      system_time_us);
+}
+
 int64_t TimestampAligner::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
@@ skipping 26 matching lines @@
   // exponential averaging.
 
   // The input for averaging, y_k - x_k in the above notation.
   int64_t diff_us = system_time_us - camera_time_us;
   // The deviation from the current average.
   int64_t error_us = diff_us - offset_us_;
 
   // If the current difference is far from the currently estimated
   // offset, the filter is reset. This could happen, e.g., if the
   // camera clock is reset, or cameras are plugged in and out, or if
-  // the application process is temporarily suspended. The limit of
-  // 300 ms should make this unlikely in normal operation, and at the
-  // same time, converging gradually rather than resetting the filter
-  // should be tolerable for jumps in camera time below this
-  // threshold.
-  static const int64_t kResetLimitUs = 300000;
-  if (std::abs(error_us) > kResetLimitUs) {
+  // the application process is temporarily suspended. Expected to
+  // happen for the very first timestamp (|frames_seen_| = 0), but
+  // otherwise very rare. The threshold of 300 ms should make this

[noahric, 2016/09/09 23:37:55]

I'd remove the "otherwise very rare"; I wouldn't expect it to happen if the
underlying capturer never changes, but capturer changes oftentimes result in
clock source changes.

[nisse-webrtc, 2016/09/12 06:58:22]

Done.

+  // unlikely in normal operation, and at the same time, converging
+  // gradually rather than resetting the filter should be tolerable
+  // for jumps in camera time below this threshold.
+  static const int64_t kResetThresholdUs = 300000;
+  if (std::abs(error_us) > kResetThresholdUs) {
     LOG(LS_INFO) << "Resetting timestamp translation after averaging "
                  << frames_seen_ << " frames. Old offset: " << offset_us_
                  << ", new offset: " << diff_us;
     frames_seen_ = 0;
-    prev_translated_time_us_ = rtc::Optional<int64_t>();
+    clip_bias_us_ = 0;
   }
 
   static const int kWindowSize = 100;
   if (frames_seen_ < kWindowSize) {
     ++frames_seen_;
   }
   offset_us_ += error_us / frames_seen_;
   return offset_us_;
 }
 
 int64_t TimestampAligner::ClipTimestamp(int64_t time_us,
                                         int64_t system_time_us) {
   // Make timestamps monotonic.
-  if (!prev_translated_time_us_) {
-    // Initialize.
-    clip_bias_us_ = 0;
-  } else if (time_us < *prev_translated_time_us_) {
-    time_us = *prev_translated_time_us_;
+  if (time_us <= prev_translated_time_us_) {
+    time_us = prev_translated_time_us_ + 1;

[noahric, 2016/09/09 23:37:55]

I'd bump it 1ms, just to make sure it's monotonic at lower accuracy.

[nisse-webrtc, 2016/09/12 06:58:22]

This makes sense at first. 

But should we then also change the test to
if (time_us < prev_translated_time_us_ + rtc::kNumMicrosecsPerMillisec)?

So I'm thinking that after all, it may be better to leave to downstream code
which require larger intervals (say, e.g, unique rtp timestamps) do whatever
they need? Relying on larger intervals here seems a bit brittle.

What do you think Per, is it a good idea to increase the minimum frame interval
here, from 1 us to some larger constant? 

My thinking about the current +1 code was not to do some general sanity check on
inter-frame timing, but to make things simpler for code which wants to use the
timestamp as an identifier, and therefore need to reject frames with duplicate
timestamps.

[perkj_webrtc, 2016/09/12 09:33:46]

We regardless need to drop frames with old ts in lower layers since each
capturer runs its own TimeStampAligner. 
But I don't see why we can not have the minimun diff be 1ms regardless.

   }
 
-  // Clip to make sure we don't produce time stamps in the future.
+  // Clip to make sure we don't produce timestamps in the future. In
+  // the anomalous case that this function is called multiple times
+  // with exactly the same system_time_us, we may also output the same
+  // timestamp several times, violating strict monotonicity.

[noahric, 2016/09/09 23:37:55]

Since we can detect that, consider logging when it happens (if time_us <=
prev_translated_time_us after this block runs).

[nisse-webrtc, 2016/09/12 06:58:22]

Done. Added a DCHECK_GE to check that time_us >= prev_translated_us_, and then
another check and log message for equality.

   time_us -= clip_bias_us_;
   if (time_us > system_time_us) {
     clip_bias_us_ += time_us - system_time_us;
     time_us = system_time_us;
   }
-  prev_translated_time_us_ = rtc::Optional<int64_t>(time_us);
+  prev_translated_time_us_ = time_us;
   return time_us;
 }
 
 }  // namespace rtc