From 6ef4af95468ffa8d6764e52c40d398b64d000db2 Mon Sep 17 00:00:00 2001 From: Harsh Maniar Date: Mon, 12 Apr 2021 12:49:37 -0700 Subject: [PATCH] Purge old FEC packets from receiver's queue before media sequence numbers wrap around MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Reviewed-by: Åsa Persson Commit-Queue: Rasmus Brandt Cr-Commit-Position: refs/heads/master@{#33795} --- .../source/flexfec_receiver_unittest.cc | 54 +++++++++++++++++++ .../source/forward_error_correction.cc | 31 ++++++++--- .../source/forward_error_correction.h | 5 ++ 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc b/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc index b9391eeb74..dee5da080c 100644 --- a/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc +++ b/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 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::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 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 diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc index 56eabc8a7f..da8025d3db 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/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) { - // Either all protected packets arrived or have been recovered. We can - // discard this FEC packet. + } else if (packets_missing == 0 || + IsOldFecPacket(**fec_packet_it, recovered_packets)) { + // 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]; } diff --git a/modules/rtp_rtcp/source/forward_error_correction.h b/modules/rtp_rtcp/source/forward_error_correction.h index 0c54ad984c..b97693d01f 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.h +++ b/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_;