Purge old FEC packets from receiver's queue before media sequence numbers wrap around

- Received FEC packets are purged from the queue if:
  1. All media packets protected by the FEC packet are received.
  2. All media packets protected by the FEC packet are recovered.
  3. Newer FEC packet(s) with sequence number '0x3fff' larger than an old FEC packet is received.
- When FEC packets get separated from their protected media packets by more than 48, none of the first conditions ever delete that FEC packet, no matter how old/ irrelevant it gets.
- Under specific circumstances, the new FEC packet (condition 3) is not received before the media sequence number space wraps around, and incorrectly activates the old FEC packet, resulting in FEC decode for the wrong packet.
- This change purges such old FEC packets in time before the media sequence numbers wrap around.

Bug: webrtc:12656
Change-Id: I6ddf5382638c8c7e9a65724b2544dfbbc4803342
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215100
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33795}

modules/rtp_rtcp/source/flexfec_receiver_unittest.cc

@@ -646,4 +646,58 @@ TEST_F(FlexfecReceiverTest, CalculatesNumberOfPackets) {
   EXPECT_EQ(1U, packet_counter.num_recovered_packets);
 }
 
+TEST_F(FlexfecReceiverTest, DoesNotDecodeWrappedMediaSequenceUsingOldFec) {
+  const size_t kFirstFrameNumMediaPackets = 2;
+  const size_t kFirstFrameNumFecPackets = 1;
+
+  PacketList media_packets;
+  PacketizeFrame(kFirstFrameNumMediaPackets, 0, &media_packets);
+
+  // Protect first frame (sequences 0 and 1) with 1 FEC packet.
+  std::list<Packet*> fec_packets =
+      EncodeFec(media_packets, kFirstFrameNumFecPackets);
+
+  // Generate enough media packets to simulate media sequence number wraparound.
+  // Use no FEC for these frames to make sure old FEC is not purged due to age.
+  const size_t kNumFramesSequenceWrapAround =
+      std::numeric_limits<uint16_t>::max();
+  const size_t kNumMediaPacketsPerFrame = 1;
+
+  for (size_t i = 1; i <= kNumFramesSequenceWrapAround; ++i) {
+    PacketizeFrame(kNumMediaPacketsPerFrame, i, &media_packets);
+  }
+
+  // Receive first (|kFirstFrameNumMediaPackets| + 48) media packets.
+  // Simulate an old FEC packet by separating it from its encoded media
+  // packets by at least 48 packets.
+  auto media_it = media_packets.begin();
+  for (size_t i = 0; i < (kFirstFrameNumMediaPackets + 48); i++) {
+    if (i == 1) {
+      // Drop the second packet of the first frame.
+      media_it++;
+    } else {
+      receiver_.OnRtpPacket(ParsePacket(**media_it++));
+    }
+  }
+
+  // Receive FEC packet. Although a protected packet was dropped,
+  // expect no recovery callback since it is delayed from first frame
+  // by more than 48 packets.
+  auto fec_it = fec_packets.begin();
+  std::unique_ptr<Packet> fec_packet_with_rtp_header =
+      packet_generator_.BuildFlexfecPacket(**fec_it);
+  receiver_.OnRtpPacket(ParsePacket(*fec_packet_with_rtp_header));
+
+  // Receive remaining media packets.
+  // NOTE: Because we sent enough to simulate wrap around, sequence 0 is
+  // received again, but is a different packet than the original first
+  // packet of first frame.
+  while (media_it != media_packets.end()) {
+    receiver_.OnRtpPacket(ParsePacket(**media_it++));
+  }
+
+  // Do not expect a recovery callback, the FEC packet is old
+  // and should not decode wrapped around media sequences.
+}
+
 }  // namespace webrtc

modules/rtp_rtcp/source/forward_error_correction.cc

