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 <stefan@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21142}

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;

modules/rtp_rtcp/source/rtcp_packet/report_block.cc

@@ -76,8 +76,9 @@ void ReportBlock::Create(uint8_t* buffer) const {
   ByteWriter<uint32_t>::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;

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

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

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