Delete clamping cumulative loss in ReportBlocks on receiving side
Bug: webrtc:9598
Change-Id: I8a54af88708e5d96e46ba67ab0ef2e0e59fe0b86
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300941
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39887}
diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
index 43b6fc8..9d2e115 100644
--- a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
+++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
@@ -1122,8 +1122,8 @@
logged_report_block.source_ssrc());
EXPECT_EQ(original_report_block.fraction_lost(),
logged_report_block.fraction_lost());
- EXPECT_EQ(original_report_block.cumulative_lost_signed(),
- logged_report_block.cumulative_lost_signed());
+ EXPECT_EQ(original_report_block.cumulative_lost(),
+ logged_report_block.cumulative_lost());
EXPECT_EQ(original_report_block.extended_high_seq_num(),
logged_report_block.extended_high_seq_num());
EXPECT_EQ(original_report_block.jitter(), logged_report_block.jitter());
diff --git a/modules/rtp_rtcp/include/report_block_data.cc b/modules/rtp_rtcp/include/report_block_data.cc
index 81f1b01..c2a3713 100644
--- a/modules/rtp_rtcp/include/report_block_data.cc
+++ b/modules/rtp_rtcp/include/report_block_data.cc
@@ -22,7 +22,7 @@
report_block_.sender_ssrc = sender_ssrc;
report_block_.source_ssrc = report_block.source_ssrc();
report_block_.fraction_lost = report_block.fraction_lost();
- report_block_.packets_lost = report_block.cumulative_lost_signed();
+ report_block_.packets_lost = report_block.cumulative_lost();
report_block_.extended_highest_sequence_number =
report_block.extended_high_seq_num();
report_block_.jitter = report_block.jitter();
diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
index 92c8f34..b6593a4 100644
--- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
@@ -285,7 +285,7 @@
// 20% = 51/255.
EXPECT_EQ(51u, report_blocks[0].fraction_lost());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
EXPECT_EQ(20, statistician->GetFractionLostInPercent());
@@ -308,7 +308,7 @@
// 20% = 51/255.
EXPECT_EQ(51u, report_blocks[0].fraction_lost());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
EXPECT_EQ(20, statistician->GetFractionLostInPercent());
@@ -333,7 +333,7 @@
// 20% = 51/255.
EXPECT_EQ(51u, report_blocks[0].fraction_lost());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
EXPECT_EQ(20, statistician->GetFractionLostInPercent());
@@ -359,7 +359,7 @@
// 20% = 51/255.
EXPECT_EQ(51u, report_blocks[0].fraction_lost());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
EXPECT_EQ(20, statistician->GetFractionLostInPercent());
@@ -374,7 +374,7 @@
// 50% = 127/255.
EXPECT_EQ(127u, report_blocks[0].fraction_lost());
- EXPECT_EQ(2, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(2, report_blocks[0].cumulative_lost());
// 2 packets lost, 7 expected
EXPECT_EQ(28, statistician->GetFractionLostInPercent());
}
@@ -396,7 +396,7 @@
EXPECT_EQ(kSsrc1, report_blocks[0].source_ssrc());
EXPECT_EQ(0, report_blocks[0].fraction_lost());
- EXPECT_EQ(0, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(0, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
EXPECT_EQ(0, statistician->GetFractionLostInPercent());
@@ -408,7 +408,7 @@
EXPECT_EQ(kSsrc1, report_blocks[0].source_ssrc());
EXPECT_EQ(0, report_blocks[0].fraction_lost());
- EXPECT_EQ(0, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(0, report_blocks[0].cumulative_lost());
EXPECT_EQ(0, statistician->GetFractionLostInPercent());
}
@@ -432,7 +432,7 @@
ASSERT_THAT(report_blocks, SizeIs(1));
EXPECT_EQ(kSsrc1, report_blocks[0].source_ssrc());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
StreamStatistician* statistician =
receive_statistics_->GetStatistician(kSsrc1);
@@ -460,7 +460,7 @@
ASSERT_THAT(report_blocks, SizeIs(1));
EXPECT_EQ(kSsrc1, report_blocks[0].source_ssrc());
- EXPECT_EQ(1, report_blocks[0].cumulative_lost_signed());
+ EXPECT_EQ(1, report_blocks[0].cumulative_lost());
}
TEST_P(ReceiveStatisticsTest, StreamRestartNeedsTwoConsecutivePackets) {
diff --git a/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
index 23ea496..47f8eb1 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
@@ -51,7 +51,7 @@
const ReportBlock& rb = parsed.report_blocks().front();
EXPECT_EQ(kRemoteSsrc, rb.source_ssrc());
EXPECT_EQ(kFractionLost, rb.fraction_lost());
- EXPECT_EQ(kCumulativeLost, rb.cumulative_lost_signed());
+ EXPECT_EQ(kCumulativeLost, rb.cumulative_lost());
EXPECT_EQ(kExtHighestSeqNum, rb.extended_high_seq_num());
EXPECT_EQ(kJitter, rb.jitter());
EXPECT_EQ(kLastSr, rb.last_sr());
diff --git a/modules/rtp_rtcp/source/rtcp_packet/report_block.cc b/modules/rtp_rtcp/source/rtcp_packet/report_block.cc
index d4579fc..e7e92d2 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/report_block.cc
+++ b/modules/rtp_rtcp/source/rtcp_packet/report_block.cc
@@ -65,12 +65,11 @@
void ReportBlock::Create(uint8_t* buffer) const {
// Runtime check should be done while setting cumulative_lost.
- RTC_DCHECK_LT(cumulative_lost_signed(),
- (1 << 23)); // Have only 3 bytes for it.
+ RTC_DCHECK_LT(cumulative_lost(), (1 << 23)); // Have only 3 bytes for it.
ByteWriter<uint32_t>::WriteBigEndian(&buffer[0], source_ssrc());
ByteWriter<uint8_t>::WriteBigEndian(&buffer[4], fraction_lost());
- ByteWriter<int32_t, 3>::WriteBigEndian(&buffer[5], cumulative_lost_signed());
+ ByteWriter<int32_t, 3>::WriteBigEndian(&buffer[5], cumulative_lost());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[8], extended_high_seq_num());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[12], jitter());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[16], last_sr());
@@ -88,13 +87,5 @@
return true;
}
-uint32_t ReportBlock::cumulative_lost() const {
- if (cumulative_lost_ < 0) {
- RTC_LOG(LS_VERBOSE) << "Ignoring negative value of cumulative_lost";
- return 0;
- }
- return cumulative_lost_;
-}
-
} // namespace rtcp
} // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtcp_packet/report_block.h b/modules/rtp_rtcp/source/rtcp_packet/report_block.h
index eb16640..6ed4f65 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/report_block.h
+++ b/modules/rtp_rtcp/source/rtcp_packet/report_block.h
@@ -49,9 +49,10 @@
uint32_t source_ssrc() const { return source_ssrc_; }
uint8_t fraction_lost() const { return fraction_lost_; }
- int32_t cumulative_lost_signed() const { return cumulative_lost_; }
- // Deprecated - returns max(0, cumulative_lost_), not negative values.
- uint32_t cumulative_lost() const;
+ [[deprecated]] int32_t cumulative_lost_signed() const {
+ return cumulative_lost_;
+ }
+ int32_t cumulative_lost() const { return cumulative_lost_; }
uint32_t extended_high_seq_num() const { return extended_high_seq_num_; }
uint32_t jitter() const { return jitter_; }
uint32_t last_sr() const { return last_sr_; }
diff --git a/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc
index 5cc102f..11031a0 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc
@@ -68,7 +68,7 @@
EXPECT_EQ(kRemoteSsrc, parsed.source_ssrc());
EXPECT_EQ(kFractionLost, parsed.fraction_lost());
- EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost_signed());
+ EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost());
EXPECT_EQ(kExtHighestSeqNum, parsed.extended_high_seq_num());
EXPECT_EQ(kJitter, parsed.jitter());
EXPECT_EQ(kLastSr, parsed.last_sr());
@@ -77,9 +77,6 @@
TEST(RtcpPacketReportBlockTest, ValidateCumulativeLost) {
// CumulativeLost is a signed 24-bit integer.
- // However, existing code expects it to be an unsigned integer.
- // The case of negative values should be unusual; we return 0
- // when caller wants an unsigned integer.
const int32_t kMaxCumulativeLost = 0x7fffff;
const int32_t kMinCumulativeLost = -0x800000;
ReportBlock rb;
@@ -87,8 +84,7 @@
EXPECT_TRUE(rb.SetCumulativeLost(kMaxCumulativeLost));
EXPECT_FALSE(rb.SetCumulativeLost(kMinCumulativeLost - 1));
EXPECT_TRUE(rb.SetCumulativeLost(kMinCumulativeLost));
- EXPECT_EQ(kMinCumulativeLost, rb.cumulative_lost_signed());
- EXPECT_EQ(0u, rb.cumulative_lost());
+ EXPECT_EQ(rb.cumulative_lost(), kMinCumulativeLost);
}
TEST(RtcpPacketReportBlockTest, ParseNegativeCumulativeLost) {
@@ -103,7 +99,7 @@
ReportBlock parsed;
EXPECT_TRUE(parsed.Parse(buffer, kBufferLength));
- EXPECT_EQ(kNegativeCumulativeLost, parsed.cumulative_lost_signed());
+ EXPECT_EQ(kNegativeCumulativeLost, parsed.cumulative_lost());
}
} // namespace
diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
index a1a3467..79d57bc 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
@@ -1579,8 +1579,7 @@
EXPECT_EQ(rtcp_block.source_ssrc(), report_block.source_ssrc);
EXPECT_EQ(kSenderSsrc, report_block.sender_ssrc);
EXPECT_EQ(rtcp_block.fraction_lost(), report_block.fraction_lost);
- EXPECT_EQ(rtcp_block.cumulative_lost_signed(),
- report_block.packets_lost);
+ EXPECT_EQ(rtcp_block.cumulative_lost(), report_block.packets_lost);
EXPECT_EQ(rtcp_block.extended_high_seq_num(),
report_block.extended_highest_sequence_number);
EXPECT_EQ(rtcp_block.jitter(), report_block.jitter);
diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index ae59dc5..0a07ab2 100644
--- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -280,7 +280,7 @@
const rtcp::ReportBlock& rb = parser()->receiver_report()->report_blocks()[0];
EXPECT_EQ(kRemoteSsrc, rb.source_ssrc());
EXPECT_EQ(0U, rb.fraction_lost());
- EXPECT_EQ(0, rb.cumulative_lost_signed());
+ EXPECT_EQ(0, rb.cumulative_lost());
EXPECT_EQ(kSeqNum, rb.extended_high_seq_num());
}
