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..af2750c081 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; diff --git a/modules/rtp_rtcp/source/rtcp_packet/report_block.cc b/modules/rtp_rtcp/source/rtcp_packet/report_block.cc index db84b6cfed..0eb8a5cb67 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/report_block.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/report_block.cc @@ -76,8 +76,9 @@ void ReportBlock::Create(uint8_t* buffer) const { 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; diff --git a/modules/rtp_rtcp/source/rtcp_packet/report_block.h b/modules/rtp_rtcp/source/rtcp_packet/report_block.h index 0d5c2fe6da..66fe28a520 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,20 @@ 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() 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_; } 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..f43b21762c 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; @@ -76,10 +76,14 @@ TEST(RtcpPacketReportBlockTest, ParseMatchCreate) { } TEST(RtcpPacketReportBlockTest, ValidateCumulativeLost) { - const uint32_t kMaxCumulativeLost = 0xffffff; + // CumulativeLost is a signed 24-bit 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)); } } // namespace diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 9ba7689425..03faf0d5ce 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()); EXPECT_EQ(kSeqNum, rb.extended_high_seq_num()); }