From 5cf8c2c5010472de3b360d3b45f1b80644899290 Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Wed, 24 Mar 2021 08:51:26 +0100 Subject: [PATCH] Fix unspecified time origin for `lastPacketReceivedTimestamp` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RTCInboundRtpStreamStats.lastPacketReceivedTimestamp` must be a time value in milliseconds with Unix epoch as time origin (see bugs.webrtc.org/12605#c4). This change fixes both audio and video `RTCInboundRtpStreamStats` stats. Tested: verified from chrome://webrtc-internals during an appr.tc call Bug: webrtc:12605 Change-Id: I68157fcf01a5933f3d4e5d3918b4a9d3fbd64f16 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212865 Reviewed-by: Henrik Boström Reviewed-by: Danil Chapovalov Commit-Queue: Alessio Bazzica Cr-Commit-Position: refs/heads/master@{#33547} --- .../source/receive_statistics_impl.cc | 19 +++++++++++++++---- .../rtp_rtcp/source/receive_statistics_impl.h | 2 ++ pc/rtc_stats_collector.cc | 12 ++++++------ pc/rtc_stats_collector_unittest.cc | 2 +- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index 4c399a107e..26c8cdd8c7 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -24,9 +24,14 @@ #include "system_wrappers/include/clock.h" namespace webrtc { +namespace { +constexpr int64_t kStatisticsTimeoutMs = 8000; +constexpr int64_t kStatisticsProcessIntervalMs = 1000; -const int64_t kStatisticsTimeoutMs = 8000; -const int64_t kStatisticsProcessIntervalMs = 1000; +// Number of seconds since 1900 January 1 00:00 GMT (see +// https://tools.ietf.org/html/rfc868). +constexpr int64_t kNtpJan1970Millisecs = 2'208'988'800'000; +} // namespace StreamStatistician::~StreamStatistician() {} @@ -35,6 +40,9 @@ StreamStatisticianImpl::StreamStatisticianImpl(uint32_t ssrc, int max_reordering_threshold) : ssrc_(ssrc), clock_(clock), + delta_internal_unix_epoch_ms_(clock_->CurrentNtpInMilliseconds() - + clock_->TimeInMilliseconds() - + kNtpJan1970Millisecs), incoming_bitrate_(kStatisticsProcessIntervalMs, RateStatistics::kBpsScale), max_reordering_threshold_(max_reordering_threshold), @@ -172,8 +180,11 @@ RtpReceiveStats StreamStatisticianImpl::GetStats() const { // TODO(nisse): Can we return a float instead? // Note: internal jitter value is in Q4 and needs to be scaled by 1/16. stats.jitter = jitter_q4_ >> 4; - stats.last_packet_received_timestamp_ms = - receive_counters_.last_packet_received_timestamp_ms; + if (receive_counters_.last_packet_received_timestamp_ms.has_value()) { + stats.last_packet_received_timestamp_ms = + *receive_counters_.last_packet_received_timestamp_ms + + delta_internal_unix_epoch_ms_; + } stats.packet_counter = receive_counters_.transmitted; return stats; } diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index 2456f93e9a..be56f4ba5a 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -73,6 +73,8 @@ class StreamStatisticianImpl : public StreamStatisticianImplInterface { const uint32_t ssrc_; Clock* const clock_; + // Delta used to map internal timestamps to Unix epoch ones. + const int64_t delta_internal_unix_epoch_ms_; RateStatistics incoming_bitrate_; // In number of packets or sequence numbers. int max_reordering_threshold_; diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 36ee5425b1..4c9dfa489e 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -372,6 +372,7 @@ std::unique_ptr CreateInboundAudioStreamStats( *voice_receiver_info.last_packet_received_timestamp_ms); } if (voice_receiver_info.estimated_playout_ntp_timestamp_ms) { + // TODO(bugs.webrtc.org/10529): Fix time origin. inbound_audio->estimated_playout_timestamp = static_cast( *voice_receiver_info.estimated_playout_ntp_timestamp_ms); } @@ -471,17 +472,16 @@ void SetInboundRTPStreamStatsFromVideoReceiverInfo( inbound_video->total_squared_inter_frame_delay = video_receiver_info.total_squared_inter_frame_delay; if (video_receiver_info.last_packet_received_timestamp_ms) { - inbound_video->last_packet_received_timestamp = - static_cast( - *video_receiver_info.last_packet_received_timestamp_ms) / - rtc::kNumMillisecsPerSec; + inbound_video->last_packet_received_timestamp = static_cast( + *video_receiver_info.last_packet_received_timestamp_ms); } if (video_receiver_info.estimated_playout_ntp_timestamp_ms) { + // TODO(bugs.webrtc.org/10529): Fix time origin if needed. inbound_video->estimated_playout_timestamp = static_cast( *video_receiver_info.estimated_playout_ntp_timestamp_ms); } - // TODO(https://crbug.com/webrtc/10529): When info's |content_info| is - // optional, support the "unspecified" value. + // TODO(bugs.webrtc.org/10529): When info's |content_info| is optional + // support the "unspecified" value. if (video_receiver_info.content_type == VideoContentType::SCREENSHARE) inbound_video->content_type = RTCContentType::kScreenshare; if (!video_receiver_info.decoder_implementation_name.empty()) { diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 897226d25e..655f7e6315 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -2107,7 +2107,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Video) { video_media_info.receivers[0].qp_sum = 9; expected_video.qp_sum = 9; video_media_info.receivers[0].last_packet_received_timestamp_ms = 1000; - expected_video.last_packet_received_timestamp = 1.0; + expected_video.last_packet_received_timestamp = 1000.0; video_media_info.receivers[0].content_type = VideoContentType::SCREENSHARE; expected_video.content_type = "screenshare"; video_media_info.receivers[0].estimated_playout_ntp_timestamp_ms = 1234;