From 10c7d7368847a2bbed1c5a22cfc6391522e2d520 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 2 Dec 2024 21:11:19 +0000 Subject: [PATCH] Fix sign error in UMA for AbsCapture.Delta Bug: webrtc:380712819 Change-Id: Icfb42f0455718058a54391e5a586f409cd28728d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/370000 Commit-Queue: Harald Alvestrand Reviewed-by: Johannes Kron Cr-Commit-Position: refs/heads/main@{#43481} --- .../absolute_capture_time_interpolator.cc | 9 ++-- ...lute_capture_time_interpolator_unittest.cc | 39 ++++++++++++++++ system_wrappers/include/ntp_time.h | 6 +++ system_wrappers/source/ntp_time_unittest.cc | 45 +++++++++++++++++++ 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/modules/rtp_rtcp/source/absolute_capture_time_interpolator.cc b/modules/rtp_rtcp/source/absolute_capture_time_interpolator.cc index b99f387e54..295aa53bc2 100644 --- a/modules/rtp_rtcp/source/absolute_capture_time_interpolator.cc +++ b/modules/rtp_rtcp/source/absolute_capture_time_interpolator.cc @@ -18,6 +18,7 @@ #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "rtc_base/checks.h" +#include "rtc_base/logging.h" #include "rtc_base/synchronization/mutex.h" #include "system_wrappers/include/clock.h" #include "system_wrappers/include/metrics.h" @@ -78,10 +79,10 @@ AbsoluteCaptureTimeInterpolator::OnReceivePacket( (receive_time - *first_packet_time_).ms()); first_extension_time_ = receive_time; } - uint64_t ntp_delta = + int64_t ntp_delta = uint64_t{clock_->ConvertTimestampToNtpTime(receive_time)} - received_extension->absolute_capture_timestamp; - TimeDelta capture_delta = TimeDelta::Micros(UQ32x32ToInt64Us(ntp_delta)); + TimeDelta capture_delta = TimeDelta::Micros(Q32x32ToInt64Us(ntp_delta)); RTC_HISTOGRAM_COUNTS_1G("WebRTC.Call.AbsCapture.Delta", abs(capture_delta.us())); if (previous_capture_delta_) { @@ -96,8 +97,8 @@ AbsoluteCaptureTimeInterpolator::OnReceivePacket( (receive_time - *first_packet_time_).ms()); first_offset_time_ = receive_time; } - TimeDelta offset_as_delta = TimeDelta::Micros(UQ32x32ToInt64Us( - *received_extension->estimated_capture_clock_offset)); + TimeDelta offset_as_delta = TimeDelta::Micros( + Q32x32ToInt64Us(*received_extension->estimated_capture_clock_offset)); RTC_HISTOGRAM_COUNTS_1G("WebRTC.Call.AbsCapture.Offset", abs(offset_as_delta.us())); if (previous_offset_as_delta_) { diff --git a/modules/rtp_rtcp/source/absolute_capture_time_interpolator_unittest.cc b/modules/rtp_rtcp/source/absolute_capture_time_interpolator_unittest.cc index 140db51f9e..2e1e05447b 100644 --- a/modules/rtp_rtcp/source/absolute_capture_time_interpolator_unittest.cc +++ b/modules/rtp_rtcp/source/absolute_capture_time_interpolator_unittest.cc @@ -18,10 +18,15 @@ #include "system_wrappers/include/clock.h" #include "system_wrappers/include/metrics.h" #include "system_wrappers/include/ntp_time.h" +#include "test/gmock.h" #include "test/gtest.h" namespace webrtc { +using testing::AllOf; +using testing::Ge; +using testing::Le; + TEST(AbsoluteCaptureTimeInterpolatorTest, GetSourceWithoutCsrcs) { constexpr uint32_t kSsrc = 12; @@ -380,4 +385,38 @@ TEST(AbsoluteCaptureTimeInterpolatorTest, MetricsAreUpdated) { 1); } +TEST(AbsoluteCaptureTimeInterpolatorTest, DeltaRecordedCorrectly) { + constexpr uint32_t kRtpTimestamp0 = 102030000; + constexpr uint32_t kSource = 1234; + constexpr uint32_t kFrequency = 1000; + SimulatedClock clock(0); + AbsoluteCaptureTimeInterpolator interpolator(&clock); + + metrics::Reset(); + clock.AdvanceTimeMilliseconds(10); + // Packet has extension, with delta 5 ms in the past. + interpolator.OnReceivePacket( + kSource, kRtpTimestamp0 + 10, kFrequency, + AbsoluteCaptureTime{ + uint64_t{clock.ConvertTimestampToNtpTime(Timestamp::Millis(5))}, + std::nullopt}); + + EXPECT_METRIC_EQ(metrics::NumSamples("WebRTC.Call.AbsCapture.ExtensionWait"), + 1); + int sample = metrics::MinSample("WebRTC.Call.AbsCapture.Delta"); + EXPECT_THAT(sample, AllOf(Ge(5000), Le(5000))); + + metrics::Reset(); + // Packet has extension, with timestamp 6 ms in the future. + interpolator.OnReceivePacket( + kSource, kRtpTimestamp0 + 15, kFrequency, + AbsoluteCaptureTime{ + uint64_t{clock.ConvertTimestampToNtpTime(Timestamp::Millis(16))}, + std::nullopt}); + + sample = metrics::MinSample("WebRTC.Call.AbsCapture.Delta"); + // Since we capture with abs(), this should also be recorded as 6 ms + EXPECT_THAT(sample, AllOf(Ge(6000), Le(6000))); +} + } // namespace webrtc diff --git a/system_wrappers/include/ntp_time.h b/system_wrappers/include/ntp_time.h index 2edfd4a0d8..25f622985a 100644 --- a/system_wrappers/include/ntp_time.h +++ b/system_wrappers/include/ntp_time.h @@ -126,5 +126,11 @@ inline int64_t UQ32x32ToInt64Us(uint64_t q32x32) { std::round(q32x32 * (1'000'000.0 / NtpTime::kFractionsPerSecond))); } +// Converts Q32.32-formatted fixed-point seconds to `int64_t` microseconds. +inline int64_t Q32x32ToInt64Us(int64_t q32x32) { + return rtc::dchecked_cast( + std::round(q32x32 * (1'000'000.0 / NtpTime::kFractionsPerSecond))); +} + } // namespace webrtc #endif // SYSTEM_WRAPPERS_INCLUDE_NTP_TIME_H_ diff --git a/system_wrappers/source/ntp_time_unittest.cc b/system_wrappers/source/ntp_time_unittest.cc index 0705531e37..40027a3660 100644 --- a/system_wrappers/source/ntp_time_unittest.cc +++ b/system_wrappers/source/ntp_time_unittest.cc @@ -13,11 +13,14 @@ #include #include "system_wrappers/include/clock.h" +#include "test/gmock.h" #include "test/gtest.h" namespace webrtc { namespace { +using testing::Lt; + constexpr uint32_t kNtpSec = 0x12345678; constexpr uint32_t kNtpFrac = 0x23456789; @@ -276,5 +279,47 @@ TEST(NtpTimeTest, VerifyInt64MsToUQ32x32RoundTrip) { } } +TEST(NtpTimeTest, VerifyInt64MsToUQ32x32UsRoundTrip) { + constexpr int kIterations = 50000; + + std::mt19937 generator(123456789); + std::uniform_int_distribution distribution( + UQ32x32ToInt64Ms(std::numeric_limits::min()), + UQ32x32ToInt64Ms(std::numeric_limits::max())); + + for (int iteration = 0; iteration < kIterations; ++iteration) { + uint64_t input_ms = distribution(generator); + uint64_t transit_uq32x32 = Int64MsToUQ32x32(input_ms); + uint64_t output_ms = UQ32x32ToInt64Us(transit_uq32x32); + + int64_t difference = input_ms * 1000 - output_ms; + ASSERT_THAT(abs(difference), Lt(2)) + << "iteration = " << iteration << ", input_ms = " << input_ms + << ", transit_uq32x32 = " << transit_uq32x32 + << ", output_us = " << output_ms; + } +} + +TEST(NtpTimeTest, VerifySignedInt64MsToUQ32x32UsRoundTrip) { + constexpr int kIterations = 50000; + + std::mt19937 generator(123456789); + std::uniform_int_distribution distribution( + UQ32x32ToInt64Ms(std::numeric_limits::min()), + UQ32x32ToInt64Ms(std::numeric_limits::max())); + + for (int iteration = 0; iteration < kIterations; ++iteration) { + int64_t input_ms = distribution(generator); + uint64_t transit_uq32x32 = Int64MsToUQ32x32(input_ms); + int64_t output_ms = UQ32x32ToInt64Us(transit_uq32x32); + + int64_t difference = input_ms * 1000 - output_ms; + ASSERT_THAT(abs(difference), Lt(2)) + << "iteration = " << iteration << ", input_ms = " << input_ms + << ", transit_uq32x32 = " << transit_uq32x32 + << ", output_us = " << output_ms; + } +} + } // namespace } // namespace webrtc