From 77d3efc5097a6dde42bb1d3c5d9235bd5a925657 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 1 Aug 2019 15:09:51 +0200 Subject: [PATCH] Simplify ReportBlockStats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10679 Change-Id: I946e805eb4edf3c3fc39b78235a5bd353db11598 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147644 Commit-Queue: Niels Moller Reviewed-by: Åsa Persson Cr-Commit-Position: refs/heads/master@{#28734} --- video/receive_statistics_proxy.cc | 4 +-- video/report_block_stats.cc | 42 +++++++++------------------- video/report_block_stats.h | 31 ++++++++++---------- video/report_block_stats_unittest.cc | 35 ++++------------------- video/send_statistics_proxy.cc | 2 +- 5 files changed, 37 insertions(+), 77 deletions(-) diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index 6c9d121ac3..937a374974 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -674,12 +674,10 @@ void ReceiveStatisticsProxy::StatisticsUpdated( const webrtc::RtcpStatistics& statistics, uint32_t ssrc) { rtc::CritScope lock(&crit_); - // TODO(pbos): Handle both local and remote ssrcs here and RTC_DCHECK that we - // receive stats from one of them. if (stats_.ssrc != ssrc) return; stats_.rtcp_stats = statistics; - report_block_stats_.Store(statistics, ssrc, 0); + report_block_stats_.Store(0, statistics); if (first_report_block_time_ms_ == -1) first_report_block_time_ms_ = clock_->TimeInMilliseconds(); diff --git a/video/report_block_stats.cc b/video/report_block_stats.cc index 004129312f..e3e95f9aed 100644 --- a/video/report_block_stats.cc +++ b/video/report_block_stats.cc @@ -31,46 +31,30 @@ ReportBlockStats::ReportBlockStats() ReportBlockStats::~ReportBlockStats() {} -void ReportBlockStats::Store(const RtcpStatistics& rtcp_stats, - uint32_t remote_ssrc, - uint32_t source_ssrc) { - RTCPReportBlock block; - block.packets_lost = rtcp_stats.packets_lost; - block.fraction_lost = rtcp_stats.fraction_lost; - block.extended_highest_sequence_number = +void ReportBlockStats::Store(uint32_t ssrc, const RtcpStatistics& rtcp_stats) { + Report report; + report.packets_lost = rtcp_stats.packets_lost; + report.extended_highest_sequence_number = rtcp_stats.extended_highest_sequence_number; - block.jitter = rtcp_stats.jitter; - block.sender_ssrc = remote_ssrc; - block.source_ssrc = source_ssrc; - uint32_t num_sequence_numbers = 0; - uint32_t num_lost_sequence_numbers = 0; - StoreAndAddPacketIncrement(block, &num_sequence_numbers, - &num_lost_sequence_numbers); + StoreAndAddPacketIncrement(ssrc, report); } -void ReportBlockStats::StoreAndAddPacketIncrement( - const RTCPReportBlock& report_block, - uint32_t* num_sequence_numbers, - uint32_t* num_lost_sequence_numbers) { +void ReportBlockStats::StoreAndAddPacketIncrement(uint32_t ssrc, + const Report& report) { // Get diff with previous report block. - ReportBlockMap::iterator prev_report_block = - prev_report_blocks_.find(report_block.source_ssrc); - if (prev_report_block != prev_report_blocks_.end()) { - int seq_num_diff = - report_block.extended_highest_sequence_number - - prev_report_block->second.extended_highest_sequence_number; - int cum_loss_diff = - report_block.packets_lost - prev_report_block->second.packets_lost; + const auto prev_report = prev_reports_.find(ssrc); + if (prev_report != prev_reports_.end()) { + int seq_num_diff = report.extended_highest_sequence_number - + prev_report->second.extended_highest_sequence_number; + int cum_loss_diff = report.packets_lost - prev_report->second.packets_lost; if (seq_num_diff >= 0 && cum_loss_diff >= 0) { - *num_sequence_numbers += seq_num_diff; - *num_lost_sequence_numbers += cum_loss_diff; // Update total number of packets/lost packets. num_sequence_numbers_ += seq_num_diff; num_lost_sequence_numbers_ += cum_loss_diff; } } // Store current report block. - prev_report_blocks_[report_block.source_ssrc] = report_block; + prev_reports_[ssrc] = report; } int ReportBlockStats::FractionLostInPercent() const { diff --git a/video/report_block_stats.h b/video/report_block_stats.h index bb9ea788af..de4a079032 100644 --- a/video/report_block_stats.h +++ b/video/report_block_stats.h @@ -14,44 +14,47 @@ #include #include -#include #include "modules/rtp_rtcp/include/rtcp_statistics.h" -#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { +// TODO(nisse): Usefulness of this class is somewhat unclear. The inputs are +// cumulative counters, from which we compute deltas, and then accumulate the +// deltas. May be needed on the send side, to handle wraparound in the short +// counters received over RTCP, but should not be needed on the receive side +// where we can use large enough types for all counters we need. + // Helper class for rtcp statistics. class ReportBlockStats { public: - typedef std::map ReportBlockMap; - typedef std::vector ReportBlockVector; ReportBlockStats(); ~ReportBlockStats(); // Updates stats and stores report block. - void Store(const RtcpStatistics& rtcp_stats, - uint32_t remote_ssrc, - uint32_t source_ssrc); + void Store(uint32_t ssrc, const RtcpStatistics& rtcp_stats); // Returns the total fraction of lost packets (or -1 if less than two report // blocks have been stored). int FractionLostInPercent() const; private: + // The information from an RTCP report block that we need. + struct Report { + uint32_t extended_highest_sequence_number; + int32_t packets_lost; + }; + // Updates the total number of packets/lost packets. - // Stores the report block. - // Returns the number of packets/lost packets since previous report block. - void StoreAndAddPacketIncrement(const RTCPReportBlock& report_block, - uint32_t* num_sequence_numbers, - uint32_t* num_lost_sequence_numbers); + // Stores the report. + void StoreAndAddPacketIncrement(uint32_t ssrc, const Report& report); // The total number of packets/lost packets. uint32_t num_sequence_numbers_; uint32_t num_lost_sequence_numbers_; - // Map holding the last stored report block (mapped by the source SSRC). - ReportBlockMap prev_report_blocks_; + // Map holding the last stored report (mapped by the source SSRC). + std::map prev_reports_; }; } // namespace webrtc diff --git a/video/report_block_stats_unittest.cc b/video/report_block_stats_unittest.cc index d87c7f7875..23b3ab82a8 100644 --- a/video/report_block_stats_unittest.cc +++ b/video/report_block_stats_unittest.cc @@ -9,6 +9,7 @@ */ #include "video/report_block_stats.h" +#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "test/gtest.h" @@ -16,7 +17,7 @@ namespace webrtc { class ReportBlockStatsTest : public ::testing::Test { protected: - ReportBlockStatsTest() : kSsrc1(0x12345), kSsrc2(0x23456) {} + ReportBlockStatsTest() : kSsrc1(0x12345) {} void SetUp() override { // kSsrc1: block 1-3. @@ -35,24 +36,6 @@ class ReportBlockStatsTest : public ::testing::Test { block1_3_.extended_highest_sequence_number = 24200; block1_3_.jitter = 333; block1_3_.source_ssrc = kSsrc1; - // kSsrc2: block 1,2. - block2_1_.packets_lost = 111; - block2_1_.fraction_lost = 222; - block2_1_.extended_highest_sequence_number = 8500; - block2_1_.jitter = 555; - block2_1_.source_ssrc = kSsrc2; - block2_2_.packets_lost = 136; - block2_2_.fraction_lost = 0; - block2_2_.extended_highest_sequence_number = 8800; - block2_2_.jitter = 888; - block2_2_.source_ssrc = kSsrc2; - - ssrc1block1_.push_back(block1_1_); - ssrc1block2_.push_back(block1_2_); - ssrc12block1_.push_back(block1_1_); - ssrc12block1_.push_back(block2_1_); - ssrc12block2_.push_back(block1_2_); - ssrc12block2_.push_back(block2_2_); } RtcpStatistics RtcpReportBlockToRtcpStatistics(const RTCPReportBlock& stats) { @@ -66,31 +49,23 @@ class ReportBlockStatsTest : public ::testing::Test { } const uint32_t kSsrc1; - const uint32_t kSsrc2; RTCPReportBlock block1_1_; RTCPReportBlock block1_2_; RTCPReportBlock block1_3_; - RTCPReportBlock block2_1_; - RTCPReportBlock block2_2_; - std::vector ssrc1block1_; - std::vector ssrc1block2_; - std::vector ssrc12block1_; - std::vector ssrc12block2_; }; TEST_F(ReportBlockStatsTest, StoreAndGetFractionLost) { - const uint32_t kRemoteSsrc = 1; ReportBlockStats stats; EXPECT_EQ(-1, stats.FractionLostInPercent()); // First block. - stats.Store(RtcpReportBlockToRtcpStatistics(block1_1_), kRemoteSsrc, kSsrc1); + stats.Store(kSsrc1, RtcpReportBlockToRtcpStatistics(block1_1_)); EXPECT_EQ(-1, stats.FractionLostInPercent()); // fl: 100 * (15-10) / (24100-24000) = 5% - stats.Store(RtcpReportBlockToRtcpStatistics(block1_2_), kRemoteSsrc, kSsrc1); + stats.Store(kSsrc1, RtcpReportBlockToRtcpStatistics(block1_2_)); EXPECT_EQ(5, stats.FractionLostInPercent()); // fl: 100 * (50-10) / (24200-24000) = 20% - stats.Store(RtcpReportBlockToRtcpStatistics(block1_3_), kRemoteSsrc, kSsrc1); + stats.Store(kSsrc1, RtcpReportBlockToRtcpStatistics(block1_3_)); EXPECT_EQ(20, stats.FractionLostInPercent()); } diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index cf417f5c3a..72dd514dbc 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -1158,7 +1158,7 @@ void SendStatisticsProxy::StatisticsUpdated(const RtcpStatistics& statistics, return; stats->rtcp_stats = statistics; - uma_container_->report_block_stats_.Store(statistics, 0, ssrc); + uma_container_->report_block_stats_.Store(ssrc, statistics); } void SendStatisticsProxy::CNameChanged(const char* cname, uint32_t ssrc) {}