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}
diff --git a/modules/rtp_rtcp/include/flexfec_receiver.h b/modules/rtp_rtcp/include/flexfec_receiver.h
index 7355262..024d691 100644
--- a/modules/rtp_rtcp/include/flexfec_receiver.h
+++ b/modules/rtp_rtcp/include/flexfec_receiver.h
@@ -39,8 +39,10 @@
// Protected to aid testing.
protected:
- bool AddReceivedPacket(const RtpPacketReceived& packet);
- bool ProcessReceivedPackets();
+ std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket(
+ const RtpPacketReceived& packet);
+ void ProcessReceivedPacket(
+ const ForwardErrorCorrection::ReceivedPacket& received_packet);
private:
// Config.
@@ -50,8 +52,6 @@
// Erasure code interfacing and callback.
std::unique_ptr<ForwardErrorCorrection> 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 5a625d8..ec35a2c0 100644
--- a/modules/rtp_rtcp/source/flexfec_receiver.cc
+++ b/modules/rtp_rtcp/source/flexfec_receiver.cc
@@ -48,10 +48,11 @@
void FlexfecReceiver::OnRtpPacket(const RtpPacketReceived& packet) {
RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_);
- if (!AddReceivedPacket(packet)) {
+ std::unique_ptr<ReceivedPacket> received_packet = AddReceivedPacket(packet);
+ if (!received_packet)
return;
- }
- ProcessReceivedPackets();
+
+ ProcessReceivedPacket(*received_packet);
}
FecPacketCounter FlexfecReceiver::GetPacketCounter() const {
@@ -61,7 +62,8 @@
// TODO(eladalon): Consider using packet.recovered() to avoid processing
// recovered packets here.
-bool FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) {
+std::unique_ptr<ReceivedPacket> 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 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
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 ae2ec1d..1761e1d 100644
--- a/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
@@ -54,7 +54,7 @@
}
// Expose methods for tests.
using FlexfecReceiver::AddReceivedPacket;
- using FlexfecReceiver::ProcessReceivedPackets;
+ using FlexfecReceiver::ProcessReceivedPacket;
};
class FlexfecReceiverTest : public ::testing::Test {
@@ -113,8 +113,10 @@
std::unique_ptr<Packet> media_packet(
packet_generator_.NextPacket(0, kPayloadLength));
- EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet)));
- EXPECT_TRUE(receiver_.ProcessReceivedPackets());
+ std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> received_packet =
+ receiver_.AddReceivedPacket(ParsePacket(*media_packet));
+ ASSERT_TRUE(received_packet);
+ receiver_.ProcessReceivedPacket(*received_packet);
}
TEST_F(FlexfecReceiverTest, ReceivesMediaAndFecPackets) {
@@ -127,10 +129,13 @@
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<ForwardErrorCorrection::ReceivedPacket> 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 @@
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<ForwardErrorCorrection::ReceivedPacket> 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 @@
fec_packet->data[10] = 6;
fec_packet->data[11] = 7;
- EXPECT_TRUE(receiver_.AddReceivedPacket(ParsePacket(*media_packet)));
- EXPECT_TRUE(receiver_.ProcessReceivedPackets());
+ std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> 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 @@
// 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<ForwardErrorCorrection::ReceivedPacket> 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> 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<ForwardErrorCorrection::ReceivedPacket> 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 fe348b5..3c1b1b4 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::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 @@
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::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<ReceivedFecPacket> 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::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<int>(received_packet->seq_num) -
- static_cast<int>((*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<int16_t>(
+ // received_packet.seq_num - back_recovered_packet->seq_num)
+ uint16_t seq_num_diff = abs(static_cast<int>(received_packet.seq_num) -
+ static_cast<int>((*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 @@
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<int>(received_packet->seq_num) -
- static_cast<int>(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<int16_t>(
+ // received_packet.seq_num - back_recovered_packet->seq_num)
+ const unsigned int seq_num_diff =
+ abs(static_cast<int>(received_packet.seq_num) -
+ static_cast<int>(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 f821f6f..97acb39 100644
--- a/modules/rtp_rtcp/source/forward_error_correction.h
+++ b/modules/rtp_rtcp/source/forward_error_correction.h
@@ -75,9 +75,10 @@
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 @@
};
using PacketList = std::list<std::unique_ptr<Packet>>;
- using ReceivedPacketList = std::list<std::unique_ptr<ReceivedPacket>>;
using RecoveredPacketList = std::list<std::unique_ptr<RecoveredPacket>>;
using ReceivedFecPacketList = std::list<std::unique_ptr<ReceivedFecPacket>>;
@@ -218,10 +218,8 @@
// 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 @@
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 @@
// 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 5f84af5..d01aa55 100644
--- a/modules/rtp_rtcp/source/rtp_fec_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_fec_unittest.cc
@@ -86,7 +86,8 @@
ForwardErrorCorrection::PacketList media_packets_;
std::list<ForwardErrorCorrection::Packet*> generated_fec_packets_;
- ForwardErrorCorrection::ReceivedPacketList received_packets_;
+ std::vector<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>
+ received_packets_;
ForwardErrorCorrection::RecoveredPacketList recovered_packets_;
int media_loss_mask_[kUlpfecMaxMediaPackets];
@@ -98,6 +99,7 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
}
// 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<UlpfecForwardErrorCorrection>;
-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 @@
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<FlexfecForwardErrorCorrection>;
TEST_F(RtpFecTestFlexfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) {
constexpr int kNumImportantPackets = 0;
@@ -468,8 +491,10 @@
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 @@
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 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 a35393d..4292f3c 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc
+++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc
@@ -216,23 +216,22 @@
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 8025729..edc3d31 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 <memory>
+#include <vector>
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "modules/rtp_rtcp/include/ulpfec_receiver.h"
@@ -41,10 +42,12 @@
rtc::CriticalSection crit_sect_;
RecoveredPacketReceiver* recovered_packet_callback_;
std::unique_ptr<ForwardErrorCorrection> 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<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>
+ 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 c65a2b5..32dc374 100644
--- a/modules/rtp_rtcp/test/testFec/test_fec.cc
+++ b/modules/rtp_rtcp/test/testFec/test_fec.cc
@@ -35,8 +35,10 @@
using fec_private_tables::kPacketMaskBurstyTbl;
void ReceivePackets(
- ForwardErrorCorrection::ReceivedPacketList* to_decode_list,
- ForwardErrorCorrection::ReceivedPacketList* received_packet_list,
+ std::vector<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>*
+ to_decode_list,
+ std::vector<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>*
+ received_packet_list,
size_t num_packets_to_decode,
float reorder_rate,
float duplicate_rate,
@@ -103,8 +105,10 @@
ForwardErrorCorrection::PacketList media_packet_list;
std::list<ForwardErrorCorrection::Packet*> fec_packet_list;
- ForwardErrorCorrection::ReceivedPacketList to_decode_list;
- ForwardErrorCorrection::ReceivedPacketList received_packet_list;
+ std::vector<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>
+ to_decode_list;
+ std::vector<std::unique_ptr<ForwardErrorCorrection::ReceivedPacket>>
+ received_packet_list;
ForwardErrorCorrection::RecoveredPacketList recovered_packet_list;
std::list<uint8_t*> fec_mask_list;
@@ -403,11 +407,10 @@
}
}
}
- 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) {
