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@6594 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
index 31303c8..eeead40 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -603,6 +603,23 @@
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/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc
index fa84762..2f8b292 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc
@@ -155,6 +155,173 @@
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 @@
EXPECT_FALSE(IsRecoveryComplete());
}
-// TODO(marpan): Add more test cases.
-
void RtpFecTest::TearDown() {
fec_->ResetState(&recovered_packet_list_);
delete fec_;
diff --git a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc
index fc11fbe..83978e4 100644
--- a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc
+++ b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc
@@ -134,7 +134,7 @@
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 @@
}
// 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);
