diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index 31303c8ad..eeead4074 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 @@ 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(rx_packet->seq_num) - + static_cast(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 fa847625e..2f8b29266 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 @@ 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(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(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(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(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(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(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(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(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(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/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc index fc11fbeec..83978e47b 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 @@ TEST(FecTest, FecTest) { fclose(randomSeedFile); randomSeedFile = NULL; - uint16_t seqNum = static_cast(rand()); + uint16_t seqNum = 0; uint32_t timeStamp = static_cast(rand()); const uint32_t ssrc = static_cast(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);