From 92732ecc5ccc81e17c1dea75ecc5e34c7ff6274f Mon Sep 17 00:00:00 2001 From: brandtr Date: Mon, 5 Jun 2017 07:25:01 -0700 Subject: [PATCH] Revert of Only compare sequence numbers from the same SSRC in ForwardErrorCorrection. (patchset #5 id:120001 of https://codereview.webrtc.org/2893293003/ ) Reason for revert: Breaks fuzzer. Original issue's description: > Only compare sequence numbers from the same SSRC in ForwardErrorCorrection. > > Prior to this CL, the ForwardErrorCorrection state would be reset whenever > the difference in sequence numbers of the last recovered media packet > and the new packet (media or FEC) was too large. This comparison did not > take into account that FlexFEC uses a different SSRC for the FEC packets, > meaning that the the state would be reset very frequently when FlexFEC > is used. This should not have led to any major problems, except for a > decreased decoding efficiency. > > This CL verifies that whenever we compare sequence numbers in > ForwardErrorCorrection, they do indeed belong to the same SSRC. > > BUG=webrtc:5654 > > Review-Url: https://codereview.webrtc.org/2893293003 > Cr-Commit-Position: refs/heads/master@{#18399} > Committed: https://chromium.googlesource.com/external/webrtc/+/1476a9d789db03457595cf7dbea7e362972f2a4d TBR=stefan@webrtc.org,holmer@google.com # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2919313005 Cr-Commit-Position: refs/heads/master@{#18446} --- .../rtp_rtcp/source/fec_test_helper.cc | 6 +- .../modules/rtp_rtcp/source/fec_test_helper.h | 4 +- .../source/forward_error_correction.cc | 79 +++------- .../source/forward_error_correction.h | 14 +- .../rtp_rtcp/source/rtp_fec_unittest.cc | 148 +++++++----------- .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 2 - .../modules/rtp_rtcp/test/testFec/test_fec.cc | 51 ++---- 7 files changed, 110 insertions(+), 194 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc b/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc index 05655facac..74406db086 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc +++ b/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc @@ -78,7 +78,7 @@ ForwardErrorCorrection::PacketList MediaPacketGenerator::ConstructMediaPackets( RTC_DCHECK(media_packet); media_packet->data[1] |= 0x80; - next_seq_num_ = seq_num; + fec_seq_num_ = seq_num; return media_packets; } @@ -88,8 +88,8 @@ ForwardErrorCorrection::PacketList MediaPacketGenerator::ConstructMediaPackets( return ConstructMediaPackets(num_media_packets, random_->Rand()); } -uint16_t MediaPacketGenerator::GetNextSeqNum() { - return next_seq_num_; +uint16_t MediaPacketGenerator::GetFecSeqNum() { + return fec_seq_num_; } AugmentedPacketGenerator::AugmentedPacketGenerator(uint32_t ssrc) diff --git a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h index 1bd8261006..50594ec70e 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h +++ b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h @@ -49,7 +49,7 @@ class MediaPacketGenerator { ForwardErrorCorrection::PacketList ConstructMediaPackets( int num_media_packets); - uint16_t GetNextSeqNum(); + uint16_t GetFecSeqNum(); private: uint32_t min_packet_size_; @@ -58,7 +58,7 @@ class MediaPacketGenerator { Random* random_; ForwardErrorCorrection::PacketList media_packets_; - uint16_t next_seq_num_; + uint16_t fec_seq_num_; }; // This class generates media packets with a certain structure of the payload. diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index 193629b0b9..8c35f4000f 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc @@ -54,11 +54,6 @@ template bool ForwardErrorCorrection::SortablePacket::LessThan::operator() ( const S& first, const T& second) { - // TODO(brandtr): Try to add a DCHECK here, verifying that - // first->ssrc == second->ssrc. Currently, it is hard to do so - // because we are unable to reliably set the ssrc member on the - // ProtectedPacket's. These always belong to the media SSRC, but - // we may not know the media SSRC when they are created. return IsNewerSequenceNumber(second->seq_num, first->seq_num); } @@ -342,7 +337,6 @@ void ForwardErrorCorrection::InsertMediaPacket( ReceivedPacket* received_packet) { // Search for duplicate packets. for (const auto& recovered_packet : *recovered_packets) { - RTC_DCHECK_EQ(received_packet->ssrc, recovered_packet->ssrc); if (received_packet->seq_num == recovered_packet->seq_num) { // Duplicate packet, no need to add to list. // Delete duplicate media packet data. @@ -355,7 +349,6 @@ void ForwardErrorCorrection::InsertMediaPacket( recovered_packet->was_recovered = false; // This media packet has already been passed on. recovered_packet->returned = true; - recovered_packet->ssrc = received_packet->ssrc; recovered_packet->seq_num = received_packet->seq_num; recovered_packet->pkt = received_packet->pkt; recovered_packet->pkt->length = received_packet->pkt->length; @@ -387,7 +380,6 @@ void ForwardErrorCorrection::InsertFecPacket( ReceivedPacket* received_packet) { // Check for duplicate. for (const auto& existing_fec_packet : received_fec_packets_) { - RTC_DCHECK_EQ(received_packet->ssrc, existing_fec_packet->ssrc); if (received_packet->seq_num == existing_fec_packet->seq_num) { // Delete duplicate FEC packet data. received_packet->pkt = nullptr; @@ -396,8 +388,8 @@ void ForwardErrorCorrection::InsertFecPacket( } std::unique_ptr fec_packet(new ReceivedFecPacket()); fec_packet->pkt = received_packet->pkt; - fec_packet->ssrc = received_packet->ssrc; fec_packet->seq_num = received_packet->seq_num; + fec_packet->ssrc = received_packet->ssrc; // Parse ULPFEC/FlexFEC header specific info. bool ret = fec_header_reader_->ReadFecHeader(fec_packet.get()); if (!ret) { @@ -427,6 +419,11 @@ void ForwardErrorCorrection::InsertFecPacket( AssignRecoveredPackets(recovered_packets, fec_packet.get()); // TODO(holmer): Consider replacing this with a binary search for the right // position, and then just insert the new packet. Would get rid of the sort. + // + // For correct decoding, |received_fec_packets_| does not necessarily + // need to be sorted by sequence number (see decoding algorithm in + // AttemptRecover()). By keeping it sorted we try to recover the + // oldest lost packets first, however. received_fec_packets_.push_back(std::move(fec_packet)); received_fec_packets_.sort(SortablePacket::LessThan()); const size_t max_fec_packets = fec_header_reader_->MaxFecPackets(); @@ -470,31 +467,19 @@ void ForwardErrorCorrection::InsertPackets( while (!received_packets->empty()) { ReceivedPacket* received_packet = received_packets->front().get(); - // Discard old FEC packets such that the sequence numbers in - // |received_fec_packets_| span at most 1/2 of the sequence number space. - // This is important for keeping |received_fec_packets_| sorted, and may - // also reduce the possibility of incorrect decoding due to sequence number - // wrap-around. + // 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 (!received_fec_packets_.empty() && - received_packet->ssrc == received_fec_packets_.front()->ssrc) { - // It only makes sense to detect wrap-around when |received_packet| - // and |front_received_fec_packet| belong to the same sequence number - // space, i.e., the same SSRC. This happens when |received_packet| - // is a FEC packet, or if |received_packet| is a media packet and - // RED+ULPFEC is used. - auto it = received_fec_packets_.begin(); - while (it != received_fec_packets_.end()) { - uint16_t seq_num_diff = abs(static_cast(received_packet->seq_num) - - static_cast((*it)->seq_num)); - if (seq_num_diff > 0x3fff) { - it = received_fec_packets_.erase(it); - } else { - // No need to keep iterating, since |received_fec_packets_| is sorted. - break; - } + if (!received_fec_packets_.empty()) { + uint16_t seq_num_diff = + abs(static_cast(received_packet->seq_num) - + static_cast(received_fec_packets_.front()->seq_num)); + if (seq_num_diff > 0x3fff) { + received_fec_packets_.pop_front(); } } @@ -564,7 +549,6 @@ bool ForwardErrorCorrection::FinishPacketRecovery( // Set the SSRC field. ByteWriter::WriteBigEndian(&recovered_packet->pkt->data[8], fec_packet.protected_ssrc); - recovered_packet->ssrc = fec_packet.protected_ssrc; return true; } @@ -700,34 +684,21 @@ uint32_t ForwardErrorCorrection::ParseSsrc(uint8_t* packet) { int ForwardErrorCorrection::DecodeFec( ReceivedPacketList* received_packets, RecoveredPacketList* recovered_packets) { - RTC_DCHECK(received_packets); - RTC_DCHECK(recovered_packets); - + // TODO(marpan/ajm): can we check for multiple ULP headers, and return an + // error? const size_t max_media_packets = fec_header_reader_->MaxMediaPackets(); if (recovered_packets->size() == max_media_packets) { - const RecoveredPacket* back_recovered_packet = - recovered_packets->back().get(); - for (const auto& received_packet : *received_packets) { - if (received_packet->ssrc == back_recovered_packet->ssrc) { - const unsigned int seq_num_diff = - abs(static_cast(received_packet->seq_num) - - static_cast(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. - LOG(LS_INFO) << "Big gap in media/ULPFEC sequence numbers. No need " - "to keep the old packets in the FEC buffers, thus " - "resetting them."; - ResetState(recovered_packets); - break; - } - } + const unsigned int seq_num_diff = + abs(static_cast(received_packets->front()->seq_num) - + static_cast(recovered_packets->back()->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. + ResetState(recovered_packets); } } - InsertPackets(received_packets, recovered_packets); AttemptRecovery(recovered_packets); - return 0; } diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h index e5a38b6c00..f45473cb1f 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h +++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h @@ -62,25 +62,29 @@ class ForwardErrorCorrection { class SortablePacket { public: // Functor which returns true if the sequence number of |first| - // is < the sequence number of |second|. Should only ever be called for - // packets belonging to the same SSRC. + // is < the sequence number of |second|. struct LessThan { template bool operator() (const S& first, const T& second); }; - uint32_t ssrc; uint16_t seq_num; }; // The received list parameter of DecodeFec() references structs of this type. // + // The ssrc member is needed to ensure that we can restore the SSRC field of + // recovered packets. In most situations this could be retrieved from other + // media packets, but in the case of an FEC packet protecting a single + // missing media packet, we have no other means of obtaining it. // TODO(holmer): Refactor into a proper class. class ReceivedPacket : public SortablePacket { public: ReceivedPacket(); ~ReceivedPacket(); + uint32_t ssrc; // SSRC of the current frame. Must be set for FEC + // packets, but not required for media packets. bool is_fec; // Set to true if this is an FEC packet and false // otherwise. rtc::scoped_refptr pkt; // Pointer to the packet storage. @@ -105,7 +109,7 @@ class ForwardErrorCorrection { // Used to link media packets to their protecting FEC packets. // // TODO(holmer): Refactor into a proper class. - class ProtectedPacket : public SortablePacket { + class ProtectedPacket : public ForwardErrorCorrection::SortablePacket { public: ProtectedPacket(); ~ProtectedPacket(); @@ -118,7 +122,7 @@ class ForwardErrorCorrection { // Used for internal storage of received FEC packets in a list. // // TODO(holmer): Refactor into a proper class. - class ReceivedFecPacket : public SortablePacket { + class ReceivedFecPacket : public ForwardErrorCorrection::SortablePacket { public: ReceivedFecPacket(); ~ReceivedFecPacket(); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc index 448f3d0c88..a1c60c8d16 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc @@ -108,8 +108,7 @@ void RtpFecTest::ReceivedPackets( const PacketListType& packet_list, int* loss_mask, bool is_fec) { - uint16_t fec_seq_num = ForwardErrorCorrectionType::GetFirstFecSeqNum( - media_packet_generator_.GetNextSeqNum()); + uint16_t fec_seq_num = media_packet_generator_.GetFecSeqNum(); int packet_idx = 0; for (const auto& packet : packet_list) { @@ -121,17 +120,19 @@ void RtpFecTest::ReceivedPackets( memcpy(received_packet->pkt->data, packet->data, packet->length); received_packet->is_fec = is_fec; if (!is_fec) { - received_packet->ssrc = kMediaSsrc; - // For media packets, the sequence number is obtained from the - // RTP header as written by MediaPacketGenerator::ConstructMediaPackets. + // For media packets, the sequence number and marker bit is + // obtained from RTP header. These were set in ConstructMediaPackets(). received_packet->seq_num = ByteReader::ReadBigEndian(&packet->data[2]); } else { - received_packet->ssrc = ForwardErrorCorrectionType::kFecSsrc; - // For FEC packets, we simulate the sequence numbers differently - // depending on if ULPFEC or FlexFEC is used. See the definition of - // ForwardErrorCorrectionType::GetFirstFecSeqNum. + // The sequence number, marker bit, and ssrc number are defined in the + // RTP header of the FEC packet, which is not constructed in this test. + // So we set these values below based on the values generated in + // ConstructMediaPackets(). received_packet->seq_num = fec_seq_num; + // The ssrc value for FEC packets is set to the one used for the + // media packets in ConstructMediaPackets(). + received_packet->ssrc = kMediaSsrc; } received_packets_.push_back(std::move(received_packet)); } @@ -176,35 +177,18 @@ bool RtpFecTest::IsRecoveryComplete() { class FlexfecForwardErrorCorrection : public ForwardErrorCorrection { public: - static const uint32_t kFecSsrc = kFlexfecSsrc; - FlexfecForwardErrorCorrection() : ForwardErrorCorrection( std::unique_ptr(new FlexfecHeaderReader()), std::unique_ptr(new FlexfecHeaderWriter())) {} - - // For FlexFEC we let the FEC packet sequence numbers be independent of - // the media packet sequence numbers. - static uint16_t GetFirstFecSeqNum(uint16_t next_media_seq_num) { - Random random(0xbe110); - return random.Rand(); - } }; class UlpfecForwardErrorCorrection : public ForwardErrorCorrection { public: - static const uint32_t kFecSsrc = kMediaSsrc; - UlpfecForwardErrorCorrection() : ForwardErrorCorrection( std::unique_ptr(new UlpfecHeaderReader()), std::unique_ptr(new UlpfecHeaderWriter())) {} - - // For ULPFEC we assume that the FEC packets are subsequent to the media - // packets in terms of sequence number. - static uint16_t GetFirstFecSeqNum(uint16_t next_media_seq_num) { - return next_media_seq_num; - } }; using FecTypes = @@ -375,14 +359,13 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameRecovery) { EXPECT_TRUE(this->IsRecoveryComplete()); } -// Sequence number wrap occurs within the ULPFEC packets for the frame. -// In this case we will discard ULPFEC packet and full recovery is not expected. -// Same problem will occur if wrap is within media packets but ULPFEC packet is +// 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 ULPFEC packets. +// is used to detect old FEC packets. // TODO(marpan): Update test if wrap-around handling changes in FEC decoding. -using RtpFecTestUlpfecOnly = RtpFecTest; -TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { +TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { constexpr int kNumImportantPackets = 0; constexpr bool kUseUnequalProtection = false; constexpr uint8_t kProtectionFactor = 200; @@ -415,61 +398,8 @@ TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { &this->recovered_packets_)); // The two FEC packets are received and should allow for complete recovery, - // but because of the wrap the first FEC packet will be discarded, and only - // one media packet is recoverable. So expect 2 media packets on recovered - // list and no complete recovery. - EXPECT_EQ(2u, this->recovered_packets_.size()); - EXPECT_TRUE(this->recovered_packets_.size() != this->media_packets_.size()); - EXPECT_FALSE(this->IsRecoveryComplete()); -} - -// TODO(brandtr): This test mimics the one above, ensuring that the recovery -// strategy of FlexFEC matches the recovery strategy of ULPFEC. Since FlexFEC -// does not share the sequence number space with the media, however, having a -// matching recovery strategy may be suboptimal. Study this further. -using RtpFecTestFlexfecOnly = RtpFecTest; -TEST_F(RtpFecTestFlexfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { - constexpr int kNumImportantPackets = 0; - constexpr bool kUseUnequalProtection = false; - constexpr 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). - this->media_packets_ = - this->media_packet_generator_.ConstructMediaPackets(3, 65532); - - EXPECT_EQ( - 0, this->fec_.EncodeFec(this->media_packets_, kProtectionFactor, - kNumImportantPackets, kUseUnequalProtection, - kFecMaskBursty, &this->generated_fec_packets_)); - - // Expect 2 FEC packets. - EXPECT_EQ(2u, this->generated_fec_packets_.size()); - - // Overwrite the sequence numbers generated by ConstructMediaPackets, - // to make sure that we do have a wrap. - auto it = this->generated_fec_packets_.begin(); - ByteWriter::WriteBigEndian(&(*it)->data[2], 65535); - ++it; - ByteWriter::WriteBigEndian(&(*it)->data[2], 0); - - // Lose the last two media packets (seq# 65533, 65534). - memset(this->media_loss_mask_, 0, sizeof(this->media_loss_mask_)); - memset(this->fec_loss_mask_, 0, sizeof(this->fec_loss_mask_)); - this->media_loss_mask_[1] = 1; - this->media_loss_mask_[2] = 1; - this->ReceivedPackets(this->media_packets_, this->media_loss_mask_, false); - this->ReceivedPackets(this->generated_fec_packets_, this->fec_loss_mask_, - true); - - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); - - // The two FEC packets are received and should allow for complete recovery, - // but because of the wrap the first FEC packet will be discarded, and only - // one media packet is recoverable. So expect 2 media packets on recovered + // 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(2u, this->recovered_packets_.size()); EXPECT_TRUE(this->recovered_packets_.size() != this->media_packets_.size()); @@ -504,9 +434,9 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithMediaOutOfOrder) { // Reorder received media packets. auto it0 = this->received_packets_.begin(); - auto it1 = this->received_packets_.begin(); - it1++; - std::swap(*it0, *it1); + auto it2 = this->received_packets_.begin(); + it2++; + std::swap(*it0, *it2); EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, &this->recovered_packets_)); @@ -1028,4 +958,42 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) { EXPECT_FALSE(this->IsRecoveryComplete()); } +// 'using' directive needed for compiler to be happy. +using RtpFecTestWithFlexfec = RtpFecTest; +TEST_F(RtpFecTestWithFlexfec, + FecRecoveryWithLossAndDifferentMediaAndFlexfecSsrcs) { + constexpr int kNumImportantPackets = 0; + constexpr bool kUseUnequalProtection = false; + constexpr int kNumMediaPackets = 4; + constexpr uint8_t kProtectionFactor = 60; + + media_packets_ = + media_packet_generator_.ConstructMediaPackets(kNumMediaPackets); + + EXPECT_EQ(0, fec_.EncodeFec(media_packets_, kProtectionFactor, + kNumImportantPackets, kUseUnequalProtection, + kFecMaskBursty, &generated_fec_packets_)); + + // Expect 1 FEC packet. + EXPECT_EQ(1u, generated_fec_packets_.size()); + + // 1 media packet lost + memset(media_loss_mask_, 0, sizeof(media_loss_mask_)); + memset(fec_loss_mask_, 0, sizeof(fec_loss_mask_)); + media_loss_mask_[3] = 1; + NetworkReceivedPackets(media_loss_mask_, fec_loss_mask_); + + // Simulate FlexFEC packet received on different SSRC. + auto it = received_packets_.begin(); + ++it; + ++it; + ++it; // Now at the FEC packet. + (*it)->ssrc = kFlexfecSsrc; + + EXPECT_EQ(0, fec_.DecodeFec(&received_packets_, &recovered_packets_)); + + // One packet lost, one FEC packet, expect complete recovery. + EXPECT_TRUE(IsRecoveryComplete()); +} + } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index a0c63c07f9..dad445dab0 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -90,7 +90,6 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( // Get payload type from RED header and sequence number from RTP header. uint8_t payload_type = incoming_rtp_packet[header.headerLength] & 0x7f; received_packet->is_fec = payload_type == ulpfec_payload_type; - received_packet->ssrc = header.ssrc; received_packet->seq_num = header.sequenceNumber; uint16_t block_length = 0; @@ -156,7 +155,6 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( second_received_packet->pkt = new ForwardErrorCorrection::Packet; second_received_packet->is_fec = true; - second_received_packet->ssrc = header.ssrc; second_received_packet->seq_num = header.sequenceNumber; ++packet_counter_.num_fec_packets; diff --git a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc index 922fe2f9a5..4aaae98852 100644 --- a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc +++ b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc @@ -78,7 +78,13 @@ void ReceivePackets( } } -void RunTest(bool use_flexfec) { +// Too slow to finish before timeout on iOS. See webrtc:4755. +#if defined(WEBRTC_IOS) +#define MAYBE_FecTest DISABLED_FecTest +#else +#define MAYBE_FecTest FecTest +#endif +TEST(FecTest, MAYBE_FecTest) { // TODO(marpan): Split this function into subroutines/helper functions. enum { kMaxNumberMediaPackets = 48 }; enum { kMaxNumberFecPackets = 48 }; @@ -101,13 +107,8 @@ void RunTest(bool use_flexfec) { ASSERT_EQ(12, kMaxMediaPackets[1]) << "Max media packets for bursty mode not " << "equal to 12."; - std::unique_ptr fec; - if (use_flexfec) { - fec = ForwardErrorCorrection::CreateFlexfec(); - } else { - fec = ForwardErrorCorrection::CreateUlpfec(); - } - + std::unique_ptr fec = + ForwardErrorCorrection::CreateUlpfec(); ForwardErrorCorrection::PacketList media_packet_list; std::list fec_packet_list; ForwardErrorCorrection::ReceivedPacketList to_decode_list; @@ -137,16 +138,7 @@ void RunTest(bool use_flexfec) { uint16_t seq_num = 0; uint32_t timestamp = random.Rand(); - const uint32_t media_ssrc = random.Rand(1u, 0xfffffffe); - uint32_t fec_ssrc; - uint16_t fec_seq_num_offset; - if (use_flexfec) { - fec_ssrc = random.Rand(1u, 0xfffffffe); - fec_seq_num_offset = random.Rand(); - } else { - fec_ssrc = media_ssrc; - fec_seq_num_offset = 0; - } + const uint32_t ssrc = random.Rand(1u, 0xfffffffe); // Loop over the mask types: random and bursty. for (int mask_type_idx = 0; mask_type_idx < kNumFecMaskTypes; @@ -276,7 +268,7 @@ void RunTest(bool use_flexfec) { ByteWriter::WriteBigEndian(&media_packet->data[4], timestamp); ByteWriter::WriteBigEndian(&media_packet->data[8], - media_ssrc); + ssrc); // Generate random values for payload for (size_t j = 12; j < media_packet->length; ++j) { media_packet->data[j] = random.Rand(); @@ -310,7 +302,6 @@ void RunTest(bool use_flexfec) { received_packet->pkt->length = media_packet->length; memcpy(received_packet->pkt->data, media_packet->data, media_packet->length); - received_packet->ssrc = media_ssrc; received_packet->seq_num = ByteReader::ReadBigEndian(&media_packet->data[2]); received_packet->is_fec = false; @@ -332,9 +323,9 @@ void RunTest(bool use_flexfec) { received_packet->pkt->length = fec_packet->length; memcpy(received_packet->pkt->data, fec_packet->data, fec_packet->length); - received_packet->seq_num = fec_seq_num_offset + seq_num; + received_packet->seq_num = seq_num; received_packet->is_fec = true; - received_packet->ssrc = fec_ssrc; + received_packet->ssrc = ssrc; received_packet_list.push_back(std::move(received_packet)); fec_mask_list.push_back(fec_packet_masks[fec_packet_idx]); @@ -462,21 +453,5 @@ void RunTest(bool use_flexfec) { << "Recovered packet list is not empty"; } -// Too slow to finish before timeout on iOS. See webrtc:4755. -#if defined(WEBRTC_IOS) -#define MAYBE_UlpecTest DISABLED_UlpecTest -#define MAYBE_FlexfecTest DISABLED_FlexfecTest -#else -#define MAYBE_UlpecTest UlpecTest -#define MAYBE_FlexfecTest FlexfecTest -#endif -TEST(FecTest, MAYBE_UlpecTest) { - RunTest(false); -} - -TEST(FecTest, MAYBE_FlexfecTest) { - RunTest(true); -} - } // namespace test } // namespace webrtc