From 70206d660841344dc580a2533678d54a862a7be7 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Fri, 8 Dec 2017 08:59:07 +0100 Subject: [PATCH] Reland "Make RTCP cumulative_lost be a signed value" Instead of modifying the API, we'll add a new function to return the true value, and have a shim that returns what other code expects. > This reverts commit 4c34f435db2b921b82b8be19ee5c1746f46cb188. > > Reason for revert: Broke internal projects. Type mismatch. > > Original change's description: > > Make RTCP cumulative_lost be a signed value > > > > This is formally defined as a signed 24-bit value in RFC 3550 section 6.4.1. > > See RFC 3550 Appendix A.3 for the reason why it may turn negative. > > > > Noticed on discuss-webrtc mailing list. > > > > BUG=webrtc:8626 > > > > Change-Id: I7317f73e9490a876e8445bd3d6b66095ce53ca0a > > Reviewed-on: https://webrtc-review.googlesource.com/30901 > > Reviewed-by: Stefan Holmer > > Commit-Queue: Harald Alvestrand > > Cr-Commit-Position: refs/heads/master@{#21142} > > TBR=stefan@webrtc.org,hta@webrtc.org > > Change-Id: I544f7979d584cfb72a2d0d526f4fef84aebeecb3 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:8626 > Reviewed-on: https://webrtc-review.googlesource.com/31040 > Reviewed-by: Zhi Huang > Commit-Queue: Zhi Huang > Cr-Commit-Position: refs/heads/master@{#21144} Change-Id: I95c8c248f4f85c4d1aa2a47424d8c4d954d4ae7a Bug: webrtc:8626 Reviewed-on: https://webrtc-review.googlesource.com/31220 Commit-Queue: Harald Alvestrand Reviewed-by: Harald Alvestrand Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#21154} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 2 +- .../rtcp_packet/receiver_report_unittest.cc | 4 ++-- .../source/rtcp_packet/report_block.cc | 18 +++++++++++---- .../source/rtcp_packet/report_block.h | 22 +++++++++++-------- .../rtcp_packet/report_block_unittest.cc | 15 ++++++++++--- modules/rtp_rtcp/source/rtcp_receiver.cc | 3 ++- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 2 +- 7 files changed, 45 insertions(+), 21 deletions(-) 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()); }