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}
diff --git a/call/video_send_stream.cc b/call/video_send_stream.cc
index 244d780..25513e4 100644
--- a/call/video_send_stream.cc
+++ b/call/video_send_stream.cc
@@ -51,8 +51,13 @@
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 << ", ";
- ss << "max_ext_seq: " << rtcp_stats.extended_highest_sequence_number << ", ";
+ if (report_block_data) {
+ 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;
diff --git a/call/video_send_stream.h b/call/video_send_stream.h
index 5c8906f..fd7a101 100644
--- a/call/video_send_stream.h
+++ b/call/video_send_stream.h
@@ -82,7 +82,6 @@
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;
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 9fd5751..0bf4f20 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -2661,15 +2661,18 @@
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;
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index 6d29e3d..0c32f8a 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -5585,9 +5585,11 @@
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;
- substream.rtcp_stats.fraction_lost = 18;
+ webrtc::RTCPReportBlock report_block;
+ 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 @@
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 @@
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;
- substream.rtcp_stats.fraction_lost = 18;
+ webrtc::RTCPReportBlock report_block;
+ 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 @@
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);
diff --git a/video/end_to_end_tests/stats_tests.cc b/video/end_to_end_tests/stats_tests.cc
index 349f551..bb613a5 100644
--- a/video/end_to_end_tests/stats_tests.cc
+++ b/video/end_to_end_tests/stats_tests.cc
@@ -182,9 +182,7 @@
const VideoSendStream::StreamStats& stream_stats = kv.second;
send_stats_filled_[CompoundKey("StatisticsUpdated", kv.first)] |=
- stream_stats.rtcp_stats.packets_lost != 0 ||
- stream_stats.rtcp_stats.extended_highest_sequence_number != 0 ||
- stream_stats.rtcp_stats.fraction_lost != 0;
+ stream_stats.report_block_data.has_value();
send_stats_filled_[CompoundKey("DataCountersUpdated", kv.first)] |=
stream_stats.rtp_stats.fec.packets != 0 ||
diff --git a/video/report_block_stats.cc b/video/report_block_stats.cc
index e3e95f9..bf60364 100644
--- a/video/report_block_stats.cc
+++ b/video/report_block_stats.cc
@@ -31,16 +31,13 @@
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.extended_highest_sequence_number =
- rtcp_stats.extended_highest_sequence_number;
- StoreAndAddPacketIncrement(ssrc, report);
-}
+ report.packets_lost = packets_lost;
+ report.extended_highest_sequence_number = extended_highest_sequence_number;
-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()) {
diff --git a/video/report_block_stats.h b/video/report_block_stats.h
index de4a079..1d11402 100644
--- a/video/report_block_stats.h
+++ b/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 @@
~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 @@
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_;
diff --git a/video/report_block_stats_unittest.cc b/video/report_block_stats_unittest.cc
index 0b02309..bd66e57 100644
--- a/video/report_block_stats_unittest.cc
+++ b/video/report_block_stats_unittest.cc
@@ -13,65 +13,51 @@
#include "test/gtest.h"
namespace webrtc {
+namespace {
-class ReportBlockStatsTest : public ::testing::Test {
- protected:
- 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;
- }
+constexpr uint32_t kSsrc1 = 123;
+constexpr uint32_t kSsrc2 = 234;
- const uint32_t kSsrc1 = 123;
- const uint32_t kSsrc2 = 234;
- RtcpStatistics stats1_1_;
- RtcpStatistics stats1_2_;
- RtcpStatistics stats1_3_;
- RtcpStatistics stats2_1_;
- RtcpStatistics stats2_2_;
-};
-
-TEST_F(ReportBlockStatsTest, StoreAndGetFractionLost) {
+TEST(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
diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc
index 971e42f..1b968ef 100644
--- a/video/send_statistics_proxy.cc
+++ b/video/send_statistics_proxy.cc
@@ -1293,13 +1293,11 @@
if (!stats)
return;
const RTCPReportBlock& report_block = report_block_data.report_block();
- stats->rtcp_stats.fraction_lost = report_block.fraction_lost;
- stats->rtcp_stats.packets_lost = report_block.packets_lost;
- stats->rtcp_stats.extended_highest_sequence_number =
- report_block.extended_highest_sequence_number;
- stats->rtcp_stats.jitter = report_block.jitter;
- uma_container_->report_block_stats_.Store(report_block.source_ssrc,
- stats->rtcp_stats);
+ uma_container_->report_block_stats_.Store(
+ /*ssrc=*/report_block.source_ssrc,
+ /*packets_lost=*/report_block.packets_lost,
+ /*extended_highest_sequence_number=*/
+ report_block.extended_highest_sequence_number);
stats->report_block_data = std::move(report_block_data);
}
diff --git a/video/send_statistics_proxy_unittest.cc b/video/send_statistics_proxy_unittest.cc
index aa13302..d4a7a49 100644
--- a/video/send_statistics_proxy_unittest.cc
+++ b/video/send_statistics_proxy_unittest.cc
@@ -159,11 +159,19 @@
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.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);
+ EXPECT_EQ(a.report_block_data.has_value(),
+ b.report_block_data.has_value());
+ if (a.report_block_data.has_value()) {
+ const RTCPReportBlock& a_rtcp_stats =
+ 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 @@
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);
}
