From a5f043f9cd201fff209e96415bebcb8445ba0176 Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 18 Sep 2017 07:58:59 -0700 Subject: [PATCH] Change ForwardErrorCorrection class to accept one received packet at a time. BUG=None Review-Url: https://codereview.webrtc.org/3012243002 Cr-Commit-Position: refs/heads/master@{#19893} --- modules/rtp_rtcp/include/flexfec_receiver.h | 8 +- modules/rtp_rtcp/source/flexfec_receiver.cc | 29 ++- .../source/flexfec_receiver_unittest.cc | 44 ++-- .../source/forward_error_correction.cc | 149 +++++++------- .../source/forward_error_correction.h | 22 +- modules/rtp_rtcp/source/rtp_fec_unittest.cc | 193 ++++++++++++------ .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 15 +- .../rtp_rtcp/source/ulpfec_receiver_impl.h | 11 +- modules/rtp_rtcp/test/testFec/test_fec.cc | 21 +- 9 files changed, 281 insertions(+), 211 deletions(-) diff --git a/modules/rtp_rtcp/include/flexfec_receiver.h b/modules/rtp_rtcp/include/flexfec_receiver.h index 7355262a13..024d691bf9 100644 --- a/modules/rtp_rtcp/include/flexfec_receiver.h +++ b/modules/rtp_rtcp/include/flexfec_receiver.h @@ -39,8 +39,10 @@ class FlexfecReceiver { // Protected to aid testing. protected: - bool AddReceivedPacket(const RtpPacketReceived& packet); - bool ProcessReceivedPackets(); + std::unique_ptr AddReceivedPacket( + const RtpPacketReceived& packet); + void ProcessReceivedPacket( + const ForwardErrorCorrection::ReceivedPacket& received_packet); private: // Config. @@ -50,8 +52,6 @@ class FlexfecReceiver { // Erasure code interfacing and callback. std::unique_ptr erasure_code_ RTC_GUARDED_BY(sequence_checker_); - ForwardErrorCorrection::ReceivedPacketList received_packets_ - RTC_GUARDED_BY(sequence_checker_); ForwardErrorCorrection::RecoveredPacketList recovered_packets_ RTC_GUARDED_BY(sequence_checker_); RecoveredPacketReceiver* const recovered_packet_receiver_; diff --git a/modules/rtp_rtcp/source/flexfec_receiver.cc b/modules/rtp_rtcp/source/flexfec_receiver.cc index 5a625d89a2..ec35a2c063 100644 --- a/modules/rtp_rtcp/source/flexfec_receiver.cc +++ b/modules/rtp_rtcp/source/flexfec_receiver.cc @@ -48,10 +48,11 @@ FlexfecReceiver::~FlexfecReceiver() = default; void FlexfecReceiver::OnRtpPacket(const RtpPacketReceived& packet) { RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_); - if (!AddReceivedPacket(packet)) { + std::unique_ptr received_packet = AddReceivedPacket(packet); + if (!received_packet) return; - } - ProcessReceivedPackets(); + + ProcessReceivedPacket(*received_packet); } FecPacketCounter FlexfecReceiver::GetPacketCounter() const { @@ -61,7 +62,8 @@ FecPacketCounter FlexfecReceiver::GetPacketCounter() const { // TODO(eladalon): Consider using packet.recovered() to avoid processing // recovered packets here. -bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { +std::unique_ptr FlexfecReceiver::AddReceivedPacket( + const RtpPacketReceived& packet) { RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_); // RTP packets with a full base header (12 bytes), but without payload, @@ -77,7 +79,7 @@ bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { // This is a FlexFEC packet. if (packet.payload_size() < kMinFlexfecHeaderSize) { LOG(LS_WARNING) << "Truncated FlexFEC packet, discarding."; - return false; + return nullptr; } received_packet->is_fec = true; ++packet_counter_.num_fec_packets; @@ -93,7 +95,7 @@ bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { // This is a media packet, or a FlexFEC packet belonging to some // other FlexFEC stream. if (received_packet->ssrc != protected_media_ssrc_) { - return false; + return nullptr; } received_packet->is_fec = false; @@ -104,10 +106,9 @@ bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { received_packet->pkt->length = packet.size(); } - received_packets_.push_back(std::move(received_packet)); ++packet_counter_.num_packets; - return true; + return received_packet; } // Note that the implementation of this member function and the implementation @@ -119,16 +120,13 @@ bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { // Here, however, the received media pipeline is more decoupled from the // FlexFEC decoder, and we therefore do not interfere with the reception // of non-recovered media packets. -bool FlexfecReceiver::ProcessReceivedPackets() { +void FlexfecReceiver::ProcessReceivedPacket( + const ReceivedPacket& received_packet) { RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_); // Decode. - if (!received_packets_.empty()) { - if (erasure_code_->DecodeFec(&received_packets_, &recovered_packets_) != - 0) { - return false; - } - } + erasure_code_->DecodeFec(received_packet, &recovered_packets_); + // Return recovered packets through callback. for (const auto& recovered_packet : recovered_packets_) { if (recovered_packet->returned) { @@ -150,7 +148,6 @@ bool FlexfecReceiver::ProcessReceivedPackets() { last_recovered_packet_ms_ = now_ms; } } - return true; } } // namespace webrtc diff --git a/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc b/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc index ae2ec1dca5..1761e1d4c5 100644 --- a/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc @@ -54,7 +54,7 @@ class FlexfecReceiverForTest : public FlexfecReceiver { } // Expose methods for tests. using FlexfecReceiver::AddReceivedPacket; - using FlexfecReceiver::ProcessReceivedPackets; + using FlexfecReceiver::ProcessReceivedPacket; }; class FlexfecReceiverTest : public ::testing::Test { @@ -113,8 +113,10 @@ TEST_F(FlexfecReceiverTest, ReceivesMediaPacket) { std::unique_ptr media_packet( packet_generator_.NextPacket(0, kPayloadLength)); - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*media_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); } TEST_F(FlexfecReceiverTest, ReceivesMediaAndFecPackets) { @@ -127,10 +129,13 @@ TEST_F(FlexfecReceiverTest, ReceivesMediaAndFecPackets) { const auto& media_packet = media_packets.front(); auto fec_packet = packet_generator_.BuildFlexfecPacket(*fec_packets.front()); - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*fec_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*media_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); + received_packet = receiver_.AddReceivedPacket(ParsePacket(*fec_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); } TEST_F(FlexfecReceiverTest, FailsOnTruncatedFecPacket) { @@ -145,8 +150,10 @@ TEST_F(FlexfecReceiverTest, FailsOnTruncatedFecPacket) { fec_packets.front()->length = 1; auto fec_packet = packet_generator_.BuildFlexfecPacket(*fec_packets.front()); - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*media_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); EXPECT_FALSE(receiver_.AddReceivedPacket(ParsePacket(*fec_packet))); } @@ -180,8 +187,10 @@ TEST_F(FlexfecReceiverTest, FailsOnUnknownFecSsrc) { fec_packet->data[10] = 6; fec_packet->data[11] = 7; - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*media_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); EXPECT_FALSE(receiver_.AddReceivedPacket(ParsePacket(*fec_packet))); } @@ -195,17 +204,20 @@ TEST_F(FlexfecReceiverTest, ReceivesMultiplePackets) { // Receive all media packets. for (const auto& media_packet : media_packets) { - EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*media_packet)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); } // Receive FEC packet. auto fec_packet = fec_packets.front(); std::unique_ptr packet_with_rtp_header = packet_generator_.BuildFlexfecPacket(*fec_packet); - EXPECT_TRUE( - receiver_.AddReceivedPacket(ParsePacket(*packet_with_rtp_header))); - EXPECT_TRUE(receiver_.ProcessReceivedPackets()); + std::unique_ptr received_packet = + receiver_.AddReceivedPacket(ParsePacket(*packet_with_rtp_header)); + ASSERT_TRUE(received_packet); + receiver_.ProcessReceivedPacket(*received_packet); } TEST_F(FlexfecReceiverTest, RecoversFromSingleMediaLoss) { diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc index fe348b5e33..3c1b1b4154 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/modules/rtp_rtcp/source/forward_error_correction.cc @@ -342,16 +342,14 @@ void ForwardErrorCorrection::ResetState( void ForwardErrorCorrection::InsertMediaPacket( RecoveredPacketList* recovered_packets, - ReceivedPacket* received_packet) { - RTC_DCHECK_EQ(received_packet->ssrc, protected_media_ssrc_); + const ReceivedPacket& received_packet) { + RTC_DCHECK_EQ(received_packet.ssrc, protected_media_ssrc_); // Search for duplicate packets. for (const auto& recovered_packet : *recovered_packets) { - RTC_DCHECK_EQ(recovered_packet->ssrc, received_packet->ssrc); - if (recovered_packet->seq_num == received_packet->seq_num) { + RTC_DCHECK_EQ(recovered_packet->ssrc, received_packet.ssrc); + if (recovered_packet->seq_num == received_packet.seq_num) { // Duplicate packet, no need to add to list. - // Delete duplicate media packet data. - received_packet->pkt = nullptr; return; } } @@ -361,10 +359,10 @@ 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; + 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; // 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. RecoveredPacket* recovered_packet_ptr = recovered_packet.get(); @@ -390,23 +388,22 @@ void ForwardErrorCorrection::UpdateCoveringFecPackets( void ForwardErrorCorrection::InsertFecPacket( const RecoveredPacketList& recovered_packets, - ReceivedPacket* received_packet) { - RTC_DCHECK_EQ(received_packet->ssrc, ssrc_); + const ReceivedPacket& received_packet) { + RTC_DCHECK_EQ(received_packet.ssrc, ssrc_); // Check for duplicate. for (const auto& existing_fec_packet : received_fec_packets_) { - RTC_DCHECK_EQ(existing_fec_packet->ssrc, received_packet->ssrc); - if (existing_fec_packet->seq_num == received_packet->seq_num) { - // Delete duplicate FEC packet data. - received_packet->pkt = nullptr; + RTC_DCHECK_EQ(existing_fec_packet->ssrc, received_packet.ssrc); + if (existing_fec_packet->seq_num == received_packet.seq_num) { + // Drop duplicate FEC packet data. return; } } 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->pkt = received_packet.pkt; + fec_packet->ssrc = received_packet.ssrc; + fec_packet->seq_num = received_packet.seq_num; // Parse ULPFEC/FlexFEC header specific info. bool ret = fec_header_reader_->ReadFecHeader(fec_packet.get()); if (!ret) { @@ -483,49 +480,46 @@ void ForwardErrorCorrection::AssignRecoveredPackets( } } -void ForwardErrorCorrection::InsertPackets( - ReceivedPacketList* received_packets, +void ForwardErrorCorrection::InsertPacket( + const ReceivedPacket& received_packet, RecoveredPacketList* recovered_packets) { - 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. - // 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; - } + // 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. + // 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()) { + // 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)); + 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_packet->is_fec) { - InsertFecPacket(*recovered_packets, received_packet); - } else { - InsertMediaPacket(recovered_packets, received_packet); - } - // Delete the received packet "wrapper". - received_packets->pop_front(); } - RTC_DCHECK(received_packets->empty()); + + if (received_packet.is_fec) { + InsertFecPacket(*recovered_packets, received_packet); + } else { + InsertMediaPacket(recovered_packets, received_packet); + } + DiscardOldRecoveredPackets(recovered_packets); } @@ -716,38 +710,35 @@ uint32_t ForwardErrorCorrection::ParseSsrc(uint8_t* packet) { return (packet[8] << 24) + (packet[9] << 16) + (packet[10] << 8) + packet[11]; } -int ForwardErrorCorrection::DecodeFec( - ReceivedPacketList* received_packets, - RecoveredPacketList* recovered_packets) { - RTC_DCHECK(received_packets); +void ForwardErrorCorrection::DecodeFec(const ReceivedPacket& received_packet, + RecoveredPacketList* recovered_packets) { RTC_DCHECK(recovered_packets); 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; - } + + 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)); + 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); } } } - InsertPackets(received_packets, recovered_packets); + InsertPacket(received_packet, recovered_packets); AttemptRecovery(recovered_packets); - - return 0; } size_t ForwardErrorCorrection::MaxPacketOverhead() const { diff --git a/modules/rtp_rtcp/source/forward_error_correction.h b/modules/rtp_rtcp/source/forward_error_correction.h index f821f6f26d..97acb393f3 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.h +++ b/modules/rtp_rtcp/source/forward_error_correction.h @@ -75,9 +75,10 @@ class ForwardErrorCorrection { uint16_t seq_num; }; - // The received list parameter of DecodeFec() references structs of this type. + // Used for the input to DecodeFec(). // - // TODO(holmer): Refactor into a proper class. + // TODO(nisse): Delete class, instead passing |is_fec| and |pkt| as separate + // arguments. class ReceivedPacket : public SortablePacket { public: ReceivedPacket(); @@ -141,7 +142,6 @@ class ForwardErrorCorrection { }; using PacketList = std::list>; - using ReceivedPacketList = std::list>; using RecoveredPacketList = std::list>; using ReceivedFecPacketList = std::list>; @@ -218,10 +218,8 @@ class ForwardErrorCorrection { // list will be valid until the next call to // DecodeFec(). // - // Returns 0 on success, -1 on failure. - // - int DecodeFec(ReceivedPacketList* received_packets, - RecoveredPacketList* recovered_packets); + void DecodeFec(const ReceivedPacket& received_packet, + RecoveredPacketList* recovered_packets); // Get the number of generated FEC packets, given the number of media packets // and the protection factor. @@ -266,14 +264,14 @@ class ForwardErrorCorrection { uint32_t media_ssrc, uint16_t seq_num_base); - // Inserts the |received_packets| into the internal received FEC packet list + // Inserts the |received_packet| into the internal received FEC packet list // or into |recovered_packets|. - void InsertPackets(ReceivedPacketList* received_packets, - RecoveredPacketList* recovered_packets); + void InsertPacket(const ReceivedPacket& received_packet, + RecoveredPacketList* recovered_packets); // Inserts the |received_packet| into |recovered_packets|. Deletes duplicates. void InsertMediaPacket(RecoveredPacketList* recovered_packets, - ReceivedPacket* received_packet); + const ReceivedPacket& received_packet); // Assigns pointers to the recovered packet from all FEC packets which cover // it. @@ -284,7 +282,7 @@ class ForwardErrorCorrection { // Insert |received_packet| into internal FEC list. Deletes duplicates. void InsertFecPacket(const RecoveredPacketList& recovered_packets, - ReceivedPacket* received_packet); + const ReceivedPacket& received_packet); // Assigns pointers to already recovered packets covered by |fec_packet|. static void AssignRecoveredPackets( diff --git a/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/modules/rtp_rtcp/source/rtp_fec_unittest.cc index 5f84af501f..d01aa55994 100644 --- a/modules/rtp_rtcp/source/rtp_fec_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_fec_unittest.cc @@ -86,7 +86,8 @@ class RtpFecTest : public ::testing::Test { ForwardErrorCorrection::PacketList media_packets_; std::list generated_fec_packets_; - ForwardErrorCorrection::ReceivedPacketList received_packets_; + std::vector> + received_packets_; ForwardErrorCorrection::RecoveredPacketList recovered_packets_; int media_loss_mask_[kUlpfecMaxMediaPackets]; @@ -98,6 +99,7 @@ void RtpFecTest::NetworkReceivedPackets( int* media_loss_mask, int* fec_loss_mask) { constexpr bool kFecPacket = true; + this->received_packets_.clear(); ReceivedPackets(media_packets_, media_loss_mask, !kFecPacket); ReceivedPackets(generated_fec_packets_, fec_loss_mask, kFecPacket); } @@ -237,8 +239,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNoLoss) { memset(this->fec_loss_mask_, 0, sizeof(this->fec_loss_mask_)); this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // No packets lost, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -267,8 +271,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // One packet lost, one FEC packet, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -281,8 +287,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 2 packets lost, one FEC packet, cannot get complete recovery. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -330,8 +338,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapTwoFrames) { // Add packets #65535, and #1 to received list. this->ReceivedPackets(this->media_packets_, this->media_loss_mask_, false); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 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 @@ -370,8 +380,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameRecovery) { this->ReceivedPackets(this->generated_fec_packets_, this->fec_loss_mask_, true); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Expect 3 media packets in recovered list, and complete recovery. // Wrap-around won't remove FEC packet, as it follows the wrap. @@ -380,13 +392,18 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameRecovery) { } // 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 // received before the media packets. This may be improved if timing information // is used to detect old ULPFEC packets. -// TODO(marpan): Update test if wrap-around handling changes in FEC decoding. + +// TODO(nisse): There's some logic to discard ULPFEC packets at wrap-around, +// however, that is not actually exercised by this test: When the first FEC +// packet is processed, it results in full recovery of one media packet and the +// FEC packet is forgotten. And then the wraparound isn't noticed when the next +// FEC packet is received. We should fix wraparound handling, which currently +// appears broken, and then figure out how to test it properly. using RtpFecTestUlpfecOnly = RtpFecTest; -TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { +TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameRecovery) { constexpr int kNumImportantPackets = 0; constexpr bool kUseUnequalProtection = false; constexpr uint8_t kProtectionFactor = 200; @@ -415,22 +432,28 @@ TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { this->ReceivedPackets(this->generated_fec_packets_, this->fec_loss_mask_, true); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &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()); + EXPECT_EQ(3u, this->recovered_packets_.size()); + EXPECT_EQ(this->recovered_packets_.size(), this->media_packets_.size()); + EXPECT_TRUE(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. +// TODO(nisse): In this test, recovery based on the first FEC packet fails with +// the log message "The recovered packet had a length larger than a typical IP +// packet, and is thus dropped." This is probably not intended, and needs +// investigation. using RtpFecTestFlexfecOnly = RtpFecTest; TEST_F(RtpFecTestFlexfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { constexpr int kNumImportantPackets = 0; @@ -468,8 +491,10 @@ TEST_F(RtpFecTestFlexfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { this->ReceivedPackets(this->generated_fec_packets_, this->fec_loss_mask_, true); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &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 @@ -512,8 +537,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithMediaOutOfOrder) { it1++; std::swap(*it0, *it1); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Expect 3 media packets in recovered list, and complete recovery. EXPECT_EQ(3u, this->recovered_packets_.size()); @@ -550,8 +577,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithFecOutOfOrder) { // Add media packets to received list. this->ReceivedPackets(this->media_packets_, this->media_loss_mask_, false); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Expect 3 media packets in recovered list, and complete recovery. EXPECT_EQ(3u, this->recovered_packets_.size()); @@ -597,8 +626,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percRandomMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // With media packet#1 and FEC packets #1, #2, #3, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -613,8 +644,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percRandomMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Cannot get complete recovery for this loss configuration with random mask. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -659,8 +692,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percBurstyMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Expect complete recovery for consecutive packet loss <= 50%. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -675,8 +710,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percBurstyMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Expect complete recovery for consecutive packet loss <= 50%. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -691,8 +728,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percBurstyMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Cannot get complete recovery for this loss configuration. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -720,8 +759,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNoLossUep) { memset(this->fec_loss_mask_, 0, sizeof(this->fec_loss_mask_)); this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // No packets lost, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -750,8 +791,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLossUep) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // One packet lost, one FEC packet, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -764,8 +807,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLossUep) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 2 packets lost, one FEC packet, cannot get complete recovery. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -808,8 +853,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percUepRandomMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // With media packet#3 and FEC packets #0, #1, #3, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -825,8 +872,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithLoss50percUepRandomMask) { this->media_loss_mask_[3] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Cannot get complete recovery for this loss configuration. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -860,8 +909,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePackets) { this->media_loss_mask_[2] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // One packet lost, one FEC packet, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -873,8 +924,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePackets) { this->media_loss_mask_[1] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Unprotected packet lost. Recovery not possible. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -887,8 +940,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePackets) { this->media_loss_mask_[2] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 2 protected packets lost, one FEC packet, cannot get complete recovery. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -925,8 +980,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsExtension) { this->media_loss_mask_[kNumMediaPackets - 1] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // One packet lost, one FEC packet, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -938,8 +995,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsExtension) { this->media_loss_mask_[kNumMediaPackets - 2] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Unprotected packet lost. Recovery not possible. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -956,8 +1015,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsExtension) { this->media_loss_mask_[kNumMediaPackets - 1] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 5 protected packets lost, one FEC packet, cannot get complete recovery. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -994,8 +1055,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) { this->media_loss_mask_[kNumMediaPackets - 1] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // One packet lost, one FEC packet, expect complete recovery. EXPECT_TRUE(this->IsRecoveryComplete()); @@ -1007,8 +1070,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) { this->media_loss_mask_[kNumMediaPackets - 2] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // Unprotected packet lost. Recovery not possible. EXPECT_FALSE(this->IsRecoveryComplete()); @@ -1025,8 +1090,10 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) { this->media_loss_mask_[kNumMediaPackets - 1] = 1; this->NetworkReceivedPackets(this->media_loss_mask_, this->fec_loss_mask_); - EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, - &this->recovered_packets_)); + for (const auto& received_packet : this->received_packets_) { + this->fec_.DecodeFec(*received_packet, + &this->recovered_packets_); + } // 5 protected packets lost, one FEC packet, cannot get complete recovery. EXPECT_FALSE(this->IsRecoveryComplete()); diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index a35393d732..4292f3ca62 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -216,23 +216,22 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( return 0; } +// TODO(nisse): Drop always-zero return value. int32_t UlpfecReceiverImpl::ProcessReceivedFec() { crit_sect_.Enter(); - if (!received_packets_.empty()) { + for (const auto& received_packet : received_packets_) { // Send received media packet to VCM. - if (!received_packets_.front()->is_fec) { - ForwardErrorCorrection::Packet* packet = received_packets_.front()->pkt; + if (!received_packet->is_fec) { + ForwardErrorCorrection::Packet* packet = received_packet->pkt; crit_sect_.Leave(); recovered_packet_callback_->OnRecoveredPacket(packet->data, packet->length); crit_sect_.Enter(); } - if (fec_->DecodeFec(&received_packets_, &recovered_packets_) != 0) { - crit_sect_.Leave(); - return -1; - } - RTC_DCHECK(received_packets_.empty()); + fec_->DecodeFec(*received_packet, &recovered_packets_); } + received_packets_.clear(); + // Send any recovered media packets to VCM. for (const auto& recovered_packet : recovered_packets_) { if (recovered_packet->returned) { diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index 80257298bd..edc3d31269 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -12,6 +12,7 @@ #define MODULES_RTP_RTCP_SOURCE_ULPFEC_RECEIVER_IMPL_H_ #include +#include #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/include/ulpfec_receiver.h" @@ -41,10 +42,12 @@ class UlpfecReceiverImpl : public UlpfecReceiver { rtc::CriticalSection crit_sect_; RecoveredPacketReceiver* recovered_packet_callback_; std::unique_ptr fec_; - // TODO(holmer): In the current version |received_packets_| is never more - // than one packet, since we process FEC every time a new packet - // arrives. We should remove the list. - ForwardErrorCorrection::ReceivedPacketList received_packets_; + // TODO(nisse): The AddReceivedRedPacket method adds one or two packets to + // this list at a time, after which it is emptied by ProcessReceivedFec. It + // will make things simpler to merge AddReceivedRedPacket and + // ProcessReceivedFec into a single method, and we can then delete this list. + std::vector> + received_packets_; ForwardErrorCorrection::RecoveredPacketList recovered_packets_; FecPacketCounter packet_counter_; }; diff --git a/modules/rtp_rtcp/test/testFec/test_fec.cc b/modules/rtp_rtcp/test/testFec/test_fec.cc index c65a2b5926..32dc374e4f 100644 --- a/modules/rtp_rtcp/test/testFec/test_fec.cc +++ b/modules/rtp_rtcp/test/testFec/test_fec.cc @@ -35,8 +35,10 @@ namespace test { using fec_private_tables::kPacketMaskBurstyTbl; void ReceivePackets( - ForwardErrorCorrection::ReceivedPacketList* to_decode_list, - ForwardErrorCorrection::ReceivedPacketList* received_packet_list, + std::vector>* + to_decode_list, + std::vector>* + received_packet_list, size_t num_packets_to_decode, float reorder_rate, float duplicate_rate, @@ -103,8 +105,10 @@ void RunTest(bool use_flexfec) { ForwardErrorCorrection::PacketList media_packet_list; std::list fec_packet_list; - ForwardErrorCorrection::ReceivedPacketList to_decode_list; - ForwardErrorCorrection::ReceivedPacketList received_packet_list; + std::vector> + to_decode_list; + std::vector> + received_packet_list; ForwardErrorCorrection::RecoveredPacketList recovered_packet_list; std::list fec_mask_list; @@ -403,11 +407,10 @@ void RunTest(bool use_flexfec) { } } } - ASSERT_EQ(0, - fec->DecodeFec(&to_decode_list, &recovered_packet_list)) - << "DecodeFec() failed"; - ASSERT_TRUE(to_decode_list.empty()) - << "Received packet list is not empty."; + for (const auto& received_packet : to_decode_list) { + fec->DecodeFec(*received_packet, &recovered_packet_list); + } + to_decode_list.clear(); } media_packet_idx = 0; for (const auto& media_packet : media_packet_list) {