Simplify ReportBlockStats

Bug: webrtc:10679
Change-Id: I946e805eb4edf3c3fc39b78235a5bd353db11598
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147644
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28734}

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();

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 {

video/report_block_stats.h

@@ -14,44 +14,47 @@
 #include <stdint.h>
 
 #include <map>
-#include <vector>
 
 #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<uint32_t, RTCPReportBlock> ReportBlockMap;
-  typedef std::vector<RTCPReportBlock> 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<uint32_t, Report> prev_reports_;
 };
 
 }  // namespace webrtc

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<RTCPReportBlock> ssrc1block1_;
-  std::vector<RTCPReportBlock> ssrc1block2_;
-  std::vector<RTCPReportBlock> ssrc12block1_;
-  std::vector<RTCPReportBlock> 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());
 }
 

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) {}