Fix bug where rtcp::TransportFeedback may generate incorrect messages.

In particular, if 14 short deltas were inserted (2 * capacity of status
vector chunk with 2bit items) followed by a large delta, that status
item would be dropped.

BUG=

Review URL: https://codereview.webrtc.org/1367193002

Cr-Commit-Position: refs/heads/master@{#10132}

webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc

@@ -447,7 +447,7 @@ bool TransportFeedback::Encode(StatusSymbol symbol) {
       return true;
     } else {
       // New symbol does not match what's already in symbol_vec.
-      if (first_symbol_cardinality_ > capacity) {
+      if (first_symbol_cardinality_ >= capacity) {
         // Symbols in symbol_vec can only be RLE-encoded. Emit the RLE-chunk
         // and re-add input. symbol_vec is then guaranteed to have room for the
         // symbol, so recursion cannot continue.

webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc

@@ -454,5 +454,29 @@ TEST(RtcpPacketTest, TransportFeedback_Padding) {
   EXPECT_EQ(kExpectedSizeWords * 4, packet->Length());  // Padding not included.
 }
 
+TEST(RtcpPacketTest, TransportFeedback_CorrectlySplitsVectorChunks) {
+  const int kOneBitVectorCapacity = 14;
+  const int64_t kLargeTimeDelta =
+      TransportFeedback::kDeltaScaleFactor * (1 << 8);
+
+  // Test that a number of small deltas followed by a large delta results in a
+  // correct split into multiple chunks, as needed.
+
+  for (int deltas = 0; deltas <= kOneBitVectorCapacity + 1; ++deltas) {
+    TransportFeedback feedback;
+    feedback.WithBase(0, 0);
+    for (int i = 0; i < deltas; ++i)
+      feedback.WithReceivedPacket(i, i * 1000);
+    feedback.WithReceivedPacket(deltas, deltas * 1000 + kLargeTimeDelta);
+
+    rtc::scoped_ptr<rtcp::RawPacket> serialized_packet = feedback.Build();
+    EXPECT_TRUE(serialized_packet.get() != nullptr);
+    rtc::scoped_ptr<TransportFeedback> deserialized_packet =
+        TransportFeedback::ParseFrom(serialized_packet->Buffer(),
+                                     serialized_packet->Length());
+    EXPECT_TRUE(deserialized_packet.get() != nullptr);
+  }
+}
+
 }  // namespace
 }  // namespace webrtc

webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc

@@ -19,11 +19,13 @@
 #include "webrtc/common_types.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_receiver.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 
 namespace webrtc {
 
@@ -117,9 +119,10 @@ class RtcpReceiverTest : public ::testing::Test {
     rtcp_packet_info_.ntp_frac = rtcpPacketInformation.ntp_frac;
     rtcp_packet_info_.rtp_timestamp = rtcpPacketInformation.rtp_timestamp;
     rtcp_packet_info_.xr_dlrr_item = rtcpPacketInformation.xr_dlrr_item;
-    if (rtcpPacketInformation.VoIPMetric) {
+    if (rtcpPacketInformation.VoIPMetric)
       rtcp_packet_info_.AddVoIPMetric(rtcpPacketInformation.VoIPMetric);
-    }
+    rtcp_packet_info_.transport_feedback_.reset(
+        rtcpPacketInformation.transport_feedback_.release());
     return 0;
   }
 
@@ -1025,6 +1028,68 @@ TEST_F(RtcpReceiverTest, Callbacks) {
                                kCumulativeLoss, kJitter));
 }
 
+TEST_F(RtcpReceiverTest, ReceivesTransportFeedback) {
+  const uint32_t kSenderSsrc = 0x10203;
+  const uint32_t kSourceSsrc = 0x123456;
+
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kSourceSsrc);
+  rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
+
+  rtcp::TransportFeedback packet;
+  packet.WithMediaSourceSsrc(kSourceSsrc);
+  packet.WithPacketSenderSsrc(kSenderSsrc);
+  packet.WithBase(1, 1000);
+  packet.WithReceivedPacket(1, 1000);
+
+  rtc::scoped_ptr<rtcp::RawPacket> built_packet = packet.Build();
+  ASSERT_TRUE(built_packet.get() != nullptr);
+
+  EXPECT_EQ(0,
+            InjectRtcpPacket(built_packet->Buffer(), built_packet->Length()));
+
+  EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback);
+  EXPECT_TRUE(rtcp_packet_info_.transport_feedback_.get() != nullptr);
+}
+
+TEST_F(RtcpReceiverTest, HandlesInvalidTransportFeedback) {
+  const uint32_t kSenderSsrc = 0x10203;
+  const uint32_t kSourceSsrc = 0x123456;
+
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kSourceSsrc);
+  rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
+
+  // Send a compound packet with a TransportFeedback followed by something else.
+  rtcp::TransportFeedback packet;
+  packet.WithMediaSourceSsrc(kSourceSsrc);
+  packet.WithPacketSenderSsrc(kSenderSsrc);
+  packet.WithBase(1, 1000);
+  packet.WithReceivedPacket(1, 1000);
+
+  static uint32_t kBitrateBps = 50000;
+  rtcp::Remb remb;
+  remb.From(kSourceSsrc);
+  remb.WithBitrateBps(kBitrateBps);
+  packet.Append(&remb);
+
+  rtc::scoped_ptr<rtcp::RawPacket> built_packet = packet.Build();
+  ASSERT_TRUE(built_packet.get() != nullptr);
+
+  // Modify the TransportFeedback packet so that it is invalid.
+  const size_t kStatusCountOffset = 14;
+  ByteWriter<uint16_t>::WriteBigEndian(
+      &built_packet->MutableBuffer()[kStatusCountOffset], 42);
+
+  EXPECT_EQ(0,
+            InjectRtcpPacket(built_packet->Buffer(), built_packet->Length()));
+
+  // Transport feedback should be ignored, but next packet should work.
+  EXPECT_EQ(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback);
+  EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpRemb);
+  EXPECT_EQ(kBitrateBps, rtcp_packet_info_.receiverEstimatedMaxBitrate);
+}
+
 }  // Anonymous namespace
 
 }  // namespace webrtc

webrtc/modules/rtp_rtcp/source/rtcp_utility.cc

@@ -1235,13 +1235,13 @@ bool RTCPUtility::RTCPParserV2::ParseFBCommon(const RtcpCommonHeader& header) {
             return true;
         }
         case 15: {
-          _packetType = RTCPPacketTypes::kTransportFeedback;
           rtcp_packet_ =
               rtcp::TransportFeedback::ParseFrom(_ptrRTCPData - 12, length);
           // Since we parse the whole packet here, keep the TopLevel state and
           // just end the current block.
+          EndCurrentBlock();
           if (rtcp_packet_.get()) {
-            EndCurrentBlock();
+            _packetType = RTCPPacketTypes::kTransportFeedback;
             return true;
           }
           break;