From 1b0371a54e601476e660c3d668561782eed16ffc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olov=20Br=C3=A4ndstr=C3=B6m?= Date: Tue, 19 Nov 2024 15:22:15 +0100 Subject: [PATCH] Reduce the moving median window size in Remote ntp time estimator. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Too big median window will cause errors with large clock drifts, since we'll end up using old values for estimated clock drift. If the window is too small, the remote clock offset estimation could be noisy or we could even end up using outliers as the offset estimation. I will not claim that I choose the correct value, and I'm not sure how to measure the quality of the remote clock offset estimations. Bug: webrtc:379809147 Change-Id: Ib317548d3eec74105d468ef53830e12eb114df7d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/368580 Reviewed-by: Danil Chapovalov Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Olov Brändström Cr-Commit-Position: refs/heads/main@{#43439} --- .../source/remote_ntp_time_estimator.cc | 4 ++-- .../remote_ntp_time_estimator_unittest.cc | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/modules/rtp_rtcp/source/remote_ntp_time_estimator.cc b/modules/rtp_rtcp/source/remote_ntp_time_estimator.cc index 21ef24a646..898e5b734d 100644 --- a/modules/rtp_rtcp/source/remote_ntp_time_estimator.cc +++ b/modules/rtp_rtcp/source/remote_ntp_time_estimator.cc @@ -21,9 +21,9 @@ namespace webrtc { namespace { -constexpr int kMinimumNumberOfSamples = 2; +constexpr int kMinimumNumberOfSamples = 3; constexpr TimeDelta kTimingLogInterval = TimeDelta::Seconds(10); -constexpr int kClocksOffsetSmoothingWindow = 100; +constexpr int kClocksOffsetSmoothingWindow = 7; // Subtracts two NtpTime values keeping maximum precision. int64_t Subtract(NtpTime minuend, NtpTime subtrahend) { diff --git a/modules/rtp_rtcp/source/remote_ntp_time_estimator_unittest.cc b/modules/rtp_rtcp/source/remote_ntp_time_estimator_unittest.cc index e9e8ce9fcc..dda58dc7cd 100644 --- a/modules/rtp_rtcp/source/remote_ntp_time_estimator_unittest.cc +++ b/modules/rtp_rtcp/source/remote_ntp_time_estimator_unittest.cc @@ -27,6 +27,9 @@ constexpr Timestamp kRemoteClockInitialTime = Timestamp::Millis(373); constexpr uint32_t kTimestampOffset = 567; constexpr int64_t kRemoteToLocalClockOffsetNtp = ToNtpUnits(kLocalClockInitialTime - kRemoteClockInitialTime); +// There can be small rounding differences when converting to the +// sub nano second precision of the NTP timestamps. +constexpr int64_t kEpsilon = 1; class RemoteNtpTimeEstimatorTest : public ::testing::Test { protected: @@ -85,10 +88,14 @@ TEST_F(RemoteNtpTimeEstimatorTest, Estimate) { // Remote sends second RTCP SR. SendRtcpSr(); + AdvanceTime(TimeDelta::Millis(800)); + // Remote sends third RTCP SR. + SendRtcpSr(); + // Local peer gets enough RTCP SR to calculate the capture time. EXPECT_EQ(capture_ntp_time_ms, estimator_.Estimate(rtp_timestamp)); - EXPECT_EQ(estimator_.EstimateRemoteToLocalClockOffset(), - kRemoteToLocalClockOffsetNtp); + EXPECT_NEAR(*estimator_.EstimateRemoteToLocalClockOffset(), + kRemoteToLocalClockOffsetNtp, kEpsilon); } TEST_F(RemoteNtpTimeEstimatorTest, AveragesErrorsOut) { @@ -103,8 +110,8 @@ TEST_F(RemoteNtpTimeEstimatorTest, AveragesErrorsOut) { int64_t capture_ntp_time_ms = local_clock_.CurrentNtpInMilliseconds(); // Local peer gets enough RTCP SR to calculate the capture time. EXPECT_EQ(capture_ntp_time_ms, estimator_.Estimate(rtp_timestamp)); - EXPECT_EQ(kRemoteToLocalClockOffsetNtp, - estimator_.EstimateRemoteToLocalClockOffset()); + EXPECT_NEAR(kRemoteToLocalClockOffsetNtp, + *estimator_.EstimateRemoteToLocalClockOffset(), kEpsilon); // Remote sends corrupted RTCP SRs AdvanceTime(TimeDelta::Seconds(1)); @@ -121,8 +128,8 @@ TEST_F(RemoteNtpTimeEstimatorTest, AveragesErrorsOut) { // Errors should be averaged out. EXPECT_EQ(capture_ntp_time_ms, estimator_.Estimate(rtp_timestamp)); - EXPECT_EQ(kRemoteToLocalClockOffsetNtp, - estimator_.EstimateRemoteToLocalClockOffset()); + EXPECT_NEAR(kRemoteToLocalClockOffsetNtp, + *estimator_.EstimateRemoteToLocalClockOffset(), kEpsilon); } } // namespace