From 0f43da224814d702d4b52474d58005a184d15736 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 28 Feb 2023 17:03:21 +0100 Subject: [PATCH] Cleanup RtcpReceiver::NTP function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace it with GetSenderReportStats that returns result instead of filling lots of output parameters On the way replace pair of uint32_t with dedicated NtpTime type Bug: None Change-Id: I5b821b8d000d63ebd8cdc1b9897a86429d97b19b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295560 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#39431} --- modules/rtp_rtcp/source/rtcp_receiver.cc | 62 ++++--------------- modules/rtp_rtcp/source/rtcp_receiver.h | 28 ++------- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 9 +-- modules/rtp_rtcp/source/rtcp_sender.cc | 10 +-- modules/rtp_rtcp/source/rtcp_sender.h | 4 +- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 36 +++-------- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 35 +++-------- modules/rtp_rtcp/source/rtp_rtcp_interface.h | 8 +-- 8 files changed, 44 insertions(+), 148 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 0a24481762..9b4cf183e8 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -156,10 +156,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, : kDefaultVideoReportInterval)), // TODO(bugs.webrtc.org/10774): Remove fallback. remote_ssrc_(0), - remote_sender_rtp_time_(0), - remote_sender_packet_count_(0), - remote_sender_octet_count_(0), - remote_sender_reports_count_(0), xr_rrtr_status_(config.non_sender_rtt_measurement), xr_rr_rtt_ms_(0), oldest_tmmbr_info_ms_(0), @@ -189,10 +185,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, : kDefaultVideoReportInterval)), // TODO(bugs.webrtc.org/10774): Remove fallback. remote_ssrc_(0), - remote_sender_rtp_time_(0), - remote_sender_packet_count_(0), - remote_sender_octet_count_(0), - remote_sender_reports_count_(0), xr_rrtr_status_(config.non_sender_rtt_measurement), xr_rr_rtt_ms_(0), oldest_tmmbr_info_ms_(0), @@ -241,7 +233,7 @@ int64_t RTCPReceiver::LastReceivedReportBlockMs() const { void RTCPReceiver::SetRemoteSSRC(uint32_t ssrc) { MutexLock lock(&rtcp_receiver_lock_); // New SSRC reset old reports. - last_received_sr_ntp_.Reset(); + remote_sender_.last_arrival_timestamp.Reset(); remote_ssrc_ = ssrc; } @@ -368,42 +360,14 @@ absl::optional RTCPReceiver::OnPeriodicRttUpdate( return rtt; } -bool RTCPReceiver::NTP(uint32_t* received_ntp_secs, - uint32_t* received_ntp_frac, - uint32_t* rtcp_arrival_time_secs, - uint32_t* rtcp_arrival_time_frac, - uint32_t* rtcp_timestamp, - uint32_t* remote_sender_packet_count, - uint64_t* remote_sender_octet_count, - uint64_t* remote_sender_reports_count) const { +absl::optional +RTCPReceiver::GetSenderReportStats() const { MutexLock lock(&rtcp_receiver_lock_); - if (!last_received_sr_ntp_.Valid()) - return false; + if (!remote_sender_.last_arrival_timestamp.Valid()) { + return absl::nullopt; + } - // NTP from incoming SenderReport. - if (received_ntp_secs) - *received_ntp_secs = remote_sender_ntp_time_.seconds(); - if (received_ntp_frac) - *received_ntp_frac = remote_sender_ntp_time_.fractions(); - // Rtp time from incoming SenderReport. - if (rtcp_timestamp) - *rtcp_timestamp = remote_sender_rtp_time_; - - // Local NTP time when we received a RTCP packet with a send block. - if (rtcp_arrival_time_secs) - *rtcp_arrival_time_secs = last_received_sr_ntp_.seconds(); - if (rtcp_arrival_time_frac) - *rtcp_arrival_time_frac = last_received_sr_ntp_.fractions(); - - // Counters. - if (remote_sender_packet_count) - *remote_sender_packet_count = remote_sender_packet_count_; - if (remote_sender_octet_count) - *remote_sender_octet_count = remote_sender_octet_count_; - if (remote_sender_reports_count) - *remote_sender_reports_count = remote_sender_reports_count_; - - return true; + return remote_sender_; } std::vector @@ -584,12 +548,12 @@ void RTCPReceiver::HandleSenderReport(const CommonHeader& rtcp_block, // Only signal that we have received a SR when we accept one. packet_information->packet_type_flags |= kRtcpSr; - remote_sender_ntp_time_ = sender_report.ntp(); - remote_sender_rtp_time_ = sender_report.rtp_timestamp(); - last_received_sr_ntp_ = clock_->CurrentNtpTime(); - remote_sender_packet_count_ = sender_report.sender_packet_count(); - remote_sender_octet_count_ = sender_report.sender_octet_count(); - remote_sender_reports_count_++; + remote_sender_.last_remote_timestamp = sender_report.ntp(); + remote_sender_.last_remote_rtp_timestamp = sender_report.rtp_timestamp(); + remote_sender_.last_arrival_timestamp = clock_->CurrentNtpTime(); + remote_sender_.packets_sent = sender_report.sender_packet_count(); + remote_sender_.bytes_sent = sender_report.sender_octet_count(); + remote_sender_.reports_count++; } else { // We will only store the send report from one source, but // we will store all the receive blocks. diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index d3c07929dd..5007b42764 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -110,22 +110,9 @@ class RTCPReceiver final { bool receiver_only() const { return receiver_only_; } - // Get received NTP. - // The types for the arguments below derive from the specification: - // - `remote_sender_packet_count`: `RTCSentRtpStreamStats.packetsSent` [1] - // - `remote_sender_octet_count`: `RTCSentRtpStreamStats.bytesSent` [1] - // - `remote_sender_reports_count`: - // `RTCRemoteOutboundRtpStreamStats.reportsSent` [2] - // [1] https://www.w3.org/TR/webrtc-stats/#remoteoutboundrtpstats-dict* - // [2] https://www.w3.org/TR/webrtc-stats/#dom-rtcsentrtpstreamstats - bool NTP(uint32_t* received_ntp_secs, - uint32_t* received_ntp_frac, - uint32_t* rtcp_arrival_time_secs, - uint32_t* rtcp_arrival_time_frac, - uint32_t* rtcp_timestamp, - uint32_t* remote_sender_packet_count, - uint64_t* remote_sender_octet_count, - uint64_t* remote_sender_reports_count) const; + // Returns stats based on the received RTCP Sender Reports. + absl::optional GetSenderReportStats() + const; std::vector ConsumeReceivedXrReferenceTimeInfo(); @@ -388,13 +375,8 @@ class RTCPReceiver final { uint32_t remote_ssrc_ RTC_GUARDED_BY(rtcp_receiver_lock_); // Received sender report. - NtpTime remote_sender_ntp_time_ RTC_GUARDED_BY(rtcp_receiver_lock_); - uint32_t remote_sender_rtp_time_ RTC_GUARDED_BY(rtcp_receiver_lock_); - // When did we receive the last send report. - NtpTime last_received_sr_ntp_ RTC_GUARDED_BY(rtcp_receiver_lock_); - uint32_t remote_sender_packet_count_ RTC_GUARDED_BY(rtcp_receiver_lock_); - uint64_t remote_sender_octet_count_ RTC_GUARDED_BY(rtcp_receiver_lock_); - uint64_t remote_sender_reports_count_ RTC_GUARDED_BY(rtcp_receiver_lock_); + RtpRtcpInterface::SenderReportStats remote_sender_ + RTC_GUARDED_BY(rtcp_receiver_lock_); // Received RRTR information in ascending receive time order. std::list received_rrtrs_ diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index ff39ccca9c..b366731fcd 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -202,8 +202,7 @@ TEST(RtcpReceiverTest, InjectSrPacket) { RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); - EXPECT_FALSE(receiver.NTP(nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + EXPECT_FALSE(receiver.GetSenderReportStats()); int64_t now = mocks.clock.TimeInMilliseconds(); rtcp::SenderReport sr; @@ -214,8 +213,7 @@ TEST(RtcpReceiverTest, InjectSrPacket) { OnReceivedRtcpReceiverReport(IsEmpty(), _, now)); receiver.IncomingPacket(sr.Build()); - EXPECT_TRUE(receiver.NTP(nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr)); + EXPECT_TRUE(receiver.GetSenderReportStats()); } TEST(RtcpReceiverTest, InjectSrPacketFromUnknownSender) { @@ -235,8 +233,7 @@ TEST(RtcpReceiverTest, InjectSrPacketFromUnknownSender) { receiver.IncomingPacket(sr.Build()); // But will not flag that he's gotten sender information. - EXPECT_FALSE(receiver.NTP(nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + EXPECT_FALSE(receiver.GetSenderReportStats()); } TEST(RtcpReceiverTest, InjectSrPacketCalculatesRTT) { diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index 7983371097..1f58a24c04 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -92,8 +92,6 @@ RTCPSender::FeedbackState::FeedbackState() : packets_sent(0), media_bytes_sent(0), send_bitrate(0), - last_rr_ntp_secs(0), - last_rr_ntp_frac(0), remote_sr(0), receiver(nullptr) {} @@ -823,14 +821,10 @@ std::vector RTCPSender::CreateReportBlocks( // streams. result = receive_statistics_->RtcpReportBlocks(RTCP_MAX_REPORT_BLOCKS); - if (!result.empty() && ((feedback_state.last_rr_ntp_secs != 0) || - (feedback_state.last_rr_ntp_frac != 0))) { + if (!result.empty() && feedback_state.last_rr.Valid()) { // Get our NTP as late as possible to avoid a race. uint32_t now = CompactNtp(clock_->CurrentNtpTime()); - - uint32_t receive_time = feedback_state.last_rr_ntp_secs & 0x0000FFFF; - receive_time <<= 16; - receive_time += (feedback_state.last_rr_ntp_frac & 0xffff0000) >> 16; + uint32_t receive_time = CompactNtp(feedback_state.last_rr); uint32_t delay_since_last_sr = now - receive_time; // TODO(danilchap): Instead of setting same value on all report blocks, diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 8f51e7947d..6dae9dbd61 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -37,6 +37,7 @@ #include "rtc_base/random.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" +#include "system_wrappers/include/ntp_time.h" namespace webrtc { @@ -93,9 +94,8 @@ class RTCPSender final { size_t media_bytes_sent; uint32_t send_bitrate; - uint32_t last_rr_ntp_secs; - uint32_t last_rr_ntp_frac; uint32_t remote_sr; + NtpTime last_rr; std::vector last_xr_rtis; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 03d0e31ddb..c3e5e93223 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -20,11 +20,13 @@ #include #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "api/transport/field_trial_based_config.h" #include "modules/rtp_rtcp/source/rtcp_packet/dlrr.h" #include "modules/rtp_rtcp/source/rtcp_sender.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" #include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" +#include "modules/rtp_rtcp/source/time_util.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "system_wrappers/include/ntp_time.h" @@ -282,18 +284,11 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl::GetFeedbackState() { } state.receiver = &rtcp_receiver_; - uint32_t received_ntp_secs = 0; - uint32_t received_ntp_frac = 0; - state.remote_sr = 0; - if (rtcp_receiver_.NTP(&received_ntp_secs, &received_ntp_frac, - /*rtcp_arrival_time_secs=*/&state.last_rr_ntp_secs, - /*rtcp_arrival_time_frac=*/&state.last_rr_ntp_frac, - /*rtcp_timestamp=*/nullptr, - /*remote_sender_packet_count=*/nullptr, - /*remote_sender_octet_count=*/nullptr, - /*remote_sender_reports_count=*/nullptr)) { - state.remote_sr = ((received_ntp_secs & 0x0000ffff) << 16) + - ((received_ntp_frac & 0xffff0000) >> 16); + if (absl::optional last_sr = + rtcp_receiver_.GetSenderReportStats(); + last_sr.has_value()) { + state.remote_sr = CompactNtp(last_sr->last_remote_timestamp); + state.last_rr = last_sr->last_arrival_timestamp; } state.last_xr_rtis = rtcp_receiver_.ConsumeReceivedXrReferenceTimeInfo(); @@ -519,22 +514,7 @@ std::vector ModuleRtpRtcpImpl::GetLatestReportBlockData() absl::optional ModuleRtpRtcpImpl::GetSenderReportStats() const { - SenderReportStats stats; - uint32_t remote_timestamp_secs; - uint32_t remote_timestamp_frac; - uint32_t arrival_timestamp_secs; - uint32_t arrival_timestamp_frac; - if (rtcp_receiver_.NTP(&remote_timestamp_secs, &remote_timestamp_frac, - &arrival_timestamp_secs, &arrival_timestamp_frac, - &stats.last_remote_rtp_timestamp, &stats.packets_sent, - &stats.bytes_sent, &stats.reports_count)) { - stats.last_remote_timestamp.Set(remote_timestamp_secs, - remote_timestamp_frac); - stats.last_arrival_timestamp.Set(arrival_timestamp_secs, - arrival_timestamp_frac); - return stats; - } - return absl::nullopt; + return rtcp_receiver_.GetSenderReportStats(); } absl::optional diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 8183e4a4e2..bbcd5d83b1 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -27,6 +27,7 @@ #include "api/units/timestamp.h" #include "modules/rtp_rtcp/source/rtcp_packet/dlrr.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" +#include "modules/rtp_rtcp/source/time_util.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/time_utils.h" @@ -253,18 +254,11 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl2::GetFeedbackState() { } state.receiver = &rtcp_receiver_; - uint32_t received_ntp_secs = 0; - uint32_t received_ntp_frac = 0; - state.remote_sr = 0; - if (rtcp_receiver_.NTP(&received_ntp_secs, &received_ntp_frac, - /*rtcp_arrival_time_secs=*/&state.last_rr_ntp_secs, - /*rtcp_arrival_time_frac=*/&state.last_rr_ntp_frac, - /*rtcp_timestamp=*/nullptr, - /*remote_sender_packet_count=*/nullptr, - /*remote_sender_octet_count=*/nullptr, - /*remote_sender_reports_count=*/nullptr)) { - state.remote_sr = ((received_ntp_secs & 0x0000ffff) << 16) + - ((received_ntp_frac & 0xffff0000) >> 16); + if (absl::optional last_sr = + rtcp_receiver_.GetSenderReportStats(); + last_sr.has_value()) { + state.remote_sr = CompactNtp(last_sr->last_remote_timestamp); + state.last_rr = last_sr->last_arrival_timestamp; } state.last_xr_rtis = rtcp_receiver_.ConsumeReceivedXrReferenceTimeInfo(); @@ -500,22 +494,7 @@ std::vector ModuleRtpRtcpImpl2::GetLatestReportBlockData() absl::optional ModuleRtpRtcpImpl2::GetSenderReportStats() const { - SenderReportStats stats; - uint32_t remote_timestamp_secs; - uint32_t remote_timestamp_frac; - uint32_t arrival_timestamp_secs; - uint32_t arrival_timestamp_frac; - if (rtcp_receiver_.NTP(&remote_timestamp_secs, &remote_timestamp_frac, - &arrival_timestamp_secs, &arrival_timestamp_frac, - &stats.last_remote_rtp_timestamp, &stats.packets_sent, - &stats.bytes_sent, &stats.reports_count)) { - stats.last_remote_timestamp.Set(remote_timestamp_secs, - remote_timestamp_frac); - stats.last_arrival_timestamp.Set(arrival_timestamp_secs, - arrival_timestamp_frac); - return stats; - } - return absl::nullopt; + return rtcp_receiver_.GetSenderReportStats(); } absl::optional diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index c1ec671c9d..d49051be63 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -159,19 +159,19 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { // Received (a.k.a., remote) NTP timestamp for the last received RTCP SR. NtpTime last_remote_timestamp; // Received (a.k.a., remote) RTP timestamp from the last received RTCP SR. - uint32_t last_remote_rtp_timestamp; + uint32_t last_remote_rtp_timestamp = 0; // Total number of RTP data packets transmitted by the sender since starting // transmission up until the time this SR packet was generated. The count // should be reset if the sender changes its SSRC identifier. - uint32_t packets_sent; + uint32_t packets_sent = 0; // Total number of payload octets (i.e., not including header or padding) // transmitted in RTP data packets by the sender since starting transmission // up until the time this SR packet was generated. The count should be reset // if the sender changes its SSRC identifier. - uint64_t bytes_sent; + uint64_t bytes_sent = 0; // Total number of RTCP SR blocks received. // https://www.w3.org/TR/webrtc-stats/#dom-rtcremoteoutboundrtpstreamstats-reportssent. - uint64_t reports_count; + uint64_t reports_count = 0; }; // Stats about the non-sender SSRC, based on RTCP extended reports (XR). // Refer to https://datatracker.ietf.org/doc/html/rfc3611#section-2.