From 75fd127640bdf1729af6b4a25875e6d01f1570e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 24 Jun 2020 16:52:20 +0200 Subject: [PATCH] Allows FEC generation after pacer step. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split out from https://webrtc-review.googlesource.com/c/src/+/173708 This CL enables FEC packets to be generated as media packets are sent, rather than generated, i.e. media packets are inserted into the fec generator after the pacing stage rather than at packetization time. This may have some small impact of performance. FEC packets are typically only generated when a new packet with a marker bit is added, which means FEC packets protecting a frame will now be sent after all of the media packets, rather than (potentially) interleaved with them. Therefore this feature is currently behind a flag so we can examine the impact. Once we are comfortable with the behavior we'll make it default and remove the old code. Note that this change does not include the "protect all header extensions" part of the original CL - that will be a follow-up. Bug: webrtc:11340 Change-Id: I3fe139c5d53968579b75b91e2612075451ff0f5d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177760 Commit-Queue: Erik Språng Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#31558} --- call/rtp_video_sender.cc | 35 +++- call/rtp_video_sender.h | 1 + modules/pacing/pacing_controller.cc | 8 +- modules/pacing/pacing_controller.h | 2 + modules/pacing/pacing_controller_unittest.cc | 51 ++++- modules/pacing/packet_router.cc | 17 +- modules/pacing/packet_router.h | 6 + .../task_queue_paced_sender_unittest.cc | 4 + modules/rtp_rtcp/include/rtp_rtcp_defines.h | 11 + modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 9 + modules/rtp_rtcp/source/rtp_packet_to_send.h | 11 + modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 11 + modules/rtp_rtcp/source/rtp_rtcp_impl.h | 5 + modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 31 ++- modules/rtp_rtcp/source/rtp_rtcp_impl2.h | 8 +- modules/rtp_rtcp/source/rtp_rtcp_interface.h | 11 + modules/rtp_rtcp/source/rtp_sender_egress.cc | 92 +++++++- modules/rtp_rtcp/source/rtp_sender_egress.h | 10 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 196 ++++++++++++------ modules/rtp_rtcp/source/rtp_sender_video.cc | 5 +- modules/rtp_rtcp/source/ulpfec_generator.cc | 2 + test/scenario/video_stream_unittest.cc | 20 ++ 22 files changed, 454 insertions(+), 92 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 5f8d2df965..854a18aa54 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -197,6 +197,7 @@ std::vector CreateRtpStreamSenders( FrameEncryptorInterface* frame_encryptor, const CryptoOptions& crypto_options, rtc::scoped_refptr frame_transformer, + bool use_deferred_fec, const WebRtcKeyValueConfig& trials) { RTC_DCHECK_GT(rtp_config.ssrcs.size(), 0); @@ -244,7 +245,9 @@ std::vector CreateRtpStreamSenders( std::unique_ptr fec_generator = MaybeCreateFecGenerator(clock, rtp_config, suspended_ssrcs, i, trials); configuration.fec_generator = fec_generator.get(); - video_config.fec_generator = fec_generator.get(); + if (!use_deferred_fec) { + video_config.fec_generator = fec_generator.get(); + } configuration.rtx_send_ssrc = rtp_config.GetRtxSsrcAssociatedWithMediaSsrc(rtp_config.ssrcs[i]); @@ -338,6 +341,9 @@ RtpVideoSender::RtpVideoSender( field_trials_.Lookup("WebRTC-UseEarlyLossDetection"), "Disabled")), has_packet_feedback_(TransportSeqNumExtensionConfigured(rtp_config)), + use_deferred_fec_( + absl::StartsWith(field_trials_.Lookup("WebRTC-DeferredFecGeneration"), + "Enabled")), active_(false), module_process_thread_(nullptr), suspended_ssrcs_(std::move(suspended_ssrcs)), @@ -356,6 +362,7 @@ RtpVideoSender::RtpVideoSender( frame_encryptor, crypto_options, std::move(frame_transformer), + use_deferred_fec_, field_trials_)), rtp_config_(rtp_config), codec_type_(GetVideoCodecType(rtp_config)), @@ -848,14 +855,26 @@ int RtpVideoSender::ProtectionRequest(const FecProtectionParams* delta_params, *sent_nack_rate_bps = 0; *sent_fec_rate_bps = 0; for (const RtpStreamSender& stream : rtp_streams_) { - if (stream.fec_generator) { - stream.fec_generator->SetProtectionParameters(*delta_params, *key_params); - *sent_fec_rate_bps += stream.fec_generator->CurrentFecRate().bps(); + if (use_deferred_fec_) { + stream.rtp_rtcp->SetFecProtectionParams(*delta_params, *key_params); + + auto send_bitrate = stream.rtp_rtcp->GetSendRates(); + *sent_video_rate_bps += send_bitrate[RtpPacketMediaType::kVideo].bps(); + *sent_fec_rate_bps += + send_bitrate[RtpPacketMediaType::kForwardErrorCorrection].bps(); + *sent_nack_rate_bps += + send_bitrate[RtpPacketMediaType::kRetransmission].bps(); + } else { + if (stream.fec_generator) { + stream.fec_generator->SetProtectionParameters(*delta_params, + *key_params); + *sent_fec_rate_bps += stream.fec_generator->CurrentFecRate().bps(); + } + *sent_video_rate_bps += stream.sender_video->VideoBitrateSent(); + *sent_nack_rate_bps += + stream.rtp_rtcp->GetSendRates()[RtpPacketMediaType::kRetransmission] + .bps(); } - *sent_video_rate_bps += stream.sender_video->VideoBitrateSent(); - *sent_nack_rate_bps += - stream.rtp_rtcp->GetSendRates()[RtpPacketMediaType::kRetransmission] - .bps(); } return 0; } diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index 0c277d6aa7..e364729236 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -176,6 +176,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, const bool account_for_packetization_overhead_; const bool use_early_loss_detection_; const bool has_packet_feedback_; + const bool use_deferred_fec_; // TODO(holmer): Remove crit_ once RtpVideoSender runs on the // transport task queue. diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 07e265b0da..7e7a01b628 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -441,6 +441,9 @@ void PacingController::ProcessPackets() { keepalive_data_sent += DataSize::Bytes(packet->payload_size() + packet->padding_size()); packet_sender_->SendPacket(std::move(packet), PacedPacketInfo()); + for (auto& packet : packet_sender_->FetchFec()) { + EnqueuePacket(std::move(packet)); + } } OnPaddingSent(keepalive_data_sent); } @@ -559,8 +562,11 @@ void PacingController::ProcessPackets() { packet_size += DataSize::Bytes(rtp_packet->headers_size()) + transport_overhead_per_packet_; } - packet_sender_->SendPacket(std::move(rtp_packet), pacing_info); + packet_sender_->SendPacket(std::move(rtp_packet), pacing_info); + for (auto& packet : packet_sender_->FetchFec()) { + EnqueuePacket(std::move(packet)); + } data_sent += packet_size; // Send done, update send/process time to the target send time. diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index 6e361aebb4..775fdc9557 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -57,6 +57,8 @@ class PacingController { virtual ~PacketSender() = default; virtual void SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info) = 0; + // Should be called after each call to SendPacket(). + virtual std::vector> FetchFec() = 0; virtual std::vector> GeneratePadding( DataSize size) = 0; }; diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc index bc4d47333e..9194d079a9 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -97,6 +97,10 @@ class MockPacingControllerCallback : public PacingController::PacketSender { int64_t capture_timestamp, bool retransmission, bool padding)); + MOCK_METHOD(std::vector>, + FetchFec, + (), + (override)); MOCK_METHOD(size_t, SendPadding, (size_t target_size)); }; @@ -108,6 +112,11 @@ class MockPacketSender : public PacingController::PacketSender { (std::unique_ptr packet, const PacedPacketInfo& cluster_info), (override)); + MOCK_METHOD(std::vector>, + FetchFec, + (), + (override)); + MOCK_METHOD(std::vector>, GeneratePadding, (DataSize target_size), @@ -125,6 +134,10 @@ class PacingControllerPadding : public PacingController::PacketSender { total_bytes_sent_ += packet->payload_size(); } + std::vector> FetchFec() override { + return {}; + } + std::vector> GeneratePadding( DataSize target_size) override { size_t num_packets = @@ -158,6 +171,10 @@ class PacingControllerProbing : public PacingController::PacketSender { } } + std::vector> FetchFec() override { + return {}; + } + std::vector> GeneratePadding( DataSize target_size) override { // From RTPSender: @@ -299,7 +316,7 @@ class PacingControllerTest } SimulatedClock clock_; - MockPacingControllerCallback callback_; + ::testing::NiceMock callback_; std::unique_ptr pacer_; }; @@ -2029,6 +2046,38 @@ TEST_P(PacingControllerTest, PaddingTargetAccountsForPaddingRate) { AdvanceTimeAndProcess(); } +TEST_P(PacingControllerTest, SendsDeferredFecPackets) { + ScopedFieldTrials trial("WebRTC-DeferredFecGeneration/Enabled/"); + SetUp(); + + const uint32_t kSsrc = 12345; + const uint32_t kFlexSsrc = 54321; + uint16_t sequence_number = 1234; + uint16_t flexfec_sequence_number = 4321; + const size_t kPacketSize = 123; + + // Set pacing rate to 1000 packet/s, no padding. + pacer_->SetPacingRates( + DataSize::Bytes(1000 * kPacketSize) / TimeDelta::Seconds(1), + DataRate::Zero()); + + int64_t now = clock_.TimeInMilliseconds(); + Send(RtpPacketMediaType::kVideo, kSsrc, sequence_number, now, kPacketSize); + EXPECT_CALL(callback_, SendPacket(kSsrc, sequence_number, now, false, false)); + EXPECT_CALL(callback_, FetchFec).WillOnce([&]() { + EXPECT_CALL(callback_, SendPacket(kFlexSsrc, flexfec_sequence_number, now, + false, false)); + EXPECT_CALL(callback_, FetchFec); + std::vector> fec_packets; + fec_packets.push_back( + BuildPacket(RtpPacketMediaType::kForwardErrorCorrection, kFlexSsrc, + flexfec_sequence_number, now, kPacketSize)); + return fec_packets; + }); + AdvanceTimeAndProcess(); + AdvanceTimeAndProcess(); +} + INSTANTIATE_TEST_SUITE_P( WithAndWithoutIntervalBudget, PacingControllerTest, diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index e75b5a337a..833c56d6fd 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -42,7 +42,9 @@ PacketRouter::PacketRouter(uint16_t start_transport_seq) bitrate_bps_(0), max_bitrate_bps_(std::numeric_limits::max()), active_remb_module_(nullptr), - transport_seq_(start_transport_seq) {} + transport_seq_(start_transport_seq) { + send_thread_checker_.Detach(); +} PacketRouter::~PacketRouter() { RTC_DCHECK(send_modules_map_.empty()); @@ -139,6 +141,7 @@ void PacketRouter::RemoveReceiveRtpModule( void PacketRouter::SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info) { + RTC_DCHECK_RUN_ON(&send_thread_checker_); TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacketRouter::SendPacket", "sequence_number", packet->SequenceNumber(), "rtp_timestamp", packet->Timestamp()); @@ -171,6 +174,18 @@ void PacketRouter::SendPacket(std::unique_ptr packet, // properties needed for payload based padding. Cache it for later use. last_send_module_ = rtp_module; } + + for (auto& packet : rtp_module->FetchFecPackets()) { + pending_fec_packets_.push_back(std::move(packet)); + } +} + +std::vector> PacketRouter::FetchFec() { + RTC_DCHECK_RUN_ON(&send_thread_checker_); + std::vector> fec_packets = + std::move(pending_fec_packets_); + pending_fec_packets_.clear(); + return fec_packets; } std::vector> PacketRouter::GeneratePadding( diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 73837f2ffe..b9395073bc 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -29,6 +29,7 @@ #include "rtc_base/constructor_magic.h" #include "rtc_base/critical_section.h" #include "rtc_base/synchronization/mutex.h" +#include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -57,6 +58,7 @@ class PacketRouter : public RemoteBitrateObserver, void SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info) override; + std::vector> FetchFec() override; std::vector> GeneratePadding( DataSize size) override; @@ -128,6 +130,10 @@ class PacketRouter : public RemoteBitrateObserver, uint64_t transport_seq_ RTC_GUARDED_BY(modules_mutex_); + SequenceChecker send_thread_checker_; + std::vector> pending_fec_packets_ + RTC_GUARDED_BY(send_thread_checker_); + RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter); }; } // namespace webrtc diff --git a/modules/pacing/task_queue_paced_sender_unittest.cc b/modules/pacing/task_queue_paced_sender_unittest.cc index 876cd96cfd..b02f387768 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -43,6 +43,10 @@ class MockPacketRouter : public PacketRouter { (std::unique_ptr packet, const PacedPacketInfo& cluster_info), (override)); + MOCK_METHOD(std::vector>, + FetchFec, + (), + (override)); MOCK_METHOD(std::vector>, GeneratePadding, (DataSize target_size), diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 1b72236bbb..079c205f37 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -33,6 +33,7 @@ namespace webrtc { class RtpPacket; +class RtpPacketToSend; namespace rtcp { class TransportFeedback; } @@ -466,5 +467,15 @@ class SendPacketObserver { int64_t capture_time_ms, uint32_t ssrc) = 0; }; + +// Interface for a class that can assign RTP sequence numbers for a packet +// to be sent. +class SequenceNumberAssigner { + public: + SequenceNumberAssigner() = default; + virtual ~SequenceNumberAssigner() = default; + + virtual void AssignSequenceNumber(RtpPacketToSend* packet) = 0; +}; } // namespace webrtc #endif // MODULES_RTP_RTCP_INCLUDE_RTP_RTCP_DEFINES_H_ diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 08b38eee7b..d597b1e289 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -98,6 +98,15 @@ class MockRtpRtcpInterface : public RtpRtcpInterface { TrySendPacket, (RtpPacketToSend * packet, const PacedPacketInfo& pacing_info), (override)); + MOCK_METHOD(void, + SetFecProtectionParams, + (const FecProtectionParams& delta_params, + const FecProtectionParams& key_params), + (override)); + MOCK_METHOD(std::vector>, + FetchFecPackets, + (), + (override)); MOCK_METHOD(void, OnPacketsAcknowledged, (rtc::ArrayView), diff --git a/modules/rtp_rtcp/source/rtp_packet_to_send.h b/modules/rtp_rtcp/source/rtp_packet_to_send.h index 8997bce0d2..9aaf9a52e6 100644 --- a/modules/rtp_rtcp/source/rtp_packet_to_send.h +++ b/modules/rtp_rtcp/source/rtp_packet_to_send.h @@ -108,6 +108,15 @@ class RtpPacketToSend : public RtpPacket { void set_is_key_frame(bool is_key_frame) { is_key_frame_ = is_key_frame; } bool is_key_frame() const { return is_key_frame_; } + // Indicates if packets should be protected by FEC (Forward Error Correction). + void set_fec_protect_packet(bool protect) { fec_protect_packet_ = protect; } + bool fec_protect_packet() const { return fec_protect_packet_; } + + // Indicates if packet is using RED encapsulation, in accordance with + // https://tools.ietf.org/html/rfc2198 + void set_is_red(bool is_red) { is_red_ = is_red; } + bool is_red() const { return is_red_; } + private: int64_t capture_time_ms_ = 0; absl::optional packet_type_; @@ -116,6 +125,8 @@ class RtpPacketToSend : public RtpPacket { std::vector application_data_; bool is_first_packet_of_frame_ = false; bool is_key_frame_ = false; + bool fec_protect_packet_ = false; + bool is_red_ = false; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index f372dbe0cd..bd5879206a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -391,6 +391,17 @@ bool ModuleRtpRtcpImpl::TrySendPacket(RtpPacketToSend* packet, return true; } +void ModuleRtpRtcpImpl::SetFecProtectionParams(const FecProtectionParams&, + const FecProtectionParams&) { + RTC_NOTREACHED() << "Deferred FEC not supported in deprecated RTP module."; +} + +std::vector> +ModuleRtpRtcpImpl::FetchFecPackets() { + RTC_NOTREACHED() << "Deferred FEC not supported in deprecated RTP module."; + return {}; +} + void ModuleRtpRtcpImpl::OnPacketsAcknowledged( rtc::ArrayView sequence_numbers) { RTC_DCHECK(rtp_sender_); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 989b8d3717..1fa57db372 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -139,6 +139,11 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) override; + void SetFecProtectionParams(const FecProtectionParams& delta_params, + const FecProtectionParams& key_params) override; + + std::vector> FetchFecPackets() override; + void OnPacketsAcknowledged( rtc::ArrayView sequence_numbers) override; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index e50f72bb29..fc02f4b290 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -42,11 +42,15 @@ ModuleRtpRtcpImpl2::RtpSenderContext::RtpSenderContext( const RtpRtcpInterface::Configuration& config) : packet_history(config.clock, config.enable_rtx_padding_prioritization), packet_sender(config, &packet_history), - non_paced_sender(&packet_sender), + non_paced_sender(&packet_sender, this), packet_generator( config, &packet_history, config.paced_sender ? config.paced_sender : &non_paced_sender) {} +void ModuleRtpRtcpImpl2::RtpSenderContext::AssignSequenceNumber( + RtpPacketToSend* packet) { + packet_generator.AssignSequenceNumber(packet); +} ModuleRtpRtcpImpl2::ModuleRtpRtcpImpl2(const Configuration& configuration) : rtcp_sender_(configuration), @@ -396,6 +400,31 @@ bool ModuleRtpRtcpImpl2::TrySendPacket(RtpPacketToSend* packet, return true; } +void ModuleRtpRtcpImpl2::SetFecProtectionParams( + const FecProtectionParams& delta_params, + const FecProtectionParams& key_params) { + RTC_DCHECK(rtp_sender_); + rtp_sender_->packet_sender.SetFecProtectionParameters(delta_params, + key_params); +} + +std::vector> +ModuleRtpRtcpImpl2::FetchFecPackets() { + RTC_DCHECK(rtp_sender_); + auto fec_packets = rtp_sender_->packet_sender.FetchFecPackets(); + if (!fec_packets.empty()) { + // Don't assign sequence numbers for FlexFEC packets. + const bool generate_sequence_numbers = + !rtp_sender_->packet_sender.FlexFecSsrc().has_value(); + if (generate_sequence_numbers) { + for (auto& fec_packet : fec_packets) { + rtp_sender_->packet_generator.AssignSequenceNumber(fec_packet.get()); + } + } + } + return fec_packets; +} + void ModuleRtpRtcpImpl2::OnPacketsAcknowledged( rtc::ArrayView sequence_numbers) { RTC_DCHECK(rtp_sender_); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h index 276f88a6b5..02f82d78c1 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h @@ -145,6 +145,11 @@ class ModuleRtpRtcpImpl2 final : public RtpRtcpInterface, bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) override; + void SetFecProtectionParams(const FecProtectionParams& delta_params, + const FecProtectionParams& key_params) override; + + std::vector> FetchFecPackets() override; + void OnPacketsAcknowledged( rtc::ArrayView sequence_numbers) override; @@ -265,8 +270,9 @@ class ModuleRtpRtcpImpl2 final : public RtpRtcpInterface, FRIEND_TEST_ALL_PREFIXES(RtpRtcpImpl2Test, Rtt); FRIEND_TEST_ALL_PREFIXES(RtpRtcpImpl2Test, RttForReceiverOnly); - struct RtpSenderContext { + struct RtpSenderContext : public SequenceNumberAssigner { explicit RtpSenderContext(const RtpRtcpInterface::Configuration& config); + void AssignSequenceNumber(RtpPacketToSend* packet) override; // Storage of packets, for retransmissions and padding, if applicable. RtpPacketHistory packet_history; // Handles final time timestamping/stats/etc and handover to Transport. diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 440837fc5d..f763da244c 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -293,6 +293,17 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { virtual bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) = 0; + // Update the FEC protection parameters to use for delta- and key-frames. + // Only used when deferred FEC is active. + virtual void SetFecProtectionParams( + const FecProtectionParams& delta_params, + const FecProtectionParams& key_params) = 0; + + // If deferred FEC generation is enabled, this method should be called after + // calling TrySendPacket(). Any generated FEC packets will be removed and + // returned from the FEC generator. + virtual std::vector> FetchFecPackets() = 0; + virtual void OnPacketsAcknowledged( rtc::ArrayView sequence_numbers) = 0; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index c309fc3f0d..d7ce67431f 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -10,6 +10,7 @@ #include "modules/rtp_rtcp/source/rtp_sender_egress.h" +#include #include #include #include @@ -36,21 +37,45 @@ bool IsEnabled(absl::string_view name, } // namespace RtpSenderEgress::NonPacedPacketSender::NonPacedPacketSender( - RtpSenderEgress* sender) - : transport_sequence_number_(0), sender_(sender) {} + RtpSenderEgress* sender, + SequenceNumberAssigner* sequence_number_assigner) + : transport_sequence_number_(0), + sender_(sender), + sequence_number_assigner_(sequence_number_assigner) { + RTC_DCHECK(sequence_number_assigner_); +} RtpSenderEgress::NonPacedPacketSender::~NonPacedPacketSender() = default; void RtpSenderEgress::NonPacedPacketSender::EnqueuePackets( std::vector> packets) { for (auto& packet : packets) { - if (!packet->SetExtension( - ++transport_sequence_number_)) { - --transport_sequence_number_; - } - packet->ReserveExtension(); - packet->ReserveExtension(); + PrepareForSend(packet.get()); sender_->SendPacket(packet.get(), PacedPacketInfo()); } + auto fec_packets = sender_->FetchFecPackets(); + if (!fec_packets.empty()) { + // Don't generate sequence numbers for flexfec, they are already running on + // an internally maintained sequence. + const bool generate_sequence_numbers = !sender_->FlexFecSsrc().has_value(); + + for (auto& packet : fec_packets) { + if (generate_sequence_numbers) { + sequence_number_assigner_->AssignSequenceNumber(packet.get()); + } + PrepareForSend(packet.get()); + } + EnqueuePackets(std::move(fec_packets)); + } +} + +void RtpSenderEgress::NonPacedPacketSender::PrepareForSend( + RtpPacketToSend* packet) { + if (!packet->SetExtension( + ++transport_sequence_number_)) { + --transport_sequence_number_; + } + packet->ReserveExtension(); + packet->ReserveExtension(); } RtpSenderEgress::RtpSenderEgress(const RtpRtcpInterface::Configuration& config, @@ -68,6 +93,10 @@ RtpSenderEgress::RtpSenderEgress(const RtpRtcpInterface::Configuration& config, event_log_(config.event_log), is_audio_(config.audio), need_rtp_packet_infos_(config.need_rtp_packet_infos), + fec_generator_( + IsEnabled("WebRTC-DeferredFecGeneration", config.field_trials) + ? config.fec_generator + : nullptr), transport_feedback_observer_(config.transport_feedback_callback), send_side_delay_observer_(config.send_side_delay_observer), send_packet_observer_(config.send_packet_observer), @@ -135,6 +164,33 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, RtpSequenceNumberMap::Info(timestamp, is_first_packet_of_frame, is_last_packet_of_frame)); } + + if (fec_generator_ && packet->fec_protect_packet()) { + // Deferred fec generation is used, add packet to generator. + + RTC_DCHECK(fec_generator_); + RTC_DCHECK(packet->packet_type() == RtpPacketMediaType::kVideo); + if (packet->is_red()) { + RtpPacketToSend unpacked_packet(*packet); + + const rtc::CopyOnWriteBuffer buffer = packet->Buffer(); + // Grab media payload type from RED header. + const size_t headers_size = packet->headers_size(); + unpacked_packet.SetPayloadType(buffer[headers_size]); + + // Copy the media payload into the unpacked buffer. + uint8_t* payload_buffer = + unpacked_packet.SetPayloadSize(packet->payload_size() - 1); + std::copy(&packet->payload()[0] + 1, + &packet->payload()[0] + packet->payload_size(), + payload_buffer); + + fec_generator_->AddPacketAndGenerateFec(unpacked_packet); + } else { + // If not RED encapsulated - we can just insert packet directly. + fec_generator_->AddPacketAndGenerateFec(*packet); + } + } } // Bug webrtc:7859. While FEC is invoked from rtp_sender_video, and not after @@ -200,6 +256,8 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, rtc::CritScope lock(&lock_); UpdateRtpStats(*packet); media_has_been_sent_ = true; + // TODO(sprang): Add support for FEC protecting all header extensions, add + // media packet to generator here instead. } } @@ -282,6 +340,24 @@ std::vector RtpSenderEgress::GetSentRtpPacketInfos( return results; } +void RtpSenderEgress::SetFecProtectionParameters( + const FecProtectionParams& delta_params, + const FecProtectionParams& key_params) { + rtc::CritScope lock(&lock_); + if (fec_generator_) { + fec_generator_->SetProtectionParameters(delta_params, key_params); + } +} + +std::vector> +RtpSenderEgress::FetchFecPackets() { + rtc::CritScope lock(&lock_); + if (fec_generator_) { + return fec_generator_->GetFecPackets(); + } + return {}; +} + bool RtpSenderEgress::HasCorrectSsrc(const RtpPacketToSend& packet) const { switch (*packet.packet_type()) { case RtpPacketMediaType::kAudio: diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.h b/modules/rtp_rtcp/source/rtp_sender_egress.h index a8e033c5bf..3f522b9642 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/rtp_sender_egress.h @@ -36,15 +36,18 @@ class RtpSenderEgress { // without passing through an actual paced sender. class NonPacedPacketSender : public RtpPacketSender { public: - explicit NonPacedPacketSender(RtpSenderEgress* sender); + NonPacedPacketSender(RtpSenderEgress* sender, + SequenceNumberAssigner* sequence_number_assigner); virtual ~NonPacedPacketSender(); void EnqueuePackets( std::vector> packets) override; private: + void PrepareForSend(RtpPacketToSend* packet); uint16_t transport_sequence_number_; RtpSenderEgress* const sender_; + SequenceNumberAssigner* sequence_number_assigner_; }; RtpSenderEgress(const RtpRtcpInterface::Configuration& config, @@ -78,6 +81,10 @@ class RtpSenderEgress { rtc::ArrayView sequence_numbers) const RTC_LOCKS_EXCLUDED(lock_); + void SetFecProtectionParameters(const FecProtectionParams& delta_params, + const FecProtectionParams& key_params); + std::vector> FetchFecPackets(); + private: // Maps capture time in milliseconds to send-side delay in milliseconds. // Send-side delay is the difference between transmission time and capture @@ -114,6 +121,7 @@ class RtpSenderEgress { RtcEventLog* const event_log_; const bool is_audio_; const bool need_rtp_packet_infos_; + VideoFecGenerator* const fec_generator_ RTC_GUARDED_BY(lock_); TransportFeedbackObserver* const transport_feedback_observer_; SendSideDelayObserver* const send_side_delay_observer_; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 12055b5b1c..9052e61168 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -32,6 +32,7 @@ #include "modules/rtp_rtcp/source/rtp_sender_egress.h" #include "modules/rtp_rtcp/source/rtp_sender_video.h" #include "modules/rtp_rtcp/source/rtp_utility.h" +#include "modules/rtp_rtcp/source/video_fec_generator.h" #include "rtc_base/arraysize.h" #include "rtc_base/rate_limiter.h" #include "rtc_base/strings/string_builder.h" @@ -140,8 +141,10 @@ MATCHER_P(SameRtcEventTypeAs, value, "") { } struct TestConfig { - explicit TestConfig(bool with_overhead) : with_overhead(with_overhead) {} + TestConfig(bool with_overhead, bool deferred_fec) + : with_overhead(with_overhead), deferred_fec(deferred_fec) {} bool with_overhead = false; + bool deferred_fec = false; }; class MockRtpPacketPacer : public RtpPacketSender { @@ -211,15 +214,18 @@ class StreamDataTestCallback : public StreamDataCountersCallback { // Mimics ModuleRtpRtcp::RtpSenderContext. // TODO(sprang): Split up unit tests and test these components individually // wherever possible. -struct RtpSenderContext { +struct RtpSenderContext : public SequenceNumberAssigner { explicit RtpSenderContext(const RtpRtcpInterface::Configuration& config) : packet_history_(config.clock, config.enable_rtx_padding_prioritization), packet_sender_(config, &packet_history_), - non_paced_sender_(&packet_sender_), + non_paced_sender_(&packet_sender_, this), packet_generator_( config, &packet_history_, config.paced_sender ? config.paced_sender : &non_paced_sender_) {} + void AssignSequenceNumber(RtpPacketToSend* packet) override { + packet_generator_.AssignSequenceNumber(packet); + } RtpPacketHistory packet_history_; RtpSenderEgress packet_sender_; RtpSenderEgress::NonPacedPacketSender non_paced_sender_; @@ -228,10 +234,14 @@ struct RtpSenderContext { class FieldTrialConfig : public WebRtcKeyValueConfig { public: - FieldTrialConfig() : overhead_enabled_(false), max_padding_factor_(1200) {} + FieldTrialConfig() + : overhead_enabled_(false), + deferred_fec_(false), + max_padding_factor_(1200) {} ~FieldTrialConfig() override {} void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; } + void UseDeferredFec(bool enabled) { deferred_fec_ = enabled; } void SetMaxPaddingFactor(double factor) { max_padding_factor_ = factor; } std::string Lookup(absl::string_view key) const override { @@ -242,12 +252,15 @@ class FieldTrialConfig : public WebRtcKeyValueConfig { return ssb.str(); } else if (key == "WebRTC-SendSideBwe-WithOverhead") { return overhead_enabled_ ? "Enabled" : "Disabled"; + } else if (key == "WebRTC-DeferredFecGeneration") { + return deferred_fec_ ? "Enabled" : "Disabled"; } return ""; } private: bool overhead_enabled_; + bool deferred_fec_; double max_padding_factor_; }; @@ -268,6 +281,7 @@ class RtpSenderTest : public ::testing::TestWithParam { &fake_clock_), kMarkerBit(true) { field_trials_.SetOverHeadEnabled(GetParam().with_overhead); + field_trials_.UseDeferredFec(GetParam().deferred_fec); } void SetUp() override { SetUpRtpSender(true, false, false); } @@ -285,12 +299,20 @@ class RtpSenderTest : public ::testing::TestWithParam { void SetUpRtpSender(bool pacer, bool populate_network2, bool always_send_mid_and_rid) { + SetUpRtpSender(pacer, populate_network2, always_send_mid_and_rid, + &flexfec_sender_); + } + + void SetUpRtpSender(bool pacer, + bool populate_network2, + bool always_send_mid_and_rid, + VideoFecGenerator* fec_generator) { RtpRtcpInterface::Configuration config; config.clock = &fake_clock_; config.outgoing_transport = &transport_; config.local_media_ssrc = kSsrc; config.rtx_send_ssrc = kRtxSsrc; - config.fec_generator = &flexfec_sender_; + config.fec_generator = fec_generator; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -1248,6 +1270,7 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; + config.field_trials = &field_trials_; rtp_sender_context_ = std::make_unique(config); rtp_sender()->SetSequenceNumber(kSeqNum); @@ -1258,7 +1281,11 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.fec_generator = &flexfec_sender; + if (!GetParam().deferred_fec) { + video_config.fec_generator = &flexfec_sender; + } + video_config.fec_type = flexfec_sender.GetFecType(); + video_config.fec_overhead_bytes = flexfec_sender.MaxPacketOverhead(); video_config.fec_type = flexfec_sender.GetFecType(); video_config.fec_overhead_bytes = flexfec_sender.MaxPacketOverhead(); video_config.field_trials = &field_trials; @@ -1274,46 +1301,56 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { uint16_t flexfec_seq_num; RTPVideoHeader video_header; - std::unique_ptr media_packet; - std::unique_ptr fec_packet; + std::unique_ptr media_packet; + std::unique_ptr fec_packet; - EXPECT_CALL(mock_paced_sender_, EnqueuePackets) - .WillOnce([&](std::vector> packets) { - for (auto& packet : packets) { - if (packet->packet_type() == RtpPacketMediaType::kVideo) { - EXPECT_EQ(packet->Ssrc(), kSsrc); - EXPECT_EQ(packet->SequenceNumber(), kSeqNum); - media_packet = std::move(packet); - } else { - EXPECT_EQ(packet->packet_type(), + EXPECT_CALL(mock_paced_sender_, EnqueuePackets) + .WillOnce([&](std::vector> packets) { + for (auto& packet : packets) { + if (packet->packet_type() == RtpPacketMediaType::kVideo) { + EXPECT_EQ(packet->Ssrc(), kSsrc); + EXPECT_EQ(packet->SequenceNumber(), kSeqNum); + media_packet = std::move(packet); + + if (GetParam().deferred_fec) { + // Simulate RtpSenderEgress adding packet to fec generator. + flexfec_sender.AddPacketAndGenerateFec(*media_packet); + auto fec_packets = flexfec_sender.GetFecPackets(); + EXPECT_EQ(fec_packets.size(), 1u); + fec_packet = std::move(fec_packets[0]); + EXPECT_EQ(fec_packet->packet_type(), RtpPacketMediaType::kForwardErrorCorrection); - EXPECT_EQ(packet->Ssrc(), kFlexFecSsrc); - fec_packet = std::move(packet); + EXPECT_EQ(fec_packet->Ssrc(), kFlexFecSsrc); } + } else if (packet->packet_type() == + RtpPacketMediaType::kForwardErrorCorrection) { + fec_packet = std::move(packet); + EXPECT_EQ(fec_packet->Ssrc(), kFlexFecSsrc); } - }); + } + }); - video_header.frame_type = VideoFrameType::kVideoFrameKey; - EXPECT_TRUE(rtp_sender_video.SendVideo( - kMediaPayloadType, kCodecType, kTimestamp, - fake_clock_.TimeInMilliseconds(), kPayloadData, nullptr, video_header, - kDefaultExpectedRetransmissionTimeMs)); - ASSERT_TRUE(media_packet != nullptr); - ASSERT_TRUE(fec_packet != nullptr); + video_header.frame_type = VideoFrameType::kVideoFrameKey; + EXPECT_TRUE(rtp_sender_video.SendVideo( + kMediaPayloadType, kCodecType, kTimestamp, + fake_clock_.TimeInMilliseconds(), kPayloadData, nullptr, video_header, + kDefaultExpectedRetransmissionTimeMs)); + ASSERT_TRUE(media_packet != nullptr); + ASSERT_TRUE(fec_packet != nullptr); - flexfec_seq_num = fec_packet->SequenceNumber(); - rtp_egress()->SendPacket(media_packet.get(), PacedPacketInfo()); - rtp_egress()->SendPacket(fec_packet.get(), PacedPacketInfo()); + flexfec_seq_num = fec_packet->SequenceNumber(); + rtp_egress()->SendPacket(media_packet.get(), PacedPacketInfo()); + rtp_egress()->SendPacket(fec_packet.get(), PacedPacketInfo()); - ASSERT_EQ(2, transport_.packets_sent()); - const RtpPacketReceived& sent_media_packet = transport_.sent_packets_[0]; - EXPECT_EQ(kMediaPayloadType, sent_media_packet.PayloadType()); - EXPECT_EQ(kSeqNum, sent_media_packet.SequenceNumber()); - EXPECT_EQ(kSsrc, sent_media_packet.Ssrc()); - const RtpPacketReceived& sent_flexfec_packet = transport_.sent_packets_[1]; - EXPECT_EQ(kFlexfecPayloadType, sent_flexfec_packet.PayloadType()); - EXPECT_EQ(flexfec_seq_num, sent_flexfec_packet.SequenceNumber()); - EXPECT_EQ(kFlexFecSsrc, sent_flexfec_packet.Ssrc()); + ASSERT_EQ(2, transport_.packets_sent()); + const RtpPacketReceived& sent_media_packet = transport_.sent_packets_[0]; + EXPECT_EQ(kMediaPayloadType, sent_media_packet.PayloadType()); + EXPECT_EQ(kSeqNum, sent_media_packet.SequenceNumber()); + EXPECT_EQ(kSsrc, sent_media_packet.Ssrc()); + const RtpPacketReceived& sent_flexfec_packet = transport_.sent_packets_[1]; + EXPECT_EQ(kFlexfecPayloadType, sent_flexfec_packet.PayloadType()); + EXPECT_EQ(flexfec_seq_num, sent_flexfec_packet.SequenceNumber()); + EXPECT_EQ(kFlexFecSsrc, sent_flexfec_packet.Ssrc()); } TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { @@ -1336,6 +1373,7 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; + config.field_trials = &field_trials_; rtp_sender_context_ = std::make_unique(config); rtp_sender()->SetSequenceNumber(kSeqNum); @@ -1344,7 +1382,9 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.fec_generator = &flexfec_sender; + if (!GetParam().deferred_fec) { + video_config.fec_generator = &flexfec_sender; + } video_config.fec_type = flexfec_sender.GetFecType(); video_config.fec_overhead_bytes = flexfec_sender_.MaxPacketOverhead(); video_config.field_trials = &field_trials; @@ -1355,7 +1395,11 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { params.fec_rate = 15; params.max_fec_frames = 1; params.fec_mask_type = kFecMaskRandom; - flexfec_sender.SetProtectionParameters(params, params); + if (GetParam().deferred_fec) { + rtp_egress()->SetFecProtectionParameters(params, params); + } else { + flexfec_sender.SetProtectionParameters(params, params); + } EXPECT_CALL(mock_rtc_event_log_, LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) @@ -1660,25 +1704,16 @@ TEST_P(RtpSenderTest, FecOverheadRate) { kNoRtpExtensions, kNoRtpExtensionSizes, nullptr /* rtp_state */, &fake_clock_); - // Reset |rtp_sender_| to use FlexFEC. - RtpRtcpInterface::Configuration config; - config.clock = &fake_clock_; - config.outgoing_transport = &transport_; - config.paced_sender = &mock_paced_sender_; - config.local_media_ssrc = kSsrc; - config.fec_generator = &flexfec_sender; - config.event_log = &mock_rtc_event_log_; - config.send_packet_observer = &send_packet_observer_; - config.retransmission_rate_limiter = &retransmission_rate_limiter_; - rtp_sender_context_ = std::make_unique(config); - - rtp_sender()->SetSequenceNumber(kSeqNum); + // Reset |rtp_sender_| to use this FlexFEC instance. + SetUpRtpSender(false, false, false, &flexfec_sender); FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.fec_generator = &flexfec_sender; + if (!GetParam().deferred_fec) { + video_config.fec_generator = &flexfec_sender; + } video_config.fec_type = flexfec_sender.GetFecType(); video_config.fec_overhead_bytes = flexfec_sender.MaxPacketOverhead(); video_config.field_trials = &field_trials; @@ -1688,12 +1723,15 @@ TEST_P(RtpSenderTest, FecOverheadRate) { params.fec_rate = 15; params.max_fec_frames = 1; params.fec_mask_type = kFecMaskRandom; - flexfec_sender.SetProtectionParameters(params, params); + if (GetParam().deferred_fec) { + rtp_egress()->SetFecProtectionParameters(params, params); + } else { + flexfec_sender.SetProtectionParameters(params, params); + } constexpr size_t kNumMediaPackets = 10; constexpr size_t kNumFecPackets = kNumMediaPackets; constexpr int64_t kTimeBetweenPacketsMs = 10; - EXPECT_CALL(mock_paced_sender_, EnqueuePackets).Times(kNumMediaPackets); for (size_t i = 0; i < kNumMediaPackets; ++i) { RTPVideoHeader video_header; @@ -1711,9 +1749,20 @@ TEST_P(RtpSenderTest, FecOverheadRate) { constexpr size_t kPayloadLength = sizeof(kPayloadData); constexpr size_t kPacketLength = kRtpHeaderLength + kFlexfecHeaderLength + kGenericCodecHeaderLength + kPayloadLength; - EXPECT_NEAR(kNumFecPackets * kPacketLength * 8 / - (kNumFecPackets * kTimeBetweenPacketsMs / 1000.0f), - flexfec_sender.CurrentFecRate().bps(), 500); + + if (GetParam().deferred_fec) { + EXPECT_NEAR( + kNumFecPackets * kPacketLength * 8 / + (kNumFecPackets * kTimeBetweenPacketsMs / 1000.0f), + rtp_egress() + ->GetSendRates()[RtpPacketMediaType::kForwardErrorCorrection] + .bps(), + 500); + } else { + EXPECT_NEAR(kNumFecPackets * kPacketLength * 8 / + (kNumFecPackets * kTimeBetweenPacketsMs / 1000.0f), + flexfec_sender.CurrentFecRate().bps(), 500); + } } TEST_P(RtpSenderTest, BitrateCallbacks) { @@ -1861,15 +1910,18 @@ TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacksUlpfec) { const uint8_t kUlpfecPayloadType = 97; const uint8_t kPayloadType = 127; const VideoCodecType kCodecType = VideoCodecType::kVideoCodecGeneric; - FieldTrialBasedConfig field_trials; + UlpfecGenerator ulpfec_generator(kRedPayloadType, kUlpfecPayloadType, &fake_clock_); + SetUpRtpSender(false, false, false, &ulpfec_generator); RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - video_config.field_trials = &field_trials; + video_config.field_trials = &field_trials_; video_config.red_payload_type = kRedPayloadType; - video_config.fec_generator = &ulpfec_generator; + if (!GetParam().deferred_fec) { + video_config.fec_generator = &ulpfec_generator; + } video_config.fec_type = ulpfec_generator.GetFecType(); video_config.fec_overhead_bytes = ulpfec_generator.MaxPacketOverhead(); RTPSenderVideo rtp_sender_video(video_config); @@ -1886,7 +1938,11 @@ TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacksUlpfec) { fec_params.fec_mask_type = kFecMaskRandom; fec_params.fec_rate = 1; fec_params.max_fec_frames = 1; - ulpfec_generator.SetProtectionParameters(fec_params, fec_params); + if (GetParam().deferred_fec) { + rtp_egress()->SetFecProtectionParameters(fec_params, fec_params); + } else { + ulpfec_generator.SetProtectionParameters(fec_params, fec_params); + } video_header.frame_type = VideoFrameType::kVideoFrameDelta; ASSERT_TRUE(rtp_sender_video.SendVideo(kPayloadType, kCodecType, 1234, 4321, payload, nullptr, video_header, @@ -2704,12 +2760,16 @@ TEST_P(RtpSenderTest, IgnoresNackAfterDisablingMedia) { INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTest, - ::testing::Values(TestConfig{false}, - TestConfig{true})); + ::testing::Values(TestConfig{false, false}, + TestConfig{false, true}, + TestConfig{true, false}, + TestConfig{false, false})); INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTestWithoutPacer, - ::testing::Values(TestConfig{false}, - TestConfig{true})); + ::testing::Values(TestConfig{false, false}, + TestConfig{false, true}, + TestConfig{true, false}, + TestConfig{false, false})); } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 58a8699688..c6a87c63bf 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -585,8 +585,8 @@ bool RTPSenderVideo::SendVideo( if (fec_generator_) { fec_generator_->AddPacketAndGenerateFec(*packet); } else { - // TODO(sprang): When deferred FEC generation is enabled, just mark the - // packet as protected here. + // Deferred FEC generation, just mark packet. + packet->set_fec_protect_packet(true); } } @@ -594,6 +594,7 @@ bool RTPSenderVideo::SendVideo( std::unique_ptr red_packet(new RtpPacketToSend(*packet)); BuildRedPayload(*packet, red_packet.get()); red_packet->SetPayloadType(*red_payload_type_); + red_packet->set_is_red(true); // Send |red_packet| instead of |packet| for allocated sequence number. red_packet->set_packet_type(RtpPacketMediaType::kVideo); diff --git a/modules/rtp_rtcp/source/ulpfec_generator.cc b/modules/rtp_rtcp/source/ulpfec_generator.cc index 265fa4d1ac..04cb59dc7d 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator.cc @@ -230,6 +230,8 @@ std::vector> UlpfecGenerator::GetFecPackets() { total_fec_size_bytes += red_packet->size(); red_packet->set_packet_type(RtpPacketMediaType::kForwardErrorCorrection); red_packet->set_allow_retransmission(false); + red_packet->set_is_red(true); + red_packet->set_fec_protect_packet(false); fec_packets.push_back(std::move(red_packet)); } diff --git a/test/scenario/video_stream_unittest.cc b/test/scenario/video_stream_unittest.cc index 37afe1b1e7..873ef639ba 100644 --- a/test/scenario/video_stream_unittest.cc +++ b/test/scenario/video_stream_unittest.cc @@ -9,6 +9,7 @@ */ #include +#include "test/field_trial.h" #include "test/gtest.h" #include "test/scenario/scenario.h" @@ -170,6 +171,25 @@ TEST(VideoStreamTest, SendsFecWithFlexFec) { EXPECT_GT(video_stats.substreams.begin()->second.rtp_stats.fec.packets, 0u); } +TEST(VideoStreamTest, SendsFecWithDeferredFlexFec) { + ScopedFieldTrials trial("WebRTC-DeferredFecGeneration/Enabled/"); + Scenario s; + auto route = + s.CreateRoutes(s.CreateClient("caller", CallClientConfig()), + {s.CreateSimulationNode([](NetworkSimulationConfig* c) { + c->loss_rate = 0.1; + c->delay = TimeDelta::Millis(100); + })}, + s.CreateClient("callee", CallClientConfig()), + {s.CreateSimulationNode(NetworkSimulationConfig())}); + auto video = s.CreateVideoStream(route->forward(), [&](VideoStreamConfig* c) { + c->stream.use_flexfec = true; + }); + s.RunFor(TimeDelta::Seconds(5)); + VideoSendStream::Stats video_stats = video->send()->GetStats(); + EXPECT_GT(video_stats.substreams.begin()->second.rtp_stats.fec.packets, 0u); +} + TEST(VideoStreamTest, ResolutionAdaptsToAvailableBandwidth) { // Declared before scenario to avoid use after free. std::atomic num_qvga_frames_(0);