From 510c94cbfb68f5af58d7a6640cedf5c58ae41c4b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 2 Jul 2021 13:51:19 +0200 Subject: [PATCH] Return one report block per media ssrc, ignoring sender ssrc. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Webrtc designed to work for point-to-point topology, and thus each rtcp_receiver handles single remote sender. While remote sender ssrc may change, it should be ok to assume the remote endpoint is still the same. Bug: webrtc:12798 Change-Id: I62aebe7ac802306fc7fa17d7bf3959d6d4cca023 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/224548 Reviewed-by: Henrik Boström Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#34407} --- modules/rtp_rtcp/source/rtcp_receiver.cc | 98 ++++++++++--------- modules/rtp_rtcp/source/rtcp_receiver.h | 43 +++++--- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 11 +-- 3 files changed, 85 insertions(+), 67 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 79f24c4779..ca61ba6359 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -260,6 +260,18 @@ uint32_t RTCPReceiver::RemoteSSRC() const { return remote_ssrc_; } +void RTCPReceiver::RttStats::AddRtt(TimeDelta rtt) { + last_rtt_ = rtt; + if (rtt < min_rtt_) { + min_rtt_ = rtt; + } + if (rtt > max_rtt_) { + max_rtt_ = rtt; + } + sum_rtt_ += rtt; + ++num_rtts_; +} + int32_t RTCPReceiver::RTT(uint32_t remote_ssrc, int64_t* last_rtt_ms, int64_t* avg_rtt_ms, @@ -267,32 +279,26 @@ int32_t RTCPReceiver::RTT(uint32_t remote_ssrc, int64_t* max_rtt_ms) const { MutexLock lock(&rtcp_receiver_lock_); - auto it = received_report_blocks_.find(main_ssrc_); - if (it == received_report_blocks_.end()) + auto it = rtts_.find(remote_ssrc); + if (it == rtts_.end()) { return -1; - - auto it_info = it->second.find(remote_ssrc); - if (it_info == it->second.end()) - return -1; - - const ReportBlockData* report_block_data = &it_info->second; - - if (report_block_data->num_rtts() == 0) - return -1; - - if (last_rtt_ms) - *last_rtt_ms = report_block_data->last_rtt_ms(); - - if (avg_rtt_ms) { - *avg_rtt_ms = - report_block_data->sum_rtt_ms() / report_block_data->num_rtts(); } - if (min_rtt_ms) - *min_rtt_ms = report_block_data->min_rtt_ms(); + if (last_rtt_ms) { + *last_rtt_ms = it->second.last_rtt().ms(); + } - if (max_rtt_ms) - *max_rtt_ms = report_block_data->max_rtt_ms(); + if (avg_rtt_ms) { + *avg_rtt_ms = it->second.average_rtt().ms(); + } + + if (min_rtt_ms) { + *min_rtt_ms = it->second.min_rtt().ms(); + } + + if (max_rtt_ms) { + *max_rtt_ms = it->second.max_rtt().ms(); + } return 0; } @@ -320,26 +326,14 @@ absl::optional RTCPReceiver::OnPeriodicRttUpdate( // amount of time. MutexLock lock(&rtcp_receiver_lock_); if (last_received_rb_.IsInfinite() || last_received_rb_ > newer_than) { - // Stow away the report block for the main ssrc. We'll use the associated - // data map to look up each sender and check the last_rtt_ms(). - auto main_report_it = received_report_blocks_.find(main_ssrc_); - if (main_report_it != received_report_blocks_.end()) { - const ReportBlockDataMap& main_data_map = main_report_it->second; - int64_t max_rtt = 0; - for (const auto& reports_per_receiver : received_report_blocks_) { - for (const auto& report : reports_per_receiver.second) { - const RTCPReportBlock& block = report.second.report_block(); - auto it_info = main_data_map.find(block.sender_ssrc); - if (it_info != main_data_map.end()) { - const ReportBlockData* report_block_data = &it_info->second; - if (report_block_data->num_rtts() > 0) { - max_rtt = std::max(report_block_data->last_rtt_ms(), max_rtt); - } - } - } + TimeDelta max_rtt = TimeDelta::MinusInfinity(); + for (const auto& rtt_stats : rtts_) { + if (rtt_stats.second.last_rtt() > max_rtt) { + max_rtt = rtt_stats.second.last_rtt(); } - if (max_rtt) - rtt.emplace(TimeDelta::Millis(max_rtt)); + } + if (max_rtt.IsFinite()) { + rtt = max_rtt; } } @@ -425,9 +419,9 @@ RTCPReceiver::ConsumeReceivedXrReferenceTimeInfo() { std::vector RTCPReceiver::GetLatestReportBlockData() const { std::vector result; MutexLock lock(&rtcp_receiver_lock_); - for (const auto& reports_per_receiver : received_report_blocks_) - for (const auto& report : reports_per_receiver.second) - result.push_back(report.second); + for (const auto& report : received_report_blocks_) { + result.push_back(report.second); + } return result; } @@ -610,7 +604,7 @@ void RTCPReceiver::HandleReportBlock(const ReportBlock& report_block, last_received_rb_ = clock_->CurrentTime(); ReportBlockData* report_block_data = - &received_report_blocks_[report_block.source_ssrc()][remote_ssrc]; + &received_report_blocks_[report_block.source_ssrc()]; RTCPReportBlock rtcp_report_block; rtcp_report_block.sender_ssrc = remote_ssrc; rtcp_report_block.source_ssrc = report_block.source_ssrc(); @@ -654,6 +648,9 @@ void RTCPReceiver::HandleReportBlock(const ReportBlock& report_block, // Convert to 1/1000 seconds (milliseconds). rtt_ms = CompactNtpRttToMs(rtt_ntp); report_block_data->AddRoundTripTimeSample(rtt_ms); + if (report_block.source_ssrc() == main_ssrc_) { + rtts_[remote_ssrc].AddRtt(TimeDelta::Millis(rtt_ms)); + } packet_information->rtt_ms = rtt_ms; } @@ -811,8 +808,15 @@ void RTCPReceiver::HandleBye(const CommonHeader& rtcp_block) { } // Clear our lists. - for (auto& reports_per_receiver : received_report_blocks_) - reports_per_receiver.second.erase(bye.sender_ssrc()); + rtts_.erase(bye.sender_ssrc()); + for (auto it = received_report_blocks_.begin(); + it != received_report_blocks_.end();) { + if (it->second.report_block().sender_ssrc == bye.sender_ssrc()) { + received_report_blocks_.erase(it++); + } else { + ++it; + } + } TmmbrInformation* tmmbr_info = GetTmmbrInformation(bye.sender_ssrc()); if (tmmbr_info) diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index 57dd1c06b4..62c6e01417 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -117,9 +117,8 @@ class RTCPReceiver final { bool sending); // A snapshot of Report Blocks with additional data of interest to statistics. - // Within this list, the sender-source SSRC pair is unique and per-pair the - // ReportBlockData represents the latest Report Block that was received for - // that pair. + // Within this list, the source SSRC is unique and ReportBlockData represents + // the latest Report Block that was received for that SSRC. std::vector GetLatestReportBlockData() const; // Returns true if we haven't received an RTCP RR for several RTCP @@ -225,14 +224,26 @@ class RTCPReceiver final { uint8_t sequence_number; }; - // TODO(boivie): `ReportBlockDataMap` and `ReportBlockMap` should be converted - // to std::unordered_map, but as there are too many tests that assume a - // specific order, it's not easily done. + class RttStats { + public: + RttStats() = default; + RttStats(const RttStats&) = default; + RttStats& operator=(const RttStats&) = default; - // RTCP report blocks mapped by remote SSRC. - using ReportBlockDataMap = std::map; - // RTCP report blocks map mapped by source SSRC. - using ReportBlockMap = std::map; + void AddRtt(TimeDelta rtt); + + TimeDelta last_rtt() const { return last_rtt_; } + TimeDelta min_rtt() const { return min_rtt_; } + TimeDelta max_rtt() const { return max_rtt_; } + TimeDelta average_rtt() const { return sum_rtt_ / num_rtts_; } + + private: + TimeDelta last_rtt_ = TimeDelta::Zero(); + TimeDelta min_rtt_ = TimeDelta::PlusInfinity(); + TimeDelta max_rtt_ = TimeDelta::MinusInfinity(); + TimeDelta sum_rtt_ = TimeDelta::Zero(); + size_t num_rtts_ = 0; + }; bool ParseCompoundPacket(rtc::ArrayView packet, PacketInformation* packet_information); @@ -369,7 +380,17 @@ class RTCPReceiver final { std::unordered_map tmmbr_infos_ RTC_GUARDED_BY(rtcp_receiver_lock_); - ReportBlockMap received_report_blocks_ RTC_GUARDED_BY(rtcp_receiver_lock_); + // Round-Trip Time per remote sender ssrc. + std::unordered_map rtts_ + RTC_GUARDED_BY(rtcp_receiver_lock_); + + // TODO(boivie): `received_report_blocks_` should be converted + // to std::unordered_map, but as there are too many tests that assume a + // specific order, it's not easily done. + + // Report blocks per local source ssrc. + std::map received_report_blocks_ + RTC_GUARDED_BY(rtcp_receiver_lock_); std::unordered_map last_fir_ RTC_GUARDED_BY(rtcp_receiver_lock_); diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 48fef19ad0..3065534108 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -497,7 +497,8 @@ TEST(RtcpReceiverTest, InjectRrPacketWithTwoReportBlocks) { kSequenceNumbers[1]))))); } -TEST(RtcpReceiverTest, InjectRrPacketsFromTwoRemoteSsrcs) { +TEST(RtcpReceiverTest, + InjectRrPacketsFromTwoRemoteSsrcsReturnsLatestReportBlock) { const uint32_t kSenderSsrc2 = 0x20304; const uint16_t kSequenceNumbers[] = {10, 12423}; const int32_t kCumLost[] = {13, 555}; @@ -552,14 +553,6 @@ TEST(RtcpReceiverTest, InjectRrPacketsFromTwoRemoteSsrcs) { EXPECT_THAT( receiver.GetLatestReportBlockData(), UnorderedElementsAre( - Property( - &ReportBlockData::report_block, - AllOf(Field(&RTCPReportBlock::source_ssrc, kReceiverMainSsrc), - Field(&RTCPReportBlock::sender_ssrc, kSenderSsrc), - Field(&RTCPReportBlock::fraction_lost, kFracLost[0]), - Field(&RTCPReportBlock::packets_lost, kCumLost[0]), - Field(&RTCPReportBlock::extended_highest_sequence_number, - kSequenceNumbers[0]))), Property( &ReportBlockData::report_block, AllOf(Field(&RTCPReportBlock::source_ssrc, kReceiverMainSsrc),