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