diff --git a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h index de4c4f1b80..dc9adb408d 100644 --- a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -31,7 +31,8 @@ struct FecPacketCounter { class UlpfecReceiver { public: - static UlpfecReceiver* Create(RecoveredPacketReceiver* callback); + static UlpfecReceiver* Create(uint32_t ssrc, + RecoveredPacketReceiver* callback); virtual ~UlpfecReceiver() {} diff --git a/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc b/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc index 74406db086..05655facac 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc +++ b/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc @@ -78,7 +78,7 @@ ForwardErrorCorrection::PacketList MediaPacketGenerator::ConstructMediaPackets( RTC_DCHECK(media_packet); media_packet->data[1] |= 0x80; - fec_seq_num_ = seq_num; + next_seq_num_ = seq_num; return media_packets; } @@ -88,8 +88,8 @@ ForwardErrorCorrection::PacketList MediaPacketGenerator::ConstructMediaPackets( return ConstructMediaPackets(num_media_packets, random_->Rand()); } -uint16_t MediaPacketGenerator::GetFecSeqNum() { - return fec_seq_num_; +uint16_t MediaPacketGenerator::GetNextSeqNum() { + return next_seq_num_; } AugmentedPacketGenerator::AugmentedPacketGenerator(uint32_t ssrc) diff --git a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h index 50594ec70e..1bd8261006 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h +++ b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h @@ -49,7 +49,7 @@ class MediaPacketGenerator { ForwardErrorCorrection::PacketList ConstructMediaPackets( int num_media_packets); - uint16_t GetFecSeqNum(); + uint16_t GetNextSeqNum(); private: uint32_t min_packet_size_; @@ -58,7 +58,7 @@ class MediaPacketGenerator { Random* random_; ForwardErrorCorrection::PacketList media_packets_; - uint16_t fec_seq_num_; + uint16_t next_seq_num_; }; // This class generates media packets with a certain structure of the payload. diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc b/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc index 4a88cb4f91..13899d5e5f 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc @@ -34,7 +34,8 @@ FlexfecReceiver::FlexfecReceiver( RecoveredPacketReceiver* recovered_packet_receiver) : ssrc_(ssrc), protected_media_ssrc_(protected_media_ssrc), - erasure_code_(ForwardErrorCorrection::CreateFlexfec()), + erasure_code_( + ForwardErrorCorrection::CreateFlexfec(ssrc, protected_media_ssrc)), recovered_packet_receiver_(recovered_packet_receiver), clock_(Clock::GetRealTimeClock()), last_recovered_packet_ms_(-1) { diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc index 29c2fdf9fe..a70d73c688 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc @@ -61,7 +61,8 @@ class FlexfecReceiverTest : public ::testing::Test { protected: FlexfecReceiverTest() : receiver_(kFlexfecSsrc, kMediaSsrc, &recovered_packet_receiver_), - erasure_code_(ForwardErrorCorrection::CreateFlexfec()), + erasure_code_( + ForwardErrorCorrection::CreateFlexfec(kFlexfecSsrc, kMediaSsrc)), packet_generator_(kMediaSsrc, kFlexfecSsrc) {} // Generates |num_media_packets| corresponding to a single frame. diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc b/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc index feefe3d1ac..7c69a99655 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc @@ -80,7 +80,8 @@ FlexfecSender::FlexfecSender( protected_media_ssrc_(protected_media_ssrc), seq_num_(rtp_state ? rtp_state->sequence_number : random_.Rand(1, kMaxInitRtpSeqNumber)), - ulpfec_generator_(ForwardErrorCorrection::CreateFlexfec()), + ulpfec_generator_( + ForwardErrorCorrection::CreateFlexfec(ssrc, protected_media_ssrc)), rtp_header_extension_map_(RegisterBweExtensions(rtp_header_extensions)), header_extensions_size_( rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes)) { diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index 8c35f4000f..a2d0e6dc9c 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc @@ -54,6 +54,7 @@ template bool ForwardErrorCorrection::SortablePacket::LessThan::operator() ( const S& first, const T& second) { + RTC_DCHECK_EQ(first->ssrc, second->ssrc); return IsNewerSequenceNumber(second->seq_num, first->seq_num); } @@ -71,27 +72,34 @@ ForwardErrorCorrection::ReceivedFecPacket::~ReceivedFecPacket() = default; ForwardErrorCorrection::ForwardErrorCorrection( std::unique_ptr fec_header_reader, - std::unique_ptr fec_header_writer) - : fec_header_reader_(std::move(fec_header_reader)), + std::unique_ptr fec_header_writer, + uint32_t ssrc, + uint32_t protected_media_ssrc) + : ssrc_(ssrc), + protected_media_ssrc_(protected_media_ssrc), + fec_header_reader_(std::move(fec_header_reader)), fec_header_writer_(std::move(fec_header_writer)), generated_fec_packets_(fec_header_writer_->MaxFecPackets()), packet_mask_size_(0) {} ForwardErrorCorrection::~ForwardErrorCorrection() = default; -std::unique_ptr ForwardErrorCorrection::CreateUlpfec() { +std::unique_ptr ForwardErrorCorrection::CreateUlpfec( + uint32_t ssrc) { std::unique_ptr fec_header_reader(new UlpfecHeaderReader()); std::unique_ptr fec_header_writer(new UlpfecHeaderWriter()); return std::unique_ptr(new ForwardErrorCorrection( - std::move(fec_header_reader), std::move(fec_header_writer))); + std::move(fec_header_reader), std::move(fec_header_writer), ssrc, ssrc)); } -std::unique_ptr -ForwardErrorCorrection::CreateFlexfec() { +std::unique_ptr ForwardErrorCorrection::CreateFlexfec( + uint32_t ssrc, + uint32_t protected_media_ssrc) { std::unique_ptr fec_header_reader(new FlexfecHeaderReader()); std::unique_ptr fec_header_writer(new FlexfecHeaderWriter()); return std::unique_ptr(new ForwardErrorCorrection( - std::move(fec_header_reader), std::move(fec_header_writer))); + std::move(fec_header_reader), std::move(fec_header_writer), ssrc, + protected_media_ssrc)); } int ForwardErrorCorrection::EncodeFec(const PacketList& media_packets, @@ -335,20 +343,25 @@ void ForwardErrorCorrection::ResetState( void ForwardErrorCorrection::InsertMediaPacket( RecoveredPacketList* recovered_packets, ReceivedPacket* received_packet) { + RTC_DCHECK_EQ(received_packet->ssrc, protected_media_ssrc_); + // Search for duplicate packets. for (const auto& recovered_packet : *recovered_packets) { - if (received_packet->seq_num == recovered_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; } } + std::unique_ptr recovered_packet(new RecoveredPacket()); // This "recovered packet" was not recovered using parity packets. 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; @@ -378,23 +391,35 @@ void ForwardErrorCorrection::UpdateCoveringFecPackets( void ForwardErrorCorrection::InsertFecPacket( const RecoveredPacketList& recovered_packets, ReceivedPacket* received_packet) { + RTC_DCHECK_EQ(received_packet->ssrc, ssrc_); + // Check for duplicate. for (const auto& existing_fec_packet : received_fec_packets_) { - if (received_packet->seq_num == existing_fec_packet->seq_num) { + 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; return; } } + std::unique_ptr fec_packet(new ReceivedFecPacket()); fec_packet->pkt = received_packet->pkt; - fec_packet->seq_num = received_packet->seq_num; 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) { return; } + + // TODO(brandtr): Update here when we support multistream protection. + if (fec_packet->protected_ssrc != protected_media_ssrc_) { + LOG(LS_INFO) + << "Received FEC packet is protecting an unknown media SSRC; dropping."; + return; + } + // Parse packet mask from header and represent as protected packets. for (uint16_t byte_idx = 0; byte_idx < fec_packet->packet_mask_size; ++byte_idx) { @@ -405,6 +430,7 @@ void ForwardErrorCorrection::InsertFecPacket( std::unique_ptr protected_packet( new ProtectedPacket()); // This wraps naturally with the sequence number. + protected_packet->ssrc = protected_media_ssrc_; protected_packet->seq_num = static_cast( fec_packet->seq_num_base + (byte_idx << 3) + bit_idx); protected_packet->pkt = nullptr; @@ -412,6 +438,7 @@ void ForwardErrorCorrection::InsertFecPacket( } } } + if (fec_packet->protected_packets.empty()) { // All-zero packet mask; we can discard this FEC packet. LOG(LS_WARNING) << "Received FEC packet has an all-zero packet mask."; @@ -419,11 +446,6 @@ void ForwardErrorCorrection::InsertFecPacket( AssignRecoveredPackets(recovered_packets, fec_packet.get()); // TODO(holmer): Consider replacing this with a binary search for the right // position, and then just insert the new packet. Would get rid of the sort. - // - // For correct decoding, |received_fec_packets_| does not necessarily - // need to be sorted by sequence number (see decoding algorithm in - // AttemptRecover()). By keeping it sorted we try to recover the - // oldest lost packets first, however. received_fec_packets_.push_back(std::move(fec_packet)); received_fec_packets_.sort(SortablePacket::LessThan()); const size_t max_fec_packets = fec_header_reader_->MaxFecPackets(); @@ -467,19 +489,31 @@ void ForwardErrorCorrection::InsertPackets( while (!received_packets->empty()) { ReceivedPacket* received_packet = received_packets->front().get(); - // Check for discarding oldest FEC packet, to avoid wrong FEC decoding from - // sequence number wrap-around. Detection of old FEC packet is based on - // sequence number difference of received packet and oldest packet in FEC - // packet list. + // 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()) { - uint16_t seq_num_diff = - abs(static_cast(received_packet->seq_num) - - static_cast(received_fec_packets_.front()->seq_num)); - if (seq_num_diff > 0x3fff) { - received_fec_packets_.pop_front(); + 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; + } } } @@ -549,6 +583,7 @@ bool ForwardErrorCorrection::FinishPacketRecovery( // Set the SSRC field. ByteWriter::WriteBigEndian(&recovered_packet->pkt->data[8], fec_packet.protected_ssrc); + recovered_packet->ssrc = fec_packet.protected_ssrc; return true; } @@ -684,21 +719,34 @@ uint32_t ForwardErrorCorrection::ParseSsrc(uint8_t* packet) { int ForwardErrorCorrection::DecodeFec( ReceivedPacketList* received_packets, RecoveredPacketList* recovered_packets) { - // TODO(marpan/ajm): can we check for multiple ULP headers, and return an - // error? + RTC_DCHECK(received_packets); + RTC_DCHECK(recovered_packets); + const size_t max_media_packets = fec_header_reader_->MaxMediaPackets(); if (recovered_packets->size() == max_media_packets) { - const unsigned int seq_num_diff = - abs(static_cast(received_packets->front()->seq_num) - - static_cast(recovered_packets->back()->seq_num)); - if (seq_num_diff > max_media_packets) { - // A big gap in sequence numbers. The old recovered packets - // are now useless, so it's safe to do a reset. - ResetState(recovered_packets); + 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; + } + } } } + InsertPackets(received_packets, recovered_packets); AttemptRecovery(recovered_packets); + return 0; } diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h index f45473cb1f..e70d7db493 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h +++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h @@ -33,6 +33,8 @@ class FecHeaderWriter; // Option exists to enable unequal protection (UEP) across packets. // This is not to be confused with protection within packets // (referred to as uneven level protection (ULP) in RFC 5109). +// TODO(brandtr): Split this class into a separate encoder +// and a separate decoder. class ForwardErrorCorrection { public: // TODO(holmer): As a next step all these struct-like packet classes should be @@ -62,29 +64,25 @@ class ForwardErrorCorrection { class SortablePacket { public: // Functor which returns true if the sequence number of |first| - // is < the sequence number of |second|. + // is < the sequence number of |second|. Should only ever be called for + // packets belonging to the same SSRC. struct LessThan { template bool operator() (const S& first, const T& second); }; + uint32_t ssrc; uint16_t seq_num; }; // The received list parameter of DecodeFec() references structs of this type. // - // The ssrc member is needed to ensure that we can restore the SSRC field of - // recovered packets. In most situations this could be retrieved from other - // media packets, but in the case of an FEC packet protecting a single - // missing media packet, we have no other means of obtaining it. // TODO(holmer): Refactor into a proper class. class ReceivedPacket : public SortablePacket { public: ReceivedPacket(); ~ReceivedPacket(); - uint32_t ssrc; // SSRC of the current frame. Must be set for FEC - // packets, but not required for media packets. bool is_fec; // Set to true if this is an FEC packet and false // otherwise. rtc::scoped_refptr pkt; // Pointer to the packet storage. @@ -109,7 +107,7 @@ class ForwardErrorCorrection { // Used to link media packets to their protecting FEC packets. // // TODO(holmer): Refactor into a proper class. - class ProtectedPacket : public ForwardErrorCorrection::SortablePacket { + class ProtectedPacket : public SortablePacket { public: ProtectedPacket(); ~ProtectedPacket(); @@ -122,7 +120,7 @@ class ForwardErrorCorrection { // Used for internal storage of received FEC packets in a list. // // TODO(holmer): Refactor into a proper class. - class ReceivedFecPacket : public ForwardErrorCorrection::SortablePacket { + class ReceivedFecPacket : public SortablePacket { public: ReceivedFecPacket(); ~ReceivedFecPacket(); @@ -150,8 +148,10 @@ class ForwardErrorCorrection { ~ForwardErrorCorrection(); // Creates a ForwardErrorCorrection tailored for a specific FEC scheme. - static std::unique_ptr CreateUlpfec(); - static std::unique_ptr CreateFlexfec(); + static std::unique_ptr CreateUlpfec(uint32_t ssrc); + static std::unique_ptr CreateFlexfec( + uint32_t ssrc, + uint32_t protected_media_ssrc); // Generates a list of FEC packets from supplied media packets. // @@ -242,7 +242,9 @@ class ForwardErrorCorrection { protected: ForwardErrorCorrection(std::unique_ptr fec_header_reader, - std::unique_ptr fec_header_writer); + std::unique_ptr fec_header_writer, + uint32_t ssrc, + uint32_t protected_media_ssrc); private: // Analyzes |media_packets| for holes in the sequence and inserts zero columns @@ -330,6 +332,10 @@ class ForwardErrorCorrection { // for recovering lost packets. void DiscardOldRecoveredPackets(RecoveredPacketList* recovered_packets); + // These SSRCs are only used by the decoder. + const uint32_t ssrc_; + const uint32_t protected_media_ssrc_; + std::unique_ptr fec_header_reader_; std::unique_ptr fec_header_writer_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc index a1c60c8d16..11b6849ef0 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc @@ -108,7 +108,8 @@ void RtpFecTest::ReceivedPackets( const PacketListType& packet_list, int* loss_mask, bool is_fec) { - uint16_t fec_seq_num = media_packet_generator_.GetFecSeqNum(); + uint16_t fec_seq_num = ForwardErrorCorrectionType::GetFirstFecSeqNum( + media_packet_generator_.GetNextSeqNum()); int packet_idx = 0; for (const auto& packet : packet_list) { @@ -120,19 +121,17 @@ void RtpFecTest::ReceivedPackets( memcpy(received_packet->pkt->data, packet->data, packet->length); received_packet->is_fec = is_fec; if (!is_fec) { - // For media packets, the sequence number and marker bit is - // obtained from RTP header. These were set in ConstructMediaPackets(). + received_packet->ssrc = kMediaSsrc; + // For media packets, the sequence number is obtained from the + // RTP header as written by MediaPacketGenerator::ConstructMediaPackets. received_packet->seq_num = ByteReader::ReadBigEndian(&packet->data[2]); } else { - // The sequence number, marker bit, and ssrc number are defined in the - // RTP header of the FEC packet, which is not constructed in this test. - // So we set these values below based on the values generated in - // ConstructMediaPackets(). + received_packet->ssrc = ForwardErrorCorrectionType::kFecSsrc; + // For FEC packets, we simulate the sequence numbers differently + // depending on if ULPFEC or FlexFEC is used. See the definition of + // ForwardErrorCorrectionType::GetFirstFecSeqNum. received_packet->seq_num = fec_seq_num; - // The ssrc value for FEC packets is set to the one used for the - // media packets in ConstructMediaPackets(). - received_packet->ssrc = kMediaSsrc; } received_packets_.push_back(std::move(received_packet)); } @@ -177,18 +176,39 @@ bool RtpFecTest::IsRecoveryComplete() { class FlexfecForwardErrorCorrection : public ForwardErrorCorrection { public: + static const uint32_t kFecSsrc = kFlexfecSsrc; + FlexfecForwardErrorCorrection() : ForwardErrorCorrection( std::unique_ptr(new FlexfecHeaderReader()), - std::unique_ptr(new FlexfecHeaderWriter())) {} + std::unique_ptr(new FlexfecHeaderWriter()), + kFecSsrc, + kMediaSsrc) {} + + // For FlexFEC we let the FEC packet sequence numbers be independent of + // the media packet sequence numbers. + static uint16_t GetFirstFecSeqNum(uint16_t next_media_seq_num) { + Random random(0xbe110); + return random.Rand(); + } }; class UlpfecForwardErrorCorrection : public ForwardErrorCorrection { public: + static const uint32_t kFecSsrc = kMediaSsrc; + UlpfecForwardErrorCorrection() : ForwardErrorCorrection( std::unique_ptr(new UlpfecHeaderReader()), - std::unique_ptr(new UlpfecHeaderWriter())) {} + std::unique_ptr(new UlpfecHeaderWriter()), + kFecSsrc, + kMediaSsrc) {} + + // For ULPFEC we assume that the FEC packets are subsequent to the media + // packets in terms of sequence number. + static uint16_t GetFirstFecSeqNum(uint16_t next_media_seq_num) { + return next_media_seq_num; + } }; using FecTypes = @@ -359,13 +379,14 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameRecovery) { EXPECT_TRUE(this->IsRecoveryComplete()); } -// Sequence number wrap occurs within the FEC packets for the frame. -// In this case we will discard FEC packet and full recovery is not expected. -// Same problem will occur if wrap is within media packets but FEC packet is +// 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 FEC packets. +// is used to detect old ULPFEC packets. // TODO(marpan): Update test if wrap-around handling changes in FEC decoding. -TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { +using RtpFecTestUlpfecOnly = RtpFecTest; +TEST_F(RtpFecTestUlpfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { constexpr int kNumImportantPackets = 0; constexpr bool kUseUnequalProtection = false; constexpr uint8_t kProtectionFactor = 200; @@ -398,8 +419,61 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { &this->recovered_packets_)); // The two FEC packets are received and should allow for complete recovery, - // but because of the wrap the second FEC packet will be discarded, and only - // one media packet is recoverable. So exepct 2 media packets on recovered + // but because of the wrap the first FEC packet will be discarded, and only + // one media packet is recoverable. So expect 2 media packets on recovered + // list and no complete recovery. + EXPECT_EQ(2u, this->recovered_packets_.size()); + EXPECT_TRUE(this->recovered_packets_.size() != this->media_packets_.size()); + EXPECT_FALSE(this->IsRecoveryComplete()); +} + +// TODO(brandtr): This test mimics the one above, ensuring that the recovery +// strategy of FlexFEC matches the recovery strategy of ULPFEC. Since FlexFEC +// does not share the sequence number space with the media, however, having a +// matching recovery strategy may be suboptimal. Study this further. +using RtpFecTestFlexfecOnly = RtpFecTest; +TEST_F(RtpFecTestFlexfecOnly, FecRecoveryWithSeqNumGapOneFrameNoRecovery) { + constexpr int kNumImportantPackets = 0; + constexpr bool kUseUnequalProtection = false; + constexpr uint8_t kProtectionFactor = 200; + + // 1 frame: 3 media packets and 2 FEC packets. + // Sequence number wrap in FEC packets. + // -----Frame 1---- + // #65532(media) #65533(media) #65534(media) #65535(FEC) #0(FEC). + this->media_packets_ = + this->media_packet_generator_.ConstructMediaPackets(3, 65532); + + EXPECT_EQ( + 0, this->fec_.EncodeFec(this->media_packets_, kProtectionFactor, + kNumImportantPackets, kUseUnequalProtection, + kFecMaskBursty, &this->generated_fec_packets_)); + + // Expect 2 FEC packets. + EXPECT_EQ(2u, this->generated_fec_packets_.size()); + + // Overwrite the sequence numbers generated by ConstructMediaPackets, + // to make sure that we do have a wrap. + auto it = this->generated_fec_packets_.begin(); + ByteWriter::WriteBigEndian(&(*it)->data[2], 65535); + ++it; + ByteWriter::WriteBigEndian(&(*it)->data[2], 0); + + // Lose the last two media packets (seq# 65533, 65534). + memset(this->media_loss_mask_, 0, sizeof(this->media_loss_mask_)); + memset(this->fec_loss_mask_, 0, sizeof(this->fec_loss_mask_)); + this->media_loss_mask_[1] = 1; + this->media_loss_mask_[2] = 1; + this->ReceivedPackets(this->media_packets_, this->media_loss_mask_, false); + this->ReceivedPackets(this->generated_fec_packets_, this->fec_loss_mask_, + true); + + EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, + &this->recovered_packets_)); + + // The two FEC packets are received and should allow for complete recovery, + // but because of the wrap the first FEC packet will be discarded, and only + // one media packet is recoverable. So expect 2 media packets on recovered // list and no complete recovery. EXPECT_EQ(2u, this->recovered_packets_.size()); EXPECT_TRUE(this->recovered_packets_.size() != this->media_packets_.size()); @@ -434,9 +508,9 @@ TYPED_TEST(RtpFecTest, FecRecoveryWithMediaOutOfOrder) { // Reorder received media packets. auto it0 = this->received_packets_.begin(); - auto it2 = this->received_packets_.begin(); - it2++; - std::swap(*it0, *it2); + auto it1 = this->received_packets_.begin(); + it1++; + std::swap(*it0, *it1); EXPECT_EQ(0, this->fec_.DecodeFec(&this->received_packets_, &this->recovered_packets_)); @@ -958,42 +1032,4 @@ TYPED_TEST(RtpFecTest, FecRecoveryNonConsecutivePacketsWrap) { EXPECT_FALSE(this->IsRecoveryComplete()); } -// 'using' directive needed for compiler to be happy. -using RtpFecTestWithFlexfec = RtpFecTest; -TEST_F(RtpFecTestWithFlexfec, - FecRecoveryWithLossAndDifferentMediaAndFlexfecSsrcs) { - constexpr int kNumImportantPackets = 0; - constexpr bool kUseUnequalProtection = false; - constexpr int kNumMediaPackets = 4; - constexpr uint8_t kProtectionFactor = 60; - - media_packets_ = - media_packet_generator_.ConstructMediaPackets(kNumMediaPackets); - - EXPECT_EQ(0, fec_.EncodeFec(media_packets_, kProtectionFactor, - kNumImportantPackets, kUseUnequalProtection, - kFecMaskBursty, &generated_fec_packets_)); - - // Expect 1 FEC packet. - EXPECT_EQ(1u, generated_fec_packets_.size()); - - // 1 media packet lost - memset(media_loss_mask_, 0, sizeof(media_loss_mask_)); - memset(fec_loss_mask_, 0, sizeof(fec_loss_mask_)); - media_loss_mask_[3] = 1; - NetworkReceivedPackets(media_loss_mask_, fec_loss_mask_); - - // Simulate FlexFEC packet received on different SSRC. - auto it = received_packets_.begin(); - ++it; - ++it; - ++it; // Now at the FEC packet. - (*it)->ssrc = kFlexfecSsrc; - - EXPECT_EQ(0, fec_.DecodeFec(&received_packets_, &recovered_packets_)); - - // One packet lost, one FEC packet, expect complete recovery. - EXPECT_TRUE(IsRecoveryComplete()); -} - } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc index 4c50e22291..4aa5331d6d 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc @@ -49,6 +49,13 @@ constexpr uint8_t kHighProtectionThreshold = 80; // |kMinMediaPackets| + 1 packets are sent to the FEC code. constexpr float kMinMediaPacketsAdaptationThreshold = 2.0f; +// At construction time, we don't know the SSRC that is used for the generated +// FEC packets, but we still need to give it to the ForwardErrorCorrection ctor +// to be used in the decoding. +// TODO(brandtr): Get rid of this awkwardness by splitting +// ForwardErrorCorrection in two objects -- one encoder and one decoder. +constexpr uint32_t kUnknownSsrc = 0; + } // namespace RedPacket::RedPacket(size_t length) @@ -94,7 +101,7 @@ size_t RedPacket::length() const { } UlpfecGenerator::UlpfecGenerator() - : UlpfecGenerator(ForwardErrorCorrection::CreateUlpfec()) {} + : UlpfecGenerator(ForwardErrorCorrection::CreateUlpfec(kUnknownSsrc)) {} UlpfecGenerator::UlpfecGenerator(std::unique_ptr fec) : fec_(std::move(fec)), diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index dad445dab0..d35e71395d 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -21,13 +21,16 @@ namespace webrtc { -UlpfecReceiver* UlpfecReceiver::Create(RecoveredPacketReceiver* callback) { - return new UlpfecReceiverImpl(callback); +UlpfecReceiver* UlpfecReceiver::Create(uint32_t ssrc, + RecoveredPacketReceiver* callback) { + return new UlpfecReceiverImpl(ssrc, callback); } -UlpfecReceiverImpl::UlpfecReceiverImpl(RecoveredPacketReceiver* callback) - : recovered_packet_callback_(callback), - fec_(ForwardErrorCorrection::CreateUlpfec()) {} +UlpfecReceiverImpl::UlpfecReceiverImpl(uint32_t ssrc, + RecoveredPacketReceiver* callback) + : ssrc_(ssrc), + recovered_packet_callback_(callback), + fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {} UlpfecReceiverImpl::~UlpfecReceiverImpl() { received_packets_.clear(); @@ -72,6 +75,12 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( const uint8_t* incoming_rtp_packet, size_t packet_length, uint8_t ulpfec_payload_type) { + if (header.ssrc != ssrc_) { + LOG(LS_INFO) + << "Received RED packet with different SSRC than expected; dropping."; + return -1; + } + rtc::CritScope cs(&crit_sect_); uint8_t red_header_length = 1; @@ -90,6 +99,7 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( // Get payload type from RED header and sequence number from RTP header. uint8_t payload_type = incoming_rtp_packet[header.headerLength] & 0x7f; received_packet->is_fec = payload_type == ulpfec_payload_type; + received_packet->ssrc = header.ssrc; received_packet->seq_num = header.sequenceNumber; uint16_t block_length = 0; @@ -155,6 +165,7 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( second_received_packet->pkt = new ForwardErrorCorrection::Packet; second_received_packet->is_fec = true; + second_received_packet->ssrc = header.ssrc; second_received_packet->seq_num = header.sequenceNumber; ++packet_counter_.num_fec_packets; diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index e24570c1cd..37085726b1 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -23,7 +23,7 @@ namespace webrtc { class UlpfecReceiverImpl : public UlpfecReceiver { public: - explicit UlpfecReceiverImpl(RecoveredPacketReceiver* callback); + explicit UlpfecReceiverImpl(uint32_t ssrc, RecoveredPacketReceiver* callback); virtual ~UlpfecReceiverImpl(); int32_t AddReceivedRedPacket(const RTPHeader& rtp_header, @@ -36,6 +36,8 @@ class UlpfecReceiverImpl : public UlpfecReceiver { FecPacketCounter GetPacketCounter() const override; private: + const uint32_t ssrc_; + rtc::CriticalSection crit_sect_; RecoveredPacketReceiver* recovered_packet_callback_; std::unique_ptr fec_; diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index 6f9243429d..7a9c446a22 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -48,8 +48,9 @@ class NullRecoveredPacketReceiver : public RecoveredPacketReceiver { class UlpfecReceiverTest : public ::testing::Test { protected: UlpfecReceiverTest() - : fec_(ForwardErrorCorrection::CreateUlpfec()), - receiver_fec_(UlpfecReceiver::Create(&recovered_packet_receiver_)), + : fec_(ForwardErrorCorrection::CreateUlpfec(kMediaSsrc)), + receiver_fec_( + UlpfecReceiver::Create(kMediaSsrc, &recovered_packet_receiver_)), packet_generator_(kMediaSsrc) {} // Generates |num_fec_packets| FEC packets, given |media_packets|. @@ -179,7 +180,7 @@ void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data, NullRecoveredPacketReceiver null_callback; std::unique_ptr receiver_fec( - UlpfecReceiver::Create(&null_callback)); + UlpfecReceiver::Create(kMediaSsrc, &null_callback)); receiver_fec->AddReceivedRedPacket(header, data, length, ulpfec_payload_type); } diff --git a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc index 4aaae98852..a65e8a684f 100644 --- a/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc +++ b/webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc @@ -78,13 +78,7 @@ void ReceivePackets( } } -// Too slow to finish before timeout on iOS. See webrtc:4755. -#if defined(WEBRTC_IOS) -#define MAYBE_FecTest DISABLED_FecTest -#else -#define MAYBE_FecTest FecTest -#endif -TEST(FecTest, MAYBE_FecTest) { +void RunTest(bool use_flexfec) { // TODO(marpan): Split this function into subroutines/helper functions. enum { kMaxNumberMediaPackets = 48 }; enum { kMaxNumberFecPackets = 48 }; @@ -107,8 +101,6 @@ TEST(FecTest, MAYBE_FecTest) { ASSERT_EQ(12, kMaxMediaPackets[1]) << "Max media packets for bursty mode not " << "equal to 12."; - std::unique_ptr fec = - ForwardErrorCorrection::CreateUlpfec(); ForwardErrorCorrection::PacketList media_packet_list; std::list fec_packet_list; ForwardErrorCorrection::ReceivedPacketList to_decode_list; @@ -138,7 +130,24 @@ TEST(FecTest, MAYBE_FecTest) { uint16_t seq_num = 0; uint32_t timestamp = random.Rand(); - const uint32_t ssrc = random.Rand(1u, 0xfffffffe); + const uint32_t media_ssrc = random.Rand(1u, 0xfffffffe); + uint32_t fec_ssrc; + uint16_t fec_seq_num_offset; + if (use_flexfec) { + fec_ssrc = random.Rand(1u, 0xfffffffe); + fec_seq_num_offset = random.Rand(); + } else { + fec_ssrc = media_ssrc; + fec_seq_num_offset = 0; + } + + std::unique_ptr fec; + if (use_flexfec) { + fec = ForwardErrorCorrection::CreateFlexfec(fec_ssrc, media_ssrc); + } else { + RTC_DCHECK_EQ(media_ssrc, fec_ssrc); + fec = ForwardErrorCorrection::CreateUlpfec(fec_ssrc); + } // Loop over the mask types: random and bursty. for (int mask_type_idx = 0; mask_type_idx < kNumFecMaskTypes; @@ -268,7 +277,7 @@ TEST(FecTest, MAYBE_FecTest) { ByteWriter::WriteBigEndian(&media_packet->data[4], timestamp); ByteWriter::WriteBigEndian(&media_packet->data[8], - ssrc); + media_ssrc); // Generate random values for payload for (size_t j = 12; j < media_packet->length; ++j) { media_packet->data[j] = random.Rand(); @@ -302,6 +311,7 @@ TEST(FecTest, MAYBE_FecTest) { received_packet->pkt->length = media_packet->length; memcpy(received_packet->pkt->data, media_packet->data, media_packet->length); + received_packet->ssrc = media_ssrc; received_packet->seq_num = ByteReader::ReadBigEndian(&media_packet->data[2]); received_packet->is_fec = false; @@ -323,9 +333,9 @@ TEST(FecTest, MAYBE_FecTest) { received_packet->pkt->length = fec_packet->length; memcpy(received_packet->pkt->data, fec_packet->data, fec_packet->length); - received_packet->seq_num = seq_num; + received_packet->seq_num = fec_seq_num_offset + seq_num; received_packet->is_fec = true; - received_packet->ssrc = ssrc; + received_packet->ssrc = fec_ssrc; received_packet_list.push_back(std::move(received_packet)); fec_mask_list.push_back(fec_packet_masks[fec_packet_idx]); @@ -453,5 +463,21 @@ TEST(FecTest, MAYBE_FecTest) { << "Recovered packet list is not empty"; } +// Too slow to finish before timeout on iOS. See webrtc:4755. +#if defined(WEBRTC_IOS) +#define MAYBE_UlpecTest DISABLED_UlpecTest +#define MAYBE_FlexfecTest DISABLED_FlexfecTest +#else +#define MAYBE_UlpecTest UlpecTest +#define MAYBE_FlexfecTest FlexfecTest +#endif +TEST(FecTest, MAYBE_UlpecTest) { + RunTest(false); +} + +TEST(FecTest, MAYBE_FlexfecTest) { + RunTest(true); +} + } // namespace test } // namespace webrtc diff --git a/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc index 6a55a3dc2a..6b8ea0f45d 100644 --- a/webrtc/video/rtp_video_stream_receiver.cc +++ b/webrtc/video/rtp_video_stream_receiver.cc @@ -100,7 +100,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( this, &rtp_payload_registry_)), rtp_receive_statistics_(ReceiveStatistics::Create(clock_)), - ulpfec_receiver_(UlpfecReceiver::Create(this)), + ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)), receiving_(false), restored_packet_in_use_(false), last_packet_log_ms_(-1),