diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index ad09ad3cd2..37ef332e52 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -179,7 +179,7 @@ struct RTCPReportBlock { RTCPReportBlock(uint32_t sender_ssrc, uint32_t source_ssrc, uint8_t fraction_lost, - uint32_t packets_lost, + int32_t packets_lost, uint32_t extended_highest_sequence_number, uint32_t jitter, uint32_t last_sender_report_timestamp, 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 8c105cd365..fb434d9840 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc @@ -27,7 +27,7 @@ namespace { const uint32_t kSenderSsrc = 0x12345678; const uint32_t kRemoteSsrc = 0x23456789; const uint8_t kFractionLost = 55; -const uint32_t kCumulativeLost = 0x111213; +const int32_t kCumulativeLost = 0x111213; const uint32_t kExtHighestSeqNum = 0x22232425; const uint32_t kJitter = 0x33343536; const uint32_t kLastSr = 0x44454647; @@ -51,7 +51,7 @@ TEST(RtcpPacketReceiverReportTest, ParseWithOneReportBlock) { 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()); + EXPECT_EQ(kCumulativeLost, rb.cumulative_lost_signed()); 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 db84b6cfed..4f98963766 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/report_block.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/report_block.cc @@ -65,19 +65,21 @@ bool ReportBlock::Parse(const uint8_t* buffer, size_t length) { void ReportBlock::Create(uint8_t* buffer) const { // Runtime check should be done while setting cumulative_lost. - RTC_DCHECK_LT(cumulative_lost(), (1 << 24)); // Have only 3 bytes for it. + RTC_DCHECK_LT(cumulative_lost_signed(), + (1 << 23)); // Have only 3 bytes for it. ByteWriter::WriteBigEndian(&buffer[0], source_ssrc()); ByteWriter::WriteBigEndian(&buffer[4], fraction_lost()); - ByteWriter::WriteBigEndian(&buffer[5], cumulative_lost()); + ByteWriter::WriteBigEndian(&buffer[5], cumulative_lost_signed()); ByteWriter::WriteBigEndian(&buffer[8], extended_high_seq_num()); ByteWriter::WriteBigEndian(&buffer[12], jitter()); ByteWriter::WriteBigEndian(&buffer[16], last_sr()); ByteWriter::WriteBigEndian(&buffer[20], delay_since_last_sr()); } -bool ReportBlock::SetCumulativeLost(uint32_t cumulative_lost) { - if (cumulative_lost >= (1u << 24)) { // Have only 3 bytes to store it. +bool ReportBlock::SetCumulativeLost(int32_t cumulative_lost) { + // We have only 3 bytes to store it, and it's a signed value. + if (cumulative_lost >= (1 << 23) || cumulative_lost < -(1 << 23)) { RTC_LOG(LS_WARNING) << "Cumulative lost is too big to fit into Report Block"; return false; @@ -86,5 +88,13 @@ bool ReportBlock::SetCumulativeLost(uint32_t cumulative_lost) { 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 0d5c2fe6da..766d59e26b 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/report_block.h +++ b/modules/rtp_rtcp/source/rtcp_packet/report_block.h @@ -17,6 +17,8 @@ namespace webrtc { namespace rtcp { +// A ReportBlock represents the Sender Report packet from +// RFC 3550 section 6.4.1. class ReportBlock { public: static const size_t kLength = 24; @@ -34,7 +36,7 @@ class ReportBlock { void SetFractionLost(uint8_t fraction_lost) { fraction_lost_ = fraction_lost; } - bool SetCumulativeLost(uint32_t cumulative_lost); + bool SetCumulativeLost(int32_t cumulative_lost); void SetExtHighestSeqNum(uint32_t ext_highest_seq_num) { extended_high_seq_num_ = ext_highest_seq_num; } @@ -46,20 +48,22 @@ class ReportBlock { uint32_t source_ssrc() const { return source_ssrc_; } uint8_t fraction_lost() const { return fraction_lost_; } - uint32_t cumulative_lost() const { return cumulative_lost_; } + int32_t cumulative_lost_signed() const { return cumulative_lost_; } + // Deprecated - returns max(0, cumulative_lost_), not negative values. + uint32_t cumulative_lost() const; 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_; } uint32_t delay_since_last_sr() const { return delay_since_last_sr_; } private: - uint32_t source_ssrc_; - uint8_t fraction_lost_; - uint32_t cumulative_lost_; - uint32_t extended_high_seq_num_; - uint32_t jitter_; - uint32_t last_sr_; - uint32_t delay_since_last_sr_; + uint32_t source_ssrc_; // 32 bits + uint8_t fraction_lost_; // 8 bits representing a fixed point value 0..1 + int32_t cumulative_lost_; // Signed 24-bit value + uint32_t extended_high_seq_num_; // 32 bits + uint32_t jitter_; // 32 bits + uint32_t last_sr_; // 32 bits + uint32_t delay_since_last_sr_; // 32 bits, units of 1/65536 seconds }; } // namespace rtcp 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 38f8104933..a074a2f4f4 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc @@ -23,7 +23,7 @@ namespace { const uint32_t kRemoteSsrc = 0x23456789; const uint8_t kFractionLost = 55; // Use values that are streamed differently LE and BE. -const uint32_t kCumulativeLost = 0x111213; +const int32_t kCumulativeLost = 0x111213; const uint32_t kExtHighestSeqNum = 0x22232425; const uint32_t kJitter = 0x33343536; const uint32_t kLastSr = 0x44454647; @@ -68,7 +68,7 @@ TEST(RtcpPacketReportBlockTest, ParseMatchCreate) { EXPECT_EQ(kRemoteSsrc, parsed.source_ssrc()); EXPECT_EQ(kFractionLost, parsed.fraction_lost()); - EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost()); + EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost_signed()); EXPECT_EQ(kExtHighestSeqNum, parsed.extended_high_seq_num()); EXPECT_EQ(kJitter, parsed.jitter()); EXPECT_EQ(kLastSr, parsed.last_sr()); @@ -76,10 +76,19 @@ TEST(RtcpPacketReportBlockTest, ParseMatchCreate) { } TEST(RtcpPacketReportBlockTest, ValidateCumulativeLost) { - const uint32_t kMaxCumulativeLost = 0xffffff; + // 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; EXPECT_FALSE(rb.SetCumulativeLost(kMaxCumulativeLost + 1)); 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()); } } // namespace diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 4bd59282ea..cff7468af6 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -461,7 +461,8 @@ void RTCPReceiver::HandleReportBlock(const ReportBlock& report_block, report_block_info->report_block.sender_ssrc = remote_ssrc; report_block_info->report_block.source_ssrc = report_block.source_ssrc(); report_block_info->report_block.fraction_lost = report_block.fraction_lost(); - report_block_info->report_block.packets_lost = report_block.cumulative_lost(); + report_block_info->report_block.packets_lost = + report_block.cumulative_lost_signed(); if (report_block.extended_high_seq_num() > report_block_info->report_block.extended_highest_sequence_number) { // We have successfully delivered new RTP packets to the remote side after diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 9ba7689425..4549c674d8 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -358,7 +358,7 @@ TEST_F(RtcpSenderTest, SendRrWithOneReportBlock) { const rtcp::ReportBlock& rb = parser()->receiver_report()->report_blocks()[0]; EXPECT_EQ(kRemoteSsrc, rb.source_ssrc()); EXPECT_EQ(0U, rb.fraction_lost()); - EXPECT_EQ(0U, rb.cumulative_lost()); + EXPECT_EQ(0, rb.cumulative_lost_signed()); EXPECT_EQ(kSeqNum, rb.extended_high_seq_num()); }