Fix for FEC decoding with sequence number wrap-around.

BUG=3507
R=stefan@webrtc.org
TBR=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/12879004

git-svn-id: http://webrtc.googlecode.com/svn/trunk/webrtc@6594 4adac7df-926f-26a2-2b94-8c16560cd09d

3 files changed, 190 insertions, 3 deletions

diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc
index 31303c8a..eeead407 100644
--- a/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -603,6 +603,23 @@ void ForwardErrorCorrection::InsertPackets(
   while (!received_packet_list->empty()) {
     ReceivedPacket* rx_packet = received_packet_list->front();
 
+    // Check for discarding oldest FEC packet, to avoid wrong FEC decoding from
+    // sequence number wrap-around. Detection of old FEC packet is based on
+    // sequence number difference of received packet and oldest packet in FEC
+    // packet list.
+    // 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 (!fec_packet_list_.empty()) {
+      uint16_t seq_num_diff = abs(
+          static_cast<int>(rx_packet->seq_num) -
+          static_cast<int>(fec_packet_list_.front()->seq_num));
+      if (seq_num_diff > 0x3fff) {
+        DiscardFECPacket(fec_packet_list_.front());
+        fec_packet_list_.pop_front();
+      }
+    }
+
     if (rx_packet->is_fec) {
       InsertFECPacket(rx_packet, recovered_packet_list);
     } else {
diff --git a/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/modules/rtp_rtcp/source/rtp_fec_unittest.cc
index fa847625..2f8b2926 100644
--- a/modules/rtp_rtcp/source/rtp_fec_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_fec_unittest.cc
@@ -155,6 +155,173 @@ TEST_F(RtpFecTest, FecRecoveryWithLoss) {
   EXPECT_FALSE(IsRecoveryComplete());
 }
 
+// Verify that we don't use an old FEC packet for FEC decoding.
+TEST_F(RtpFecTest, FecRecoveryWithSeqNumGapTwoFrames) {
+  const int kNumImportantPackets = 0;
+  const bool kUseUnequalProtection = false;
+  uint8_t kProtectionFactor = 20;
+
+  // Two frames: first frame (old) with two media packets and 1 FEC packet.
+  // Second frame (new) with 3 media packets, and no FEC packets.
+  //       ---Frame 1----                     ----Frame 2------
+  //  #0(media) #1(media) #2(FEC)     #65535(media) #0(media) #1(media).
+  // If we lose either packet 0 or 1 of second frame, FEC decoding should not
+  // try to decode using "old" FEC packet #2.
+
+  // Construct media packets for first frame, starting at sequence number 0.
+  fec_seq_num_ = ConstructMediaPacketsSeqNum(2, 0);
+
+  EXPECT_EQ(0, fec_->GenerateFEC(media_packet_list_, kProtectionFactor,
+                                 kNumImportantPackets, kUseUnequalProtection,
+                                 webrtc::kFecMaskBursty, &fec_packet_list_));
+  // Expect 1 FEC packet.
+  EXPECT_EQ(1, static_cast<int>(fec_packet_list_.size()));
+  // Add FEC packet (seq#2) of this first frame to received list (i.e., assume
+  // the two media packet were lost).
+  ReceivedPackets(fec_packet_list_, fec_loss_mask_, true);
+
+  // Construct media packets for second frame, with sequence number wrap.
+  ClearList(&media_packet_list_);
+  fec_seq_num_ = ConstructMediaPacketsSeqNum(3, 65535);
+
+  // Expect 3 media packets for this frame.
+  EXPECT_EQ(3, static_cast<int>(media_packet_list_.size()));
+
+  // Second media packet lost (seq#0).
+  memset(media_loss_mask_, 0, sizeof(media_loss_mask_));
+  media_loss_mask_[1] = 1;
+  // Add packets #65535, and #1 to received list.
+  ReceivedPackets(media_packet_list_, media_loss_mask_, false);
+
+  EXPECT_EQ(0,
+            fec_->DecodeFEC(&received_packet_list_, &recovered_packet_list_));
+
+  // Expect that no decoding is done to get missing packet (seq#0) of second
+  // frame, using old FEC packet (seq#2) from first (old) frame. So number of
+  // recovered packets is 2, and not equal to number of media packets (=3).
+  EXPECT_EQ(2, static_cast<int>(recovered_packet_list_.size()));
+  EXPECT_TRUE(recovered_packet_list_.size() != media_packet_list_.size());
+  FreeRecoveredPacketList();
+}
+
+// Verify we can still recovery frame if sequence number wrap occurs within
+// the frame and FEC packet following wrap is received after media packets.
+TEST_F(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameRecovery) {
+  const int kNumImportantPackets = 0;
+  const bool kUseUnequalProtection = false;
+  uint8_t kProtectionFactor = 20;
+
+  // One frame, with sequence number wrap in media packets.
+  //         -----Frame 1----
+  //  #65534(media) #65535(media) #0(media) #1(FEC).
+  fec_seq_num_ = ConstructMediaPacketsSeqNum(3, 65534);
+
+  EXPECT_EQ(0, fec_->GenerateFEC(media_packet_list_, kProtectionFactor,
+                                 kNumImportantPackets, kUseUnequalProtection,
+                                 webrtc::kFecMaskBursty, &fec_packet_list_));
+
+  // Expect 1 FEC packet.
+  EXPECT_EQ(1, static_cast<int>(fec_packet_list_.size()));
+
+  // Lose one media packet (seq# 65535).
+  memset(media_loss_mask_, 0, sizeof(media_loss_mask_));
+  memset(fec_loss_mask_, 0, sizeof(fec_loss_mask_));
+  media_loss_mask_[1] = 1;
+  ReceivedPackets(media_packet_list_, media_loss_mask_, false);
+  // Add FEC packet to received list following the media packets.
+  ReceivedPackets(fec_packet_list_, fec_loss_mask_, true);
+
+  EXPECT_EQ(0,
+            fec_->DecodeFEC(&received_packet_list_, &recovered_packet_list_));
+
+  // Expect 3 media packets in recovered list, and complete recovery.
+  // Wrap-around won't remove FEC packet, as it follows the wrap.
+  EXPECT_EQ(3, static_cast<int>(recovered_packet_list_.size()));
+  EXPECT_TRUE(IsRecoveryComplete());
+  FreeRecoveredPacketList();
+}
+
+// Sequence number wrap occurs within the FEC packets for the frame.
+// In this case we will discard FEC packet and full recovery is not expected.
+// Same problem will occur if wrap is within media packets but FEC packet is
+// received before the media packets. This may be improved if timing information
+// is used to detect old FEC packets.
+// TODO(marpan): Update test if wrap-around handling changes in FEC decoding.
+TEST_F(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameNoRecovery) {
+  const int kNumImportantPackets = 0;
+  const bool kUseUnequalProtection = false;
+  uint8_t kProtectionFactor = 200;
+
+  // 1 frame: 3 media packets and 2 FEC packets.
+  // Sequence number wrap in FEC packets.
+  //           -----Frame 1----
+  // #65532(media) #65533(media) #65534(media) #65535(FEC) #0(FEC).
+  fec_seq_num_ = ConstructMediaPacketsSeqNum(3, 65532);
+
+  EXPECT_EQ(0, fec_->GenerateFEC(media_packet_list_, kProtectionFactor,
+                                 kNumImportantPackets, kUseUnequalProtection,
+                                 webrtc::kFecMaskBursty, &fec_packet_list_));
+
+  // Expect 2 FEC packets.
+  EXPECT_EQ(2, static_cast<int>(fec_packet_list_.size()));
+
+  // Lose the last two media packets (seq# 65533, 65534).
+  memset(media_loss_mask_, 0, sizeof(media_loss_mask_));
+  memset(fec_loss_mask_, 0, sizeof(fec_loss_mask_));
+  media_loss_mask_[1] = 1;
+  media_loss_mask_[2] = 1;
+  ReceivedPackets(media_packet_list_, media_loss_mask_, false);
+  ReceivedPackets(fec_packet_list_, fec_loss_mask_, true);
+
+  EXPECT_EQ(0,
+            fec_->DecodeFEC(&received_packet_list_, &recovered_packet_list_));
+
+  // The two FEC packets are received and should allow for complete recovery,
+  // but because of the wrap the second FEC packet will be discarded, and only
+  // one media packet is recoverable. So exepct 2 media packets on recovered
+  // list and no complete recovery.
+  EXPECT_EQ(2, static_cast<int>(recovered_packet_list_.size()));
+  EXPECT_TRUE(recovered_packet_list_.size() != media_packet_list_.size());
+  EXPECT_FALSE(IsRecoveryComplete());
+  FreeRecoveredPacketList();
+}
+
+// Verify we can still recovery frame if FEC is received before media packets.
+TEST_F(RtpFecTest, FecRecoveryWithFecOutOfOrder) {
+  const int kNumImportantPackets = 0;
+  const bool kUseUnequalProtection = false;
+  uint8_t kProtectionFactor = 20;
+
+  // One frame: 3 media packets, 1 FEC packet.
+  //         -----Frame 1----
+  //  #0(media) #1(media) #2(media) #3(FEC).
+  fec_seq_num_ = ConstructMediaPacketsSeqNum(3, 0);
+
+  EXPECT_EQ(0, fec_->GenerateFEC(media_packet_list_, kProtectionFactor,
+                                 kNumImportantPackets, kUseUnequalProtection,
+                                 webrtc::kFecMaskBursty, &fec_packet_list_));
+
+  // Expect 1 FEC packet.
+  EXPECT_EQ(1, static_cast<int>(fec_packet_list_.size()));
+
+  // Lose one media packet (seq# 1).
+  memset(media_loss_mask_, 0, sizeof(media_loss_mask_));
+  memset(fec_loss_mask_, 0, sizeof(fec_loss_mask_));
+  media_loss_mask_[1] = 1;
+  // Add FEC packet to received list before the media packets.
+  ReceivedPackets(fec_packet_list_, fec_loss_mask_, true);
+  // Add media packets to received list.
+  ReceivedPackets(media_packet_list_, media_loss_mask_, false);
+
+  EXPECT_EQ(0,
+            fec_->DecodeFEC(&received_packet_list_, &recovered_packet_list_));
+
+  // Expect 3 media packets in recovered list, and complete recovery.
+  EXPECT_EQ(3, static_cast<int>(recovered_packet_list_.size()));
+  EXPECT_TRUE(IsRecoveryComplete());
+  FreeRecoveredPacketList();
+}
+
 // Test 50% protection with random mask type: Two cases are considered:
 // a 50% non-consecutive loss which can be fully recovered, and a 50%
 // consecutive loss which cannot be fully recovered.
@@ -625,8 +792,6 @@ TEST_F(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) {
   EXPECT_FALSE(IsRecoveryComplete());
 }
 
-// TODO(marpan): Add more test cases.
-
 void RtpFecTest::TearDown() {
   fec_->ResetState(&recovered_packet_list_);
   delete fec_;
diff --git a/modules/rtp_rtcp/test/testFec/test_fec.cc b/modules/rtp_rtcp/test/testFec/test_fec.cc
index fc11fbee..83978e47 100644
--- a/modules/rtp_rtcp/test/testFec/test_fec.cc
+++ b/modules/rtp_rtcp/test/testFec/test_fec.cc
@@ -134,7 +134,7 @@ TEST(FecTest, FecTest) {
   fclose(randomSeedFile);
   randomSeedFile = NULL;
 
-  uint16_t seqNum = static_cast<uint16_t>(rand());
+  uint16_t seqNum = 0;
   uint32_t timeStamp = static_cast<uint32_t>(rand());
   const uint32_t ssrc = static_cast<uint32_t>(rand());
 
@@ -224,6 +224,11 @@ TEST(FecTest, FecTest) {
             }
 
             // Construct media packets.
+            // Reset the sequence number here for each FEC code/mask tested
+            // below, to avoid sequence number wrap-around. In actual decoding,
+            // old FEC packets in list are dropped if sequence number wrap
+            // around is detected. This case is currently not handled below.
+            seqNum = 0;
             for (uint32_t i = 0; i < numMediaPackets; ++i) {
               mediaPacket = new ForwardErrorCorrection::Packet;
               mediaPacketList.push_back(mediaPacket);