Remove redundant VideoSendStream::rtcp_stats field

its content is duplicated in the report_block_data member

Bug: webrtc:10678
Change-Id: I89421ae4ab5f727a233161924372105e222ed404
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219080
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34039}

call/video_send_stream.cc

@@ -51,8 +51,13 @@ std::string VideoSendStream::StreamStats::ToString() const {
   ss << "retransmit_bps: " << retransmit_bitrate_bps << ", ";
   ss << "avg_delay_ms: " << avg_delay_ms << ", ";
   ss << "max_delay_ms: " << max_delay_ms << ", ";
-  ss << "cum_loss: " << rtcp_stats.packets_lost << ", ";
+  if (report_block_data) {
-  ss << "max_ext_seq: " << rtcp_stats.extended_highest_sequence_number << ", ";
+    ss << "cum_loss: " << report_block_data->report_block().packets_lost
+       << ", ";
+    ss << "max_ext_seq: "
+       << report_block_data->report_block().extended_highest_sequence_number
+       << ", ";
+  }
   ss << "nack: " << rtcp_packet_type_counts.nack_packets << ", ";
   ss << "fir: " << rtcp_packet_type_counts.fir_packets << ", ";
   ss << "pli: " << rtcp_packet_type_counts.pli_packets;

call/video_send_stream.h

@@ -82,7 +82,6 @@ class VideoSendStream {
     uint64_t total_packet_send_delay_ms = 0;
     StreamDataCounters rtp_stats;
     RtcpPacketTypeCounter rtcp_packet_type_counts;
-    RtcpStatistics rtcp_stats;
     // A snapshot of the most recent Report Block with additional data of
     // interest to statistics. Used to implement RTCRemoteInboundRtpStreamStats.
     absl::optional<ReportBlockData> report_block_data;

media/engine/webrtc_video_engine.cc

@@ -2661,15 +2661,18 @@ WebRtcVideoChannel::WebRtcVideoSendStream::GetPerLayerVideoSenderInfos(
         stream_stats.rtp_stats.retransmitted.payload_bytes;
     info.retransmitted_packets_sent =
         stream_stats.rtp_stats.retransmitted.packets;
-    info.packets_lost = stream_stats.rtcp_stats.packets_lost;
     info.firs_rcvd = stream_stats.rtcp_packet_type_counts.fir_packets;
     info.nacks_rcvd = stream_stats.rtcp_packet_type_counts.nack_packets;
     info.plis_rcvd = stream_stats.rtcp_packet_type_counts.pli_packets;
     if (stream_stats.report_block_data.has_value()) {
-      info.report_block_datas.push_back(stream_stats.report_block_data.value());
+      info.packets_lost =
+          stream_stats.report_block_data->report_block().packets_lost;
+      info.fraction_lost =
+          static_cast<float>(
+              stream_stats.report_block_data->report_block().fraction_lost) /
+          (1 << 8);
+      info.report_block_datas.push_back(*stream_stats.report_block_data);
     }
-    info.fraction_lost =
-        static_cast<float>(stream_stats.rtcp_stats.fraction_lost) / (1 << 8);
     info.qp_sum = stream_stats.qp_sum;
     info.total_encode_time_ms = stream_stats.total_encode_time_ms;
     info.total_encoded_bytes_target = stream_stats.total_encoded_bytes_target;

media/engine/webrtc_video_engine_unittest.cc

@@ -5585,9 +5585,11 @@ TEST_F(WebRtcVideoChannelTest, GetAggregatedStatsReportForSubStreams) {
   substream.rtcp_packet_type_counts.fir_packets = 14;
   substream.rtcp_packet_type_counts.nack_packets = 15;
   substream.rtcp_packet_type_counts.pli_packets = 16;
-  substream.rtcp_stats.packets_lost = 17;
+  webrtc::RTCPReportBlock report_block;
-  substream.rtcp_stats.fraction_lost = 18;
+  report_block.packets_lost = 17;
+  report_block.fraction_lost = 18;
   webrtc::ReportBlockData report_block_data;
+  report_block_data.SetReportBlock(report_block, 0);
   report_block_data.AddRoundTripTimeSample(19);
   substream.report_block_data = report_block_data;
   substream.encode_frame_rate = 20.0;
@@ -5621,9 +5623,12 @@ TEST_F(WebRtcVideoChannelTest, GetAggregatedStatsReportForSubStreams) {
             static_cast<int>(2 * substream.rtp_stats.transmitted.packets));
   EXPECT_EQ(sender.retransmitted_packets_sent,
             2u * substream.rtp_stats.retransmitted.packets);
-  EXPECT_EQ(sender.packets_lost, 2 * substream.rtcp_stats.packets_lost);
+  EXPECT_EQ(sender.packets_lost,
+            2 * substream.report_block_data->report_block().packets_lost);
   EXPECT_EQ(sender.fraction_lost,
-            static_cast<float>(substream.rtcp_stats.fraction_lost) / (1 << 8));
+            static_cast<float>(
+                substream.report_block_data->report_block().fraction_lost) /
+                (1 << 8));
   EXPECT_EQ(sender.rtt_ms, 0);
   EXPECT_EQ(sender.codec_name, DefaultCodec().name);
   EXPECT_EQ(sender.codec_payload_type, DefaultCodec().id);
@@ -5703,9 +5708,11 @@ TEST_F(WebRtcVideoChannelTest, GetPerLayerStatsReportForSubStreams) {
   substream.rtcp_packet_type_counts.fir_packets = 14;
   substream.rtcp_packet_type_counts.nack_packets = 15;
   substream.rtcp_packet_type_counts.pli_packets = 16;
-  substream.rtcp_stats.packets_lost = 17;
+  webrtc::RTCPReportBlock report_block;
-  substream.rtcp_stats.fraction_lost = 18;
+  report_block.packets_lost = 17;
+  report_block.fraction_lost = 18;
   webrtc::ReportBlockData report_block_data;
+  report_block_data.SetReportBlock(report_block, 0);
   report_block_data.AddRoundTripTimeSample(19);
   substream.report_block_data = report_block_data;
   substream.encode_frame_rate = 20.0;
@@ -5739,9 +5746,12 @@ TEST_F(WebRtcVideoChannelTest, GetPerLayerStatsReportForSubStreams) {
             static_cast<int>(substream.rtp_stats.transmitted.packets));
   EXPECT_EQ(sender.retransmitted_packets_sent,
             substream.rtp_stats.retransmitted.packets);
-  EXPECT_EQ(sender.packets_lost, substream.rtcp_stats.packets_lost);
+  EXPECT_EQ(sender.packets_lost,
+            substream.report_block_data->report_block().packets_lost);
   EXPECT_EQ(sender.fraction_lost,
-            static_cast<float>(substream.rtcp_stats.fraction_lost) / (1 << 8));
+            static_cast<float>(
+                substream.report_block_data->report_block().fraction_lost) /
+                (1 << 8));
   EXPECT_EQ(sender.rtt_ms, 0);
   EXPECT_EQ(sender.codec_name, DefaultCodec().name);
   EXPECT_EQ(sender.codec_payload_type, DefaultCodec().id);

video/end_to_end_tests/stats_tests.cc

@@ -182,9 +182,7 @@ TEST_F(StatsEndToEndTest, GetStats) {
         const VideoSendStream::StreamStats& stream_stats = kv.second;
 
         send_stats_filled_[CompoundKey("StatisticsUpdated", kv.first)] |=
-            stream_stats.rtcp_stats.packets_lost != 0 ||
+            stream_stats.report_block_data.has_value();
-            stream_stats.rtcp_stats.extended_highest_sequence_number != 0 ||
-            stream_stats.rtcp_stats.fraction_lost != 0;
 
         send_stats_filled_[CompoundKey("DataCountersUpdated", kv.first)] |=
             stream_stats.rtp_stats.fec.packets != 0 ||

video/report_block_stats.cc

@@ -31,16 +31,13 @@ ReportBlockStats::ReportBlockStats()
 
 ReportBlockStats::~ReportBlockStats() {}
 
-void ReportBlockStats::Store(uint32_t ssrc, const RtcpStatistics& rtcp_stats) {
+void ReportBlockStats::Store(uint32_t ssrc,
+                             int packets_lost,
+                             uint32_t extended_highest_sequence_number) {
   Report report;
-  report.packets_lost = rtcp_stats.packets_lost;
+  report.packets_lost = packets_lost;
-  report.extended_highest_sequence_number =
+  report.extended_highest_sequence_number = extended_highest_sequence_number;
-      rtcp_stats.extended_highest_sequence_number;
-  StoreAndAddPacketIncrement(ssrc, report);
-}
 
-void ReportBlockStats::StoreAndAddPacketIncrement(uint32_t ssrc,
-                                                  const Report& report) {
   // Get diff with previous report block.
   const auto prev_report = prev_reports_.find(ssrc);
   if (prev_report != prev_reports_.end()) {

video/report_block_stats.h

@@ -15,8 +15,6 @@
 
 #include <map>
 
-#include "modules/rtp_rtcp/include/rtcp_statistics.h"
-
 namespace webrtc {
 
 // TODO(nisse): Usefulness of this class is somewhat unclear. The inputs are
@@ -32,7 +30,9 @@ class ReportBlockStats {
   ~ReportBlockStats();
 
   // Updates stats and stores report block.
-  void Store(uint32_t ssrc, const RtcpStatistics& rtcp_stats);
+  void Store(uint32_t ssrc,
+             int packets_lost,
+             uint32_t extended_highest_sequence_number);
 
   // Returns the total fraction of lost packets (or -1 if less than two report
   // blocks have been stored).
@@ -45,10 +45,6 @@ class ReportBlockStats {
     int32_t packets_lost;
   };
 
-  // Updates the total number of packets/lost packets.
-  // 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_;

video/report_block_stats_unittest.cc

@@ -13,65 +13,51 @@
 #include "test/gtest.h"
 
 namespace webrtc {
+namespace {
 
-class ReportBlockStatsTest : public ::testing::Test {
+constexpr uint32_t kSsrc1 = 123;
- protected:
+constexpr uint32_t kSsrc2 = 234;
-  ReportBlockStatsTest() {
-    // kSsrc1: report 1-3.
-    stats1_1_.packets_lost = 10;
-    stats1_1_.extended_highest_sequence_number = 24000;
-    stats1_2_.packets_lost = 15;
-    stats1_2_.extended_highest_sequence_number = 24100;
-    stats1_3_.packets_lost = 50;
-    stats1_3_.extended_highest_sequence_number = 24200;
-    // kSsrc2: report 1,2.
-    stats2_1_.packets_lost = 111;
-    stats2_1_.extended_highest_sequence_number = 8500;
-    stats2_2_.packets_lost = 136;
-    stats2_2_.extended_highest_sequence_number = 8800;
-  }
 
-  const uint32_t kSsrc1 = 123;
+TEST(ReportBlockStatsTest, StoreAndGetFractionLost) {
-  const uint32_t kSsrc2 = 234;
-  RtcpStatistics stats1_1_;
-  RtcpStatistics stats1_2_;
-  RtcpStatistics stats1_3_;
-  RtcpStatistics stats2_1_;
-  RtcpStatistics stats2_2_;
-};
-
-TEST_F(ReportBlockStatsTest, StoreAndGetFractionLost) {
   ReportBlockStats stats;
   EXPECT_EQ(-1, stats.FractionLostInPercent());
 
   // First report.
-  stats.Store(kSsrc1, stats1_1_);
+  stats.Store(kSsrc1, /*packets_lost=*/10,
+              /*extended_highest_sequence_number=*/24'000);
   EXPECT_EQ(-1, stats.FractionLostInPercent());
   // fl: 100 * (15-10) / (24100-24000) = 5%
-  stats.Store(kSsrc1, stats1_2_);
+  stats.Store(kSsrc1, /*packets_lost=*/15,
+              /*extended_highest_sequence_number=*/24'100);
   EXPECT_EQ(5, stats.FractionLostInPercent());
   // fl: 100 * (50-10) / (24200-24000) = 20%
-  stats.Store(kSsrc1, stats1_3_);
+  stats.Store(kSsrc1, /*packets_lost=*/50,
+              /*extended_highest_sequence_number=*/24'200);
   EXPECT_EQ(20, stats.FractionLostInPercent());
 }
 
-TEST_F(ReportBlockStatsTest, StoreAndGetFractionLost_TwoSsrcs) {
+TEST(ReportBlockStatsTest, StoreAndGetFractionLost_TwoSsrcs) {
   ReportBlockStats stats;
   EXPECT_EQ(-1, stats.FractionLostInPercent());
 
   // First report.
-  stats.Store(kSsrc1, stats1_1_);
+  stats.Store(kSsrc1, /*packets_lost=*/10,
+              /*extended_highest_sequence_number=*/24'000);
   EXPECT_EQ(-1, stats.FractionLostInPercent());
   // fl: 100 * (15-10) / (24100-24000) = 5%
-  stats.Store(kSsrc1, stats1_2_);
+  stats.Store(kSsrc1, /*packets_lost=*/15,
+              /*extended_highest_sequence_number=*/24'100);
   EXPECT_EQ(5, stats.FractionLostInPercent());
 
   // First report, kSsrc2.
-  stats.Store(kSsrc2, stats2_1_);
+  stats.Store(kSsrc2, /*packets_lost=*/111,
+              /*extended_highest_sequence_number=*/8'500);
   EXPECT_EQ(5, stats.FractionLostInPercent());
   // fl: 100 * ((15-10) + (136-111)) / ((24100-24000) + (8800-8500)) = 7%
-  stats.Store(kSsrc2, stats2_2_);
+  stats.Store(kSsrc2, /*packets_lost=*/136,
+              /*extended_highest_sequence_number=*/8'800);
   EXPECT_EQ(7, stats.FractionLostInPercent());
 }
 
+}  // namespace
 }  // namespace webrtc

video/send_statistics_proxy.cc

@@ -1293,13 +1293,11 @@ void SendStatisticsProxy::OnReportBlockDataUpdated(
   if (!stats)
     return;
   const RTCPReportBlock& report_block = report_block_data.report_block();
-  stats->rtcp_stats.fraction_lost = report_block.fraction_lost;
+  uma_container_->report_block_stats_.Store(
-  stats->rtcp_stats.packets_lost = report_block.packets_lost;
+      /*ssrc=*/report_block.source_ssrc,
-  stats->rtcp_stats.extended_highest_sequence_number =
+      /*packets_lost=*/report_block.packets_lost,
-      report_block.extended_highest_sequence_number;
+      /*extended_highest_sequence_number=*/
-  stats->rtcp_stats.jitter = report_block.jitter;
+      report_block.extended_highest_sequence_number);
-  uma_container_->report_block_stats_.Store(report_block.source_ssrc,
-                                            stats->rtcp_stats);
 
   stats->report_block_data = std::move(report_block_data);
 }

video/send_statistics_proxy_unittest.cc

@@ -159,11 +159,19 @@ class SendStatisticsProxyTest : public ::testing::Test {
                 b.rtp_stats.retransmitted.packets);
       EXPECT_EQ(a.rtp_stats.fec.packets, b.rtp_stats.fec.packets);
 
-      EXPECT_EQ(a.rtcp_stats.fraction_lost, b.rtcp_stats.fraction_lost);
+      EXPECT_EQ(a.report_block_data.has_value(),
-      EXPECT_EQ(a.rtcp_stats.packets_lost, b.rtcp_stats.packets_lost);
+                b.report_block_data.has_value());
-      EXPECT_EQ(a.rtcp_stats.extended_highest_sequence_number,
+      if (a.report_block_data.has_value()) {
-                b.rtcp_stats.extended_highest_sequence_number);
+        const RTCPReportBlock& a_rtcp_stats =
-      EXPECT_EQ(a.rtcp_stats.jitter, b.rtcp_stats.jitter);
+            a.report_block_data->report_block();
+        const RTCPReportBlock& b_rtcp_stats =
+            b.report_block_data->report_block();
+        EXPECT_EQ(a_rtcp_stats.fraction_lost, b_rtcp_stats.fraction_lost);
+        EXPECT_EQ(a_rtcp_stats.packets_lost, b_rtcp_stats.packets_lost);
+        EXPECT_EQ(a_rtcp_stats.extended_highest_sequence_number,
+                  b_rtcp_stats.extended_highest_sequence_number);
+        EXPECT_EQ(a_rtcp_stats.jitter, b_rtcp_stats.jitter);
+      }
     }
   }
 
@@ -177,46 +185,32 @@ class SendStatisticsProxyTest : public ::testing::Test {
 TEST_F(SendStatisticsProxyTest, ReportBlockDataObserver) {
   ReportBlockDataObserver* callback = statistics_proxy_.get();
   for (uint32_t ssrc : config_.rtp.ssrcs) {
-    VideoSendStream::StreamStats& ssrc_stats = expected_.substreams[ssrc];
-
     // Add statistics with some arbitrary, but unique, numbers.
-    uint32_t offset = ssrc * sizeof(RtcpStatistics);
+    uint32_t offset = ssrc * 4;
     RTCPReportBlock report_block;
     report_block.source_ssrc = ssrc;
     report_block.packets_lost = offset;
     report_block.extended_highest_sequence_number = offset + 1;
     report_block.fraction_lost = offset + 2;
     report_block.jitter = offset + 3;
-
-    ssrc_stats.rtcp_stats.packets_lost = report_block.packets_lost;
-    ssrc_stats.rtcp_stats.extended_highest_sequence_number =
-        report_block.extended_highest_sequence_number;
-    ssrc_stats.rtcp_stats.fraction_lost = report_block.fraction_lost;
-    ssrc_stats.rtcp_stats.jitter = report_block.jitter;
     ReportBlockData data;
     data.SetReportBlock(report_block, 0);
+    expected_.substreams[ssrc].report_block_data = data;
 
     callback->OnReportBlockDataUpdated(data);
   }
   for (uint32_t ssrc : config_.rtp.rtx.ssrcs) {
-    VideoSendStream::StreamStats& ssrc_stats = expected_.substreams[ssrc];
-
     // Add statistics with some arbitrary, but unique, numbers.
-    uint32_t offset = ssrc * sizeof(RtcpStatistics);
+    uint32_t offset = ssrc * 4;
     RTCPReportBlock report_block;
     report_block.source_ssrc = ssrc;
     report_block.packets_lost = offset;
     report_block.extended_highest_sequence_number = offset + 1;
     report_block.fraction_lost = offset + 2;
     report_block.jitter = offset + 3;
-
-    ssrc_stats.rtcp_stats.packets_lost = report_block.packets_lost;
-    ssrc_stats.rtcp_stats.extended_highest_sequence_number =
-        report_block.extended_highest_sequence_number;
-    ssrc_stats.rtcp_stats.fraction_lost = report_block.fraction_lost;
-    ssrc_stats.rtcp_stats.jitter = report_block.jitter;
     ReportBlockData data;
     data.SetReportBlock(report_block, 0);
+    expected_.substreams[ssrc].report_block_data = data;
 
     callback->OnReportBlockDataUpdated(data);
   }