@@ -31,6 +31,8 @@ namespace webrtc {
 namespace {
 // Transport header size in bytes. Assume UDP/IPv4 as a reasonable minimum.
 constexpr size_t kTransportOverhead = 28;
+
+constexpr uint16_t kOldSequenceThreshold = 0x3fff;
 }  // namespace
 
 ForwardErrorCorrection::Packet::Packet() : data(0), ref_count_(0) {}
@@ -508,9 +510,6 @@ void ForwardErrorCorrection::InsertPacket(
   // This is important for keeping |received_fec_packets_| sorted, and may
   // also reduce the possibility of incorrect decoding due to sequence number
   // wrap-around.
-  // TODO(marpan/holmer): We should be able to improve detection/discarding of
-  // old FEC packets based on timestamp information or better sequence number
-  // thresholding (e.g., to distinguish between wrap-around and reordering).
   if (!received_fec_packets_.empty() &&
       received_packet.ssrc == received_fec_packets_.front()->ssrc) {
     // It only makes sense to detect wrap-around when |received_packet|
@@ -521,7 +520,7 @@ void ForwardErrorCorrection::InsertPacket(
     auto it = received_fec_packets_.begin();
     while (it != received_fec_packets_.end()) {
       uint16_t seq_num_diff = MinDiff(received_packet.seq_num, (*it)->seq_num);
-      if (seq_num_diff > 0x3fff) {
+      if (seq_num_diff > kOldSequenceThreshold) {
         it = received_fec_packets_.erase(it);
       } else {
         // No need to keep iterating, since |received_fec_packets_| is sorted.
@@ -698,9 +697,10 @@ void ForwardErrorCorrection::AttemptRecovery(
       // this may allow additional packets to be recovered.
       // Restart for first FEC packet.
       fec_packet_it = received_fec_packets_.begin();
-    } else if (packets_missing == 0) {
+    } else if (packets_missing == 0 ||
-      // Either all protected packets arrived or have been recovered. We can
+               IsOldFecPacket(**fec_packet_it, recovered_packets)) {
-      // discard this FEC packet.
+      // Either all protected packets arrived or have been recovered, or the FEC
+      // packet is old. We can discard this FEC packet.
       fec_packet_it = received_fec_packets_.erase(fec_packet_it);
     } else {
       fec_packet_it++;
@@ -731,6 +731,23 @@ void ForwardErrorCorrection::DiscardOldRecoveredPackets(
   RTC_DCHECK_LE(recovered_packets->size(), max_media_packets);
 }
 
+bool ForwardErrorCorrection::IsOldFecPacket(
+    const ReceivedFecPacket& fec_packet,
+    const RecoveredPacketList* recovered_packets) {
+  if (recovered_packets->empty()) {
+    return false;
+  }
+
+  const uint16_t back_recovered_seq_num = recovered_packets->back()->seq_num;
+  const uint16_t last_protected_seq_num =
+      fec_packet.protected_packets.back()->seq_num;
+
+  // FEC packet is old if its last protected sequence number is much
+  // older than the latest protected sequence number received.
+  return (MinDiff(back_recovered_seq_num, last_protected_seq_num) >
+          kOldSequenceThreshold);
+}
+
 uint16_t ForwardErrorCorrection::ParseSequenceNumber(const uint8_t* packet) {
   return (packet[2] << 8) + packet[3];
 }

modules/rtp_rtcp/source/forward_error_correction.h

@@ -330,6 +330,11 @@ class ForwardErrorCorrection {
   // for recovering lost packets.
   void DiscardOldRecoveredPackets(RecoveredPacketList* recovered_packets);
 
+  // Checks if the FEC packet is old enough and no longer relevant for
+  // recovering lost media packets.
+  bool IsOldFecPacket(const ReceivedFecPacket& fec_packet,
+                      const RecoveredPacketList* recovered_packets);
+
   // These SSRCs are only used by the decoder.
   const uint32_t ssrc_;
   const uint32_t protected_media_ssrc_;