diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 854a18aa54..5f8d2df965 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -197,7 +197,6 @@ 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); @@ -245,9 +244,7 @@ std::vector CreateRtpStreamSenders( std::unique_ptr fec_generator = MaybeCreateFecGenerator(clock, rtp_config, suspended_ssrcs, i, trials); configuration.fec_generator = fec_generator.get(); - if (!use_deferred_fec) { - video_config.fec_generator = fec_generator.get(); - } + video_config.fec_generator = fec_generator.get(); configuration.rtx_send_ssrc = rtp_config.GetRtxSsrcAssociatedWithMediaSsrc(rtp_config.ssrcs[i]); @@ -341,9 +338,6 @@ 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)), @@ -362,7 +356,6 @@ RtpVideoSender::RtpVideoSender( frame_encryptor, crypto_options, std::move(frame_transformer), - use_deferred_fec_, field_trials_)), rtp_config_(rtp_config), codec_type_(GetVideoCodecType(rtp_config)), @@ -855,26 +848,14 @@ int RtpVideoSender::ProtectionRequest(const FecProtectionParams* delta_params, *sent_nack_rate_bps = 0; *sent_fec_rate_bps = 0; for (const RtpStreamSender& stream : rtp_streams_) { - 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(); + 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(); } return 0; } diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index e364729236..0c277d6aa7 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -176,7 +176,6 @@ 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 7e7a01b628..07e265b0da 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -441,9 +441,6 @@ 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); } @@ -562,11 +559,8 @@ void PacingController::ProcessPackets() { packet_size += DataSize::Bytes(rtp_packet->headers_size()) + transport_overhead_per_packet_; } - 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 775fdc9557..6e361aebb4 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -57,8 +57,6 @@ 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 9194d079a9..bc4d47333e 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -97,10 +97,6 @@ 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)); }; @@ -112,11 +108,6 @@ 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), @@ -134,10 +125,6 @@ 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 = @@ -171,10 +158,6 @@ class PacingControllerProbing : public PacingController::PacketSender { } } - std::vector> FetchFec() override { - return {}; - } - std::vector> GeneratePadding( DataSize target_size) override { // From RTPSender: @@ -316,7 +299,7 @@ class PacingControllerTest } SimulatedClock clock_; - ::testing::NiceMock callback_; + MockPacingControllerCallback callback_; std::unique_ptr pacer_; }; @@ -2046,38 +2029,6 @@ 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 833c56d6fd..e75b5a337a 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -42,9 +42,7 @@ 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) { - send_thread_checker_.Detach(); -} + transport_seq_(start_transport_seq) {} PacketRouter::~PacketRouter() { RTC_DCHECK(send_modules_map_.empty()); @@ -141,7 +139,6 @@ 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()); @@ -174,18 +171,6 @@ 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 b9395073bc..73837f2ffe 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -29,7 +29,6 @@ #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 { @@ -58,7 +57,6 @@ 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; @@ -130,10 +128,6 @@ 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 b02f387768..876cd96cfd 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -43,10 +43,6 @@ 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 46c310e276..869f3d218a 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -33,7 +33,6 @@ namespace webrtc { class RtpPacket; -class RtpPacketToSend; namespace rtcp { class TransportFeedback; } @@ -467,15 +466,5 @@ 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 d597b1e289..08b38eee7b 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -98,15 +98,6 @@ 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 9aaf9a52e6..8997bce0d2 100644 --- a/modules/rtp_rtcp/source/rtp_packet_to_send.h +++ b/modules/rtp_rtcp/source/rtp_packet_to_send.h @@ -108,15 +108,6 @@ 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_; @@ -125,8 +116,6 @@ 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 9f8bac54f4..7b9586ad78 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -389,17 +389,6 @@ bool ModuleRtpRtcpImpl::TrySendPacket(RtpPacketToSend* packet, return true; } -void ModuleRtpRtcpImpl::SetFecProtectionParams(const FecProtectionParams&, - const FecProtectionParams&) { - // Deferred FEC not supported in deprecated RTP module. -} - -std::vector> -ModuleRtpRtcpImpl::FetchFecPackets() { - // 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 1fa57db372..989b8d3717 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -139,11 +139,6 @@ 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 67cb70e1cb..70f05d7085 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -42,15 +42,11 @@ 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, this), + non_paced_sender(&packet_sender), 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) : worker_queue_(TaskQueueBase::Current()), @@ -337,31 +333,6 @@ 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 932c43656d..c04edfcb7f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h @@ -148,11 +148,6 @@ 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; @@ -273,9 +268,8 @@ class ModuleRtpRtcpImpl2 final : public RtpRtcpInterface, FRIEND_TEST_ALL_PREFIXES(RtpRtcpImpl2Test, Rtt); FRIEND_TEST_ALL_PREFIXES(RtpRtcpImpl2Test, RttForReceiverOnly); - struct RtpSenderContext : public SequenceNumberAssigner { + struct RtpSenderContext { 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 f763da244c..440837fc5d 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -293,17 +293,6 @@ 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 c8f10ee1f2..7196dcd659 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -10,7 +10,6 @@ #include "modules/rtp_rtcp/source/rtp_sender_egress.h" -#include #include #include #include @@ -39,45 +38,21 @@ bool IsEnabled(absl::string_view name, } // namespace RtpSenderEgress::NonPacedPacketSender::NonPacedPacketSender( - RtpSenderEgress* sender, - SequenceNumberAssigner* sequence_number_assigner) - : transport_sequence_number_(0), - sender_(sender), - sequence_number_assigner_(sequence_number_assigner) { - RTC_DCHECK(sequence_number_assigner_); -} + RtpSenderEgress* sender) + : transport_sequence_number_(0), sender_(sender) {} RtpSenderEgress::NonPacedPacketSender::~NonPacedPacketSender() = default; void RtpSenderEgress::NonPacedPacketSender::EnqueuePackets( std::vector> packets) { for (auto& packet : packets) { - PrepareForSend(packet.get()); + if (!packet->SetExtension( + ++transport_sequence_number_)) { + --transport_sequence_number_; + } + packet->ReserveExtension(); + packet->ReserveExtension(); 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, @@ -98,10 +73,6 @@ RtpSenderEgress::RtpSenderEgress(const RtpRtcpInterface::Configuration& config, is_audio_(config.audio), #endif 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), @@ -171,41 +142,6 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, })); } - 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); - absl::optional> - new_fec_params; - { - rtc::CritScope lock(&lock_); - new_fec_params.swap(pending_fec_params_); - } - if (new_fec_params) { - fec_generator_->SetProtectionParameters(new_fec_params->first, - new_fec_params->second); - } - 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 // the pacer, these modifications of the header below are happening after the // FEC protection packets are calculated. This will corrupt recovered packets @@ -272,14 +208,9 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, } if (send_success) { - // |media_has_been_sent_| is used by RTPSender to figure out if it can send - // padding in the absence of transport-cc or abs-send-time. - // In those cases media must be sent first to set a reference timestamp. + // TODO(tommi): Is this assuming is_media is true? media_has_been_sent_ = true; - // TODO(sprang): Add support for FEC protecting all header extensions, add - // media packet to generator here instead. - RTC_DCHECK(packet->packet_type().has_value()); RtpPacketMediaType packet_type = *packet->packet_type(); RtpPacketCounter counter(*packet); @@ -364,24 +295,6 @@ std::vector RtpSenderEgress::GetSentRtpPacketInfos( return results; } -void RtpSenderEgress::SetFecProtectionParameters( - const FecProtectionParams& delta_params, - const FecProtectionParams& key_params) { - // TODO(sprang): Post task to pacer queue instead, one pacer is fully - // migrated to a task queue. - rtc::CritScope lock(&lock_); - pending_fec_params_.emplace(delta_params, key_params); -} - -std::vector> -RtpSenderEgress::FetchFecPackets() { - RTC_DCHECK_RUN_ON(&pacer_checker_); - 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 e2cc00ff24..5b50ddfb93 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/rtp_sender_egress.h @@ -13,7 +13,6 @@ #include #include -#include #include #include "absl/types/optional.h" @@ -42,18 +41,15 @@ class RtpSenderEgress { // without passing through an actual paced sender. class NonPacedPacketSender : public RtpPacketSender { public: - NonPacedPacketSender(RtpSenderEgress* sender, - SequenceNumberAssigner* sequence_number_assigner); + explicit NonPacedPacketSender(RtpSenderEgress* sender); 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, @@ -86,10 +82,6 @@ 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 @@ -141,7 +133,6 @@ class RtpSenderEgress { const bool is_audio_; #endif const bool need_rtp_packet_infos_; - VideoFecGenerator* const fec_generator_ RTC_GUARDED_BY(pacer_checker_); TransportFeedbackObserver* const transport_feedback_observer_; SendSideDelayObserver* const send_side_delay_observer_; @@ -163,8 +154,6 @@ class RtpSenderEgress { StreamDataCounters rtx_rtp_stats_ RTC_GUARDED_BY(lock_); // One element per value in RtpPacketMediaType, with index matching value. std::vector send_rates_ RTC_GUARDED_BY(lock_); - absl::optional> - pending_fec_params_ RTC_GUARDED_BY(lock_); // Maps sent packets' sequence numbers to a tuple consisting of: // 1. The timestamp, without the randomizing offset mandated by the RFC. diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 9e1aefa6f9..dbd474cbe1 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -32,7 +32,6 @@ #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" @@ -142,10 +141,8 @@ MATCHER_P(SameRtcEventTypeAs, value, "") { } struct TestConfig { - TestConfig(bool with_overhead, bool deferred_fec) - : with_overhead(with_overhead), deferred_fec(deferred_fec) {} + explicit TestConfig(bool with_overhead) : with_overhead(with_overhead) {} bool with_overhead = false; - bool deferred_fec = false; }; class MockRtpPacketPacer : public RtpPacketSender { @@ -215,18 +212,15 @@ class StreamDataTestCallback : public StreamDataCountersCallback { // Mimics ModuleRtpRtcp::RtpSenderContext. // TODO(sprang): Split up unit tests and test these components individually // wherever possible. -struct RtpSenderContext : public SequenceNumberAssigner { +struct RtpSenderContext { 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_, this), + non_paced_sender_(&packet_sender_), 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_; @@ -235,14 +229,10 @@ struct RtpSenderContext : public SequenceNumberAssigner { class FieldTrialConfig : public WebRtcKeyValueConfig { public: - FieldTrialConfig() - : overhead_enabled_(false), - deferred_fec_(false), - max_padding_factor_(1200) {} + FieldTrialConfig() : overhead_enabled_(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 { @@ -253,15 +243,12 @@ 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_; }; @@ -282,7 +269,6 @@ 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); } @@ -300,20 +286,12 @@ 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 = fec_generator; + 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_; @@ -1272,7 +1250,6 @@ 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); @@ -1283,11 +1260,7 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_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_generator = &flexfec_sender; video_config.fec_type = flexfec_sender.GetFecType(); video_config.fec_overhead_bytes = flexfec_sender.MaxPacketOverhead(); video_config.field_trials = &field_trials; @@ -1313,21 +1286,11 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { 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(fec_packet->Ssrc(), kFlexFecSsrc); - } } else { EXPECT_EQ(packet->packet_type(), RtpPacketMediaType::kForwardErrorCorrection); + EXPECT_EQ(packet->Ssrc(), kFlexFecSsrc); fec_packet = std::move(packet); - EXPECT_EQ(fec_packet->Ssrc(), kFlexFecSsrc); } } }); @@ -1375,7 +1338,6 @@ 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); @@ -1384,9 +1346,7 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - if (!GetParam().deferred_fec) { - video_config.fec_generator = &flexfec_sender; - } + 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; @@ -1397,11 +1357,7 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { params.fec_rate = 15; params.max_fec_frames = 1; params.fec_mask_type = kFecMaskRandom; - if (GetParam().deferred_fec) { - rtp_egress()->SetFecProtectionParameters(params, params); - } else { - flexfec_sender.SetProtectionParameters(params, params); - } + flexfec_sender.SetProtectionParameters(params, params); EXPECT_CALL(mock_rtc_event_log_, LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) @@ -1706,16 +1662,25 @@ TEST_P(RtpSenderTest, FecOverheadRate) { kNoRtpExtensions, kNoRtpExtensionSizes, nullptr /* rtp_state */, &fake_clock_); - // Reset |rtp_sender_| to use this FlexFEC instance. - SetUpRtpSender(false, false, false, &flexfec_sender); + // 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); FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = &fake_clock_; video_config.rtp_sender = rtp_sender(); - if (!GetParam().deferred_fec) { - video_config.fec_generator = &flexfec_sender; - } + 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; @@ -1725,15 +1690,12 @@ TEST_P(RtpSenderTest, FecOverheadRate) { params.fec_rate = 15; params.max_fec_frames = 1; params.fec_mask_type = kFecMaskRandom; - if (GetParam().deferred_fec) { - rtp_egress()->SetFecProtectionParameters(params, params); - } else { - flexfec_sender.SetProtectionParameters(params, params); - } + 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; @@ -1751,21 +1713,9 @@ TEST_P(RtpSenderTest, FecOverheadRate) { constexpr size_t kPayloadLength = sizeof(kPayloadData); constexpr size_t kPacketLength = kRtpHeaderLength + kFlexfecHeaderLength + kGenericCodecHeaderLength + kPayloadLength; - - loop_.Flush(); - 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); - } + EXPECT_NEAR(kNumFecPackets * kPacketLength * 8 / + (kNumFecPackets * kTimeBetweenPacketsMs / 1000.0f), + flexfec_sender.CurrentFecRate().bps(), 500); } TEST_P(RtpSenderTest, BitrateCallbacks) { @@ -1912,18 +1862,15 @@ 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; - if (!GetParam().deferred_fec) { - video_config.fec_generator = &ulpfec_generator; - } + 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); @@ -1940,11 +1887,7 @@ TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacksUlpfec) { fec_params.fec_mask_type = kFecMaskRandom; fec_params.fec_rate = 1; fec_params.max_fec_frames = 1; - if (GetParam().deferred_fec) { - rtp_egress()->SetFecProtectionParameters(fec_params, fec_params); - } else { - ulpfec_generator.SetProtectionParameters(fec_params, fec_params); - } + 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, @@ -2770,16 +2713,12 @@ TEST_P(RtpSenderTest, IgnoresNackAfterDisablingMedia) { INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTest, - ::testing::Values(TestConfig{false, false}, - TestConfig{false, true}, - TestConfig{true, false}, - TestConfig{false, false})); + ::testing::Values(TestConfig{false}, + TestConfig{true})); INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTestWithoutPacer, - ::testing::Values(TestConfig{false, false}, - TestConfig{false, true}, - TestConfig{true, false}, - TestConfig{false, false})); + ::testing::Values(TestConfig{false}, + TestConfig{true})); } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 7d870d25a8..9ebfa77b8a 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -598,8 +598,8 @@ bool RTPSenderVideo::SendVideo( if (fec_generator_) { fec_generator_->AddPacketAndGenerateFec(*packet); } else { - // Deferred FEC generation, just mark packet. - packet->set_fec_protect_packet(true); + // TODO(sprang): When deferred FEC generation is enabled, just mark the + // packet as protected here. } } @@ -607,7 +607,6 @@ 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 04cb59dc7d..265fa4d1ac 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator.cc @@ -230,8 +230,6 @@ 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 873ef639ba..37afe1b1e7 100644 --- a/test/scenario/video_stream_unittest.cc +++ b/test/scenario/video_stream_unittest.cc @@ -9,7 +9,6 @@ */ #include -#include "test/field_trial.h" #include "test/gtest.h" #include "test/scenario/scenario.h" @@ -171,25 +170,6 @@ 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);