From 958288a640d2636cf676e5988da50dc8f9d602da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 2 Oct 2017 13:51:36 +0200 Subject: [PATCH] Fix wrap-around logic in ForwardErrorCorrection. New function AbsSequenceNumberDifference. Bug: None Change-Id: I3906e3be313ec69973a20096c17c06e20448149d Reviewed-on: https://webrtc-review.googlesource.com/4384 Commit-Queue: Stefan Holmer Reviewed-by: Stefan Holmer Reviewed-by: Rasmus Brandt Cr-Commit-Position: refs/heads/master@{#20086} --- .../source/forward_error_correction.cc | 13 ++------ modules/rtp_rtcp/source/rtp_fec_unittest.cc | 30 ++++++++++++++----- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc index 3c1b1b4154..8dbe9d817c 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/modules/rtp_rtcp/source/forward_error_correction.cc @@ -23,6 +23,7 @@ #include "modules/rtp_rtcp/source/ulpfec_header_reader_writer.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" +#include "rtc_base/mod_ops.h" namespace webrtc { @@ -500,11 +501,7 @@ void ForwardErrorCorrection::InsertPacket( // RED+ULPFEC is used. auto it = received_fec_packets_.begin(); while (it != received_fec_packets_.end()) { - // TODO(nisse): This handling of wraparound appears broken, should be - // static_cast( - // received_packet.seq_num - back_recovered_packet->seq_num) - uint16_t seq_num_diff = abs(static_cast(received_packet.seq_num) - - static_cast((*it)->seq_num)); + uint16_t seq_num_diff = MinDiff(received_packet.seq_num, (*it)->seq_num); if (seq_num_diff > 0x3fff) { it = received_fec_packets_.erase(it); } else { @@ -720,12 +717,8 @@ void ForwardErrorCorrection::DecodeFec(const ReceivedPacket& received_packet, recovered_packets->back().get(); if (received_packet.ssrc == back_recovered_packet->ssrc) { - // TODO(nisse): This handling of wraparound appears broken, should be - // static_cast( - // received_packet.seq_num - back_recovered_packet->seq_num) const unsigned int seq_num_diff = - abs(static_cast(received_packet.seq_num) - - static_cast(back_recovered_packet->seq_num)); + MinDiff(received_packet.seq_num, back_recovered_packet->seq_num); if (seq_num_diff > max_media_packets) { // A big gap in sequence numbers. The old recovered packets // are now useless, so it's safe to do a reset. diff --git a/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/modules/rtp_rtcp/source/rtp_fec_unittest.cc index d01aa55994..606dafbbf2 100644 --- a/modules/rtp_rtcp/source/rtp_fec_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_fec_unittest.cc @@ -297,16 +297,18 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss) { } // Verify that we don't use an old FEC packet for FEC decoding. -TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapTwoFrames) { +TYPED_TEST(RtpFecTest, NoFecRecoveryWithOldFecPacket) { constexpr int kNumImportantPackets = 0; constexpr bool kUseUnequalProtection = false; constexpr 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 + // Third frame (new) with 3 media packets, and no FEC packets. + // + // #0(media) #1(media) #2(FEC) ----Frame 1----- + // #32767(media) 32768(media) 32769(media) ----Frame 2----- + // #65535(media) #0(media) #1(media). ----Frame 3----- + // If we lose either packet 0 or 1 of third frame, FEC decoding should not // try to decode using "old" FEC packet #2. // Construct media packets for first frame, starting at sequence number 0. @@ -326,6 +328,17 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapTwoFrames) { true); // Construct media packets for second frame, with sequence number wrap. + this->media_packets_ = + this->media_packet_generator_.ConstructMediaPackets(3, 32767); + + // Expect 3 media packets for this frame. + EXPECT_EQ(3u, this->media_packets_.size()); + + // No packets lost + memset(this->media_loss_mask_, 0, sizeof(this->media_loss_mask_)); + this->ReceivedPackets(this->media_packets_, this->media_loss_mask_, false); + + // Construct media packets for third frame, with sequence number wrap. this->media_packets_ = this->media_packet_generator_.ConstructMediaPackets(3, 65535); @@ -343,10 +356,11 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapTwoFrames) { &this->recovered_packets_); } - // Expect that no decoding is done to get missing packet (seq#0) of second + // Expect that no decoding is done to get missing packet (seq#0) of third // 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(2u, this->recovered_packets_.size()); + // recovered packets is 5 (0 from first frame, three from second frame, and 2 + // for the third frame, with no packets recovered via FEC). + EXPECT_EQ(5u, this->recovered_packets_.size()); EXPECT_TRUE(this->recovered_packets_.size() != this->media_packets_.size()); }