Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(212)

Unified Diff: modules/rtp_rtcp/source/forward_error_correction.cc

Issue 3012243002: Change ForwardErrorCorrection class to accept one received packet at a time. (Closed)
Patch Set: Rebased. Created 3 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « modules/rtp_rtcp/source/forward_error_correction.h ('k') | modules/rtp_rtcp/source/rtp_fec_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: modules/rtp_rtcp/source/forward_error_correction.cc
diff --git a/modules/rtp_rtcp/source/forward_error_correction.cc b/modules/rtp_rtcp/source/forward_error_correction.cc
index fe348b5e3366f9f5cc93dfc73c23e42573d14d00..3c1b1b4154ef4f65687f8f41a7bb89c59687b7e2 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<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::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<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();
+ if (received_packet.is_fec) {
+ InsertFecPacket(*recovered_packets, received_packet);
+ } else {
+ InsertMediaPacket(recovered_packets, received_packet);
}
- RTC_DCHECK(received_packets->empty());
+
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<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 {
« no previous file with comments | « modules/rtp_rtcp/source/forward_error_correction.h ('k') | modules/rtp_rtcp/source/rtp_fec_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698