Bug Fix: WebRTC Receiver Timestamp Jump Detection

webrtc/system_wrappers/source/timestamp_extrapolator.cc

 /*
  *  Copyright (c) 2011 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.
  */
 
@@ skipping 59 matching lines @@
         // Reset the extrapolator
         _rwLock->ReleaseLockExclusive();
         Reset(tMs);
         _rwLock->AcquireLockExclusive();
     }
     else
     {
         _prevMs = tMs;
     }
 
-    // Remove offset to prevent badly scaled matrices
-    tMs -= _startMs;
-
     CheckForWrapArounds(ts90khz);
 
     int64_t unwrapped_ts90khz = static_cast<int64_t>(ts90khz) +
         _wrapArounds * ((static_cast<int64_t>(1) << 32) - 1);
 
     if (_prevUnwrappedTimestamp >= 0 &&
+        std::abs(unwrapped_ts90khz - _prevUnwrappedTimestamp) >
+            90 * 1000 * 10)

[nisse-webrtc, 2017/03/30 06:37:07]

This limit deserves a symbolic name.

And I think it makes sense to log a message when reset happens, since it should
be rare and indicates a problem somewhere in the system.

[stefan-webrtc, 2017/03/30 06:55:14]

I think this will trigger cases where we may not want to reset. For instance if
there was some network interruption for 10 seconds and then it resumed fine. But
maybe it's still fine to reset in that case anyway?

Maybe we should use the existing DelayChangeDetection() method instead and reset
after looking for a consistent offset in timestamps?

[qiangchen, 2017/03/30 17:47:38]

That's a good idea, and I do see the following code (line 109) does it. In that
case, _pP[1][1] is set to a large number, which  essentially means we throw away
the historical sample data, and in turn w will quickly be adjusted to proper
value by the following several updates. 

But line 89 is before this check, which means that if we have a large timestamp
jump back, line 109 can never be triggered for long time, further the
extrapolater stops updating at all, which in turn results in wrong clock time
conversion for a long time.

Do you think it is proper to 
1. remove my current resetting fix, and
2. put the block (ln88~94) after the block (ln106~116)?

+    {
+        // Time stamp jump more than 10s.
+        // We took this as a sign of a different stream, reset the extrapolator.
+        _rwLock->ReleaseLockExclusive();
+        Reset(tMs);
+        _rwLock->AcquireLockExclusive();
+    }
+
+    // Remove offset to prevent badly scaled matrices
+    tMs -= _startMs;
+
+    if (_prevUnwrappedTimestamp >= 0 &&
         unwrapped_ts90khz < _prevUnwrappedTimestamp)
     {
         // Drop reordered frames.
         _rwLock->ReleaseLockExclusive();
         return;
     }
 
     if (_firstAfterReset)
     {
         // Make an initial guess of the offset,
@@ skipping 126 matching lines @@
     if (_detectorAccumulatorPos > _alarmThreshold || _detectorAccumulatorNeg < -_alarmThreshold)
     {
         // Alarm
         _detectorAccumulatorPos = _detectorAccumulatorNeg = 0;
         return true;
     }
     return false;
 }
 
 }