TransportFeedback must be able to start with dropped packets.

A bug in the transpot feedback adapter causes new feedback message to
always start with a received packet. This makes it impossible for the
receiver to distinguish from actual dropped packets and dropped feedback
messages.

BUG=webrtc:6073
R=stefan@webrtc.org

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

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

webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc

@@ -91,8 +91,8 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
                                            int64_t arrival_time) {
   int64_t seq = unwrapper_.Unwrap(sequence_number);
 
-  if (window_start_seq_ == -1) {
-    window_start_seq_ = seq;
+  if (packet_arrival_times_.lower_bound(window_start_seq_) ==
+      packet_arrival_times_.end()) {
     // Start new feedback packet, cull old packets.
     for (auto it = packet_arrival_times_.begin();
          it != packet_arrival_times_.end() && it->first < seq &&
@@ -101,6 +101,10 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
       ++it;
       packet_arrival_times_.erase(delete_it);
     }
+  }
+
+  if (window_start_seq_ == -1) {
+    window_start_seq_ = sequence_number;
   } else if (seq < window_start_seq_) {
     window_start_seq_ = seq;
   }
@@ -114,20 +118,24 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
 
 bool RemoteEstimatorProxy::BuildFeedbackPacket(
     rtcp::TransportFeedback* feedback_packet) {
-  rtc::CritScope cs(&lock_);
-  if (window_start_seq_ == -1)
-    return false;
-
   // window_start_seq_ is the first sequence number to include in the current
   // feedback packet. Some older may still be in the map, in case a reordering
   // happens and we need to retransmit them.
-  auto it = packet_arrival_times_.find(window_start_seq_);
-  RTC_DCHECK(it != packet_arrival_times_.end());
+  rtc::CritScope cs(&lock_);
+  auto it = packet_arrival_times_.lower_bound(window_start_seq_);
+  if (it == packet_arrival_times_.end()) {
+    // Feedback for all packets already sent.
+    return false;
+  }
 
   // TODO(sprang): Measure receive times in microseconds and remove the
   // conversions below.
+  const int64_t first_sequence = it->first;
   feedback_packet->WithMediaSourceSsrc(media_ssrc_);
-  feedback_packet->WithBase(static_cast<uint16_t>(it->first & 0xFFFF),
+  // Base sequence is the expected next (window_start_seq_). This is known, but
+  // we might not have actually received it, so the base time shall be the time
+  // of the first received packet in the feedback.
+  feedback_packet->WithBase(static_cast<uint16_t>(window_start_seq_ & 0xFFFF),
                             it->second * 1000);
   feedback_packet->WithFeedbackSequenceNumber(feedback_sequence_++);
   for (; it != packet_arrival_times_.end(); ++it) {
@@ -135,19 +143,18 @@ bool RemoteEstimatorProxy::BuildFeedbackPacket(
             static_cast<uint16_t>(it->first & 0xFFFF), it->second * 1000)) {
       // If we can't even add the first seq to the feedback packet, we won't be
       // able to build it at all.
-      RTC_CHECK_NE(window_start_seq_, it->first);
+      RTC_CHECK_NE(first_sequence, it->first);
 
       // Could not add timestamp, feedback packet might be full. Return and
       // try again with a fresh packet.
-      window_start_seq_ = it->first;
       break;
     }
+
     // Note: Don't erase items from packet_arrival_times_ after sending, in case
     // they need to be re-sent after a reordering. Removal will be handled
     // by OnPacketArrival once packets are too old.
+    window_start_seq_ = it->first + 1;
   }
-  if (it == packet_arrival_times_.end())
-    window_start_seq_ = -1;
 
   return true;
 }

webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc

@@ -19,6 +19,7 @@
 using ::testing::_;
 using ::testing::InSequence;
 using ::testing::Invoke;
+using ::testing::Return;
 
 namespace webrtc {
 
@@ -107,6 +108,40 @@ TEST_F(RemoteEstimatorProxyTest, DuplicatedPackets) {
   Process();
 }
 
+TEST_F(RemoteEstimatorProxyTest, FeedbackWithMissingStart) {
+  // First feedback.
+  IncomingPacket(kBaseSeq, kBaseTimeMs);
+  IncomingPacket(kBaseSeq + 1, kBaseTimeMs + 1000);
+  EXPECT_CALL(router_, SendFeedback(_)).Times(1).WillOnce(Return(true));
+  Process();
+
+  // Second feedback starts with a missing packet (DROP kBaseSeq + 2).
+  IncomingPacket(kBaseSeq + 3, kBaseTimeMs + 3000);
+
+  EXPECT_CALL(router_, SendFeedback(_))
+      .Times(1)
+      .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) {
+        packet->Build();
+        EXPECT_EQ(kBaseSeq + 2, packet->GetBaseSequence());
+        EXPECT_EQ(kMediaSsrc, packet->GetMediaSourceSsrc());
+
+        std::vector<rtcp::TransportFeedback::StatusSymbol> status_vec =
+            packet->GetStatusVector();
+        EXPECT_EQ(2u, status_vec.size());
+        EXPECT_EQ(rtcp::TransportFeedback::StatusSymbol::kNotReceived,
+                  status_vec[0]);
+        EXPECT_EQ(rtcp::TransportFeedback::StatusSymbol::kReceivedSmallDelta,
+                  status_vec[1]);
+        std::vector<int64_t> delta_vec = packet->GetReceiveDeltasUs();
+        EXPECT_EQ(1u, delta_vec.size());
+        EXPECT_EQ(kBaseTimeMs + 3000,
+                  (packet->GetBaseTimeUs() + delta_vec[0]) / 1000);
+        return true;
+      }));
+
+  Process();
+}
+
 TEST_F(RemoteEstimatorProxyTest, SendsFeedbackWithVaryingDeltas) {
   IncomingPacket(kBaseSeq, kBaseTimeMs);
   IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs);

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

@@ -317,7 +317,11 @@ void TransportFeedback::WithBase(uint16_t base_sequence,
   RTC_DCHECK_EQ(-1, base_seq_);
   RTC_DCHECK_NE(-1, ref_timestamp_us);
   base_seq_ = base_sequence;
-  last_seq_ = base_sequence;
+  // last_seq_ is the sequence number of the last packed added _before_ a call
+  // to WithReceivedPacket(). Since the first sequence to be added is
+  // base_sequence, we need this to be one lower in order for potential missing
+  // packets to be populated properly.
+  last_seq_ = base_sequence - 1;
   base_time_ = ref_timestamp_us / kBaseScaleFactor;
   last_timestamp_ = base_time_ * kBaseScaleFactor;
 }

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

@@ -359,15 +359,18 @@ TEST(RtcpPacketTest, TransportFeedback_Limits) {
   // Sequence number wrap above 0x8000.
   std::unique_ptr<TransportFeedback> packet(new TransportFeedback());
   packet->WithBase(0, 0);
+  EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0));
   EXPECT_TRUE(packet->WithReceivedPacket(0x8000, 1000));
 
   packet.reset(new TransportFeedback());
   packet->WithBase(0, 0);
+  EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0));
   EXPECT_FALSE(packet->WithReceivedPacket(0x8000 + 1, 1000));
 
   // Packet status count max 0xFFFF.
   packet.reset(new TransportFeedback());
   packet->WithBase(0, 0);
+  EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0));
   EXPECT_TRUE(packet->WithReceivedPacket(0x8000, 1000));
   EXPECT_TRUE(packet->WithReceivedPacket(0xFFFF, 2000));
   EXPECT_FALSE(packet->WithReceivedPacket(0, 3000));