From 848ea9f0d3678118cb8926a2898454e5a4df58ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 25 May 2020 12:04:14 +0200 Subject: [PATCH] Lets PacingController call PacketRouter directly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since locking model has been cleaned up, PacingController can now call PacketRouter directly - without having to go via PacedSender or TaskQueuePacedSender. Bug: webrtc:10809 Change-Id: I181f04167d677c35395286f8b246aefb4c3e7ec7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175909 Reviewed-by: Sebastian Jansson Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#31342} --- modules/pacing/paced_sender.cc | 19 +++----- modules/pacing/paced_sender.h | 12 +---- modules/pacing/paced_sender_unittest.cc | 2 +- modules/pacing/pacing_controller.cc | 4 +- modules/pacing/pacing_controller.h | 4 +- modules/pacing/pacing_controller_unittest.cc | 46 +++++++++---------- modules/pacing/packet_router.cc | 6 +-- modules/pacing/packet_router.h | 13 +++--- modules/pacing/packet_router_unittest.cc | 19 ++++---- modules/pacing/task_queue_paced_sender.cc | 14 +----- modules/pacing/task_queue_paced_sender.h | 14 +----- .../task_queue_paced_sender_unittest.cc | 2 +- 12 files changed, 58 insertions(+), 97 deletions(-) diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 1d02fe95e4..88effe4b6a 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -28,7 +28,8 @@ namespace webrtc { const int64_t PacedSender::kMaxQueueLengthMs = 2000; const float PacedSender::kDefaultPaceMultiplier = 2.5f; -PacedSender::PacedSender(Clock* clock, PacketRouter* packet_router, +PacedSender::PacedSender(Clock* clock, + PacketRouter* packet_router, RtcEventLog* event_log, const WebRtcKeyValueConfig* field_trials, ProcessThread* process_thread) @@ -39,10 +40,11 @@ PacedSender::PacedSender(Clock* clock, PacketRouter* packet_router, ? PacingController::ProcessMode::kDynamic : PacingController::ProcessMode::kPeriodic), pacing_controller_(clock, - static_cast(this), - event_log, field_trials, process_mode_), + packet_router, + event_log, + field_trials, + process_mode_), clock_(clock), - packet_router_(packet_router), process_thread_(process_thread) { if (process_thread_) process_thread_->RegisterModule(&module_proxy_, RTC_FROM_HERE); @@ -194,13 +196,4 @@ void PacedSender::SetQueueTimeLimit(TimeDelta limit) { MaybeWakupProcessThread(); } -void PacedSender::SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info) { - packet_router_->SendPacket(std::move(packet), cluster_info); -} - -std::vector> PacedSender::GeneratePadding( - DataSize size) { - return packet_router_->GeneratePadding(size.bytes()); -} } // namespace webrtc diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 16137dfcd6..cc83b403ba 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -43,8 +43,7 @@ class RtcEventLog; // updating dependencies. class PacedSender : public Module, public RtpPacketPacer, - public RtpPacketSender, - private PacingController::PacketSender { + public RtpPacketSender { public: // Expected max pacer delay in ms. If ExpectedQueueTime() is higher than // this value, the packet producers should wait (eg drop frames rather than @@ -140,14 +139,6 @@ class PacedSender : public Module, // In dynamic process mode, refreshes the next process time. void MaybeWakupProcessThread(); - // Methods implementing PacedSenderController:PacketSender. - void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info) override - RTC_EXCLUSIVE_LOCKS_REQUIRED(critsect_); - - std::vector> GeneratePadding( - DataSize size) override RTC_EXCLUSIVE_LOCKS_REQUIRED(critsect_); - // Private implementation of Module to not expose those implementation details // publicly and control when the class is registered/deregistered. class ModuleProxy : public Module { @@ -171,7 +162,6 @@ class PacedSender : public Module, PacingController pacing_controller_ RTC_GUARDED_BY(critsect_); Clock* const clock_; - PacketRouter* const packet_router_; ProcessThread* const process_thread_; }; } // namespace webrtc diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 26d2eac413..eaee276270 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -44,7 +44,7 @@ class MockCallback : public PacketRouter { const PacedPacketInfo& cluster_info)); MOCK_METHOD1( GeneratePadding, - std::vector>(size_t target_size_bytes)); + std::vector>(DataSize target_size)); }; class ProcessModeTrials : public WebRtcKeyValueConfig { diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index b1f6e896a7..77f21bedbe 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -438,7 +438,7 @@ void PacingController::ProcessPackets() { for (auto& packet : keepalive_packets) { keepalive_data_sent += DataSize::Bytes(packet->payload_size() + packet->padding_size()); - packet_sender_->SendRtpPacket(std::move(packet), PacedPacketInfo()); + packet_sender_->SendPacket(std::move(packet), PacedPacketInfo()); } OnPaddingSent(keepalive_data_sent); } @@ -557,7 +557,7 @@ void PacingController::ProcessPackets() { packet_size += DataSize::Bytes(rtp_packet->headers_size()) + transport_overhead_per_packet_; } - packet_sender_->SendRtpPacket(std::move(rtp_packet), pacing_info); + packet_sender_->SendPacket(std::move(rtp_packet), pacing_info); data_sent += packet_size; diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index 20d2539e45..6e361aebb4 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -55,8 +55,8 @@ class PacingController { class PacketSender { public: virtual ~PacketSender() = default; - virtual void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info) = 0; + virtual void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& cluster_info) = 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 fa23da70a0..6d6e6e2d6f 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -69,8 +69,8 @@ std::unique_ptr BuildPacket(RtpPacketMediaType type, // methods that focus on core aspects. class MockPacingControllerCallback : public PacingController::PacketSender { public: - void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info) override { + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& cluster_info) override { SendPacket(packet->Ssrc(), packet->SequenceNumber(), packet->capture_time_ms(), packet->packet_type() == RtpPacketMediaType::kRetransmission, @@ -102,7 +102,7 @@ class MockPacingControllerCallback : public PacingController::PacketSender { // Mock callback implementing the raw api. class MockPacketSender : public PacingController::PacketSender { public: - MOCK_METHOD2(SendRtpPacket, + MOCK_METHOD2(SendPacket, void(std::unique_ptr packet, const PacedPacketInfo& cluster_info)); MOCK_METHOD1( @@ -116,8 +116,8 @@ class PacingControllerPadding : public PacingController::PacketSender { PacingControllerPadding() : padding_sent_(0), total_bytes_sent_(0) {} - void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& pacing_info) override { + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) override { total_bytes_sent_ += packet->payload_size(); } @@ -147,8 +147,8 @@ class PacingControllerProbing : public PacingController::PacketSender { public: PacingControllerProbing() : packets_sent_(0), padding_sent_(0) {} - void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& pacing_info) override { + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& pacing_info) override { if (packet->packet_type() != RtpPacketMediaType::kPadding) { ++packets_sent_; } @@ -1571,7 +1571,7 @@ TEST_P(PacingControllerTest, ProbeClusterId) { // First probing cluster. EXPECT_CALL(callback, - SendRtpPacket(_, Field(&PacedPacketInfo::probe_cluster_id, 0))) + SendPacket(_, Field(&PacedPacketInfo::probe_cluster_id, 0))) .Times(5); for (int i = 0; i < 5; ++i) { @@ -1580,7 +1580,7 @@ TEST_P(PacingControllerTest, ProbeClusterId) { // Second probing cluster. EXPECT_CALL(callback, - SendRtpPacket(_, Field(&PacedPacketInfo::probe_cluster_id, 1))) + SendPacket(_, Field(&PacedPacketInfo::probe_cluster_id, 1))) .Times(5); for (int i = 0; i < 5; ++i) { @@ -1598,7 +1598,7 @@ TEST_P(PacingControllerTest, ProbeClusterId) { return padding_packets; }); bool non_probe_packet_seen = false; - EXPECT_CALL(callback, SendRtpPacket) + EXPECT_CALL(callback, SendPacket) .WillOnce([&](std::unique_ptr packet, const PacedPacketInfo& cluster_info) { EXPECT_EQ(cluster_info.probe_cluster_id, kNotAProbe); @@ -1628,23 +1628,23 @@ TEST_P(PacingControllerTest, OwnedPacketPrioritizedOnType) { ::testing::InSequence seq; EXPECT_CALL( callback, - SendRtpPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kAudioSsrc)), _)); - EXPECT_CALL(callback, - SendRtpPacket( - Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _)); + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kAudioSsrc)), _)); + EXPECT_CALL( + callback, + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _)); // FEC and video actually have the same priority, so will come out in // insertion order. - EXPECT_CALL(callback, - SendRtpPacket( - Pointee(Property(&RtpPacketToSend::Ssrc, kFlexFecSsrc)), _)); EXPECT_CALL( callback, - SendRtpPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoSsrc)), _)); + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kFlexFecSsrc)), _)); + EXPECT_CALL( + callback, + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoSsrc)), _)); - EXPECT_CALL(callback, - SendRtpPacket( - Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _)); + EXPECT_CALL( + callback, + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _)); while (pacer_->QueueSizePackets() > 0) { if (PeriodicProcess()) { @@ -1679,7 +1679,7 @@ TEST_P(PacingControllerTest, SmallFirstProbePacket) { size_t packets_sent = 0; bool media_seen = false; - EXPECT_CALL(callback, SendRtpPacket) + EXPECT_CALL(callback, SendPacket) .Times(::testing::AnyNumber()) .WillRepeatedly([&](std::unique_ptr packet, const PacedPacketInfo& cluster_info) { @@ -1817,7 +1817,7 @@ TEST_P(PacingControllerTest, for (bool account_for_audio : {false, true}) { uint16_t sequence_number = 1234; MockPacketSender callback; - EXPECT_CALL(callback, SendRtpPacket).Times(::testing::AnyNumber()); + EXPECT_CALL(callback, SendPacket).Times(::testing::AnyNumber()); pacer_ = std::make_unique(&clock_, &callback, nullptr, nullptr, GetParam()); pacer_->SetAccountForAudioPackets(account_for_audio); diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index fa64331493..e9e8d4bd23 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -167,7 +167,7 @@ void PacketRouter::SendPacket(std::unique_ptr packet, } std::vector> PacketRouter::GeneratePadding( - size_t target_size_bytes) { + DataSize size) { rtc::CritScope cs(&modules_crit_); // First try on the last rtp module to have sent media. This increases the // the chance that any payload based padding will be useful as it will be @@ -178,7 +178,7 @@ std::vector> PacketRouter::GeneratePadding( std::vector> padding_packets; if (last_send_module_ != nullptr && last_send_module_->SupportsRtxPayloadPadding()) { - padding_packets = last_send_module_->GeneratePadding(target_size_bytes); + padding_packets = last_send_module_->GeneratePadding(size.bytes()); if (!padding_packets.empty()) { return padding_packets; } @@ -189,7 +189,7 @@ std::vector> PacketRouter::GeneratePadding( // be taken into account by the bandwidth estimator, e.g. in FF. for (RtpRtcp* rtp_module : send_modules_list_) { if (rtp_module->SupportsPadding()) { - padding_packets = rtp_module->GeneratePadding(target_size_bytes); + padding_packets = rtp_module->GeneratePadding(size.bytes()); if (!padding_packets.empty()) { last_send_module_ = rtp_module; break; diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 40b3ad1407..2c03e9623a 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -21,6 +21,7 @@ #include #include "api/transport/network_types.h" +#include "modules/pacing/pacing_controller.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" @@ -39,7 +40,8 @@ class RtpRtcp; // (receiver report). For the latter case, we also keep track of the // receive modules. class PacketRouter : public RemoteBitrateObserver, - public TransportFeedbackSenderInterface { + public TransportFeedbackSenderInterface, + public PacingController::PacketSender { public: PacketRouter(); explicit PacketRouter(uint16_t start_transport_seq); @@ -52,11 +54,10 @@ class PacketRouter : public RemoteBitrateObserver, bool remb_candidate); void RemoveReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender); - virtual void SendPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info); - - virtual std::vector> GeneratePadding( - size_t target_size_bytes); + void SendPacket(std::unique_ptr packet, + const PacedPacketInfo& cluster_info) override; + std::vector> GeneratePadding( + DataSize size) override; uint16_t CurrentTransportSequenceNumber() const; diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index b8f16cb924..75729cb544 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -68,7 +68,7 @@ class PacketRouterTest : public ::testing::Test { }; TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_GeneratePadding) { - constexpr size_t bytes = 300; + constexpr DataSize bytes = DataSize::Bytes(300); const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes); EXPECT_TRUE(packet_router_.GeneratePadding(bytes).empty()); @@ -122,7 +122,8 @@ TEST_F(PacketRouterTest, GeneratePaddingPrioritizesRtx) { return std::vector>( kExpectedPaddingPackets); }); - auto generated_padding = packet_router_.GeneratePadding(kPaddingSize); + auto generated_padding = + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingSize)); EXPECT_EQ(generated_padding.size(), kExpectedPaddingPackets); packet_router_.RemoveSendRtpModule(&rtp_1); @@ -159,7 +160,7 @@ TEST_F(PacketRouterTest, GeneratePaddingPrioritizesVideo) { packet_router_.AddSendRtpModule(&audio_module, false); EXPECT_CALL(audio_module, GeneratePadding(kPaddingSize)) .WillOnce(generate_padding); - packet_router_.GeneratePadding(kPaddingSize); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingSize)); // Add the video module, this should now be prioritized since we cannot // guarantee that audio packets will be included in the BWE. @@ -167,7 +168,7 @@ TEST_F(PacketRouterTest, GeneratePaddingPrioritizesVideo) { EXPECT_CALL(audio_module, GeneratePadding).Times(0); EXPECT_CALL(video_module, GeneratePadding(kPaddingSize)) .WillOnce(generate_padding); - packet_router_.GeneratePadding(kPaddingSize); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingSize)); // Remove and the add audio module again. Module order shouldn't matter; // video should still be prioritized. @@ -176,14 +177,14 @@ TEST_F(PacketRouterTest, GeneratePaddingPrioritizesVideo) { EXPECT_CALL(audio_module, GeneratePadding).Times(0); EXPECT_CALL(video_module, GeneratePadding(kPaddingSize)) .WillOnce(generate_padding); - packet_router_.GeneratePadding(kPaddingSize); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingSize)); // Remove and the video module, we should fall back to padding on the // audio module again. packet_router_.RemoveSendRtpModule(&video_module); EXPECT_CALL(audio_module, GeneratePadding(kPaddingSize)) .WillOnce(generate_padding); - packet_router_.GeneratePadding(kPaddingSize); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingSize)); packet_router_.RemoveSendRtpModule(&audio_module); } @@ -243,7 +244,7 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { packets.push_back(BuildRtpPacket(kSsrc2)); return packets; }); - packet_router_.GeneratePadding(kPaddingBytes); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingBytes)); // Send media on first module. Padding should be sent on that module. packet_router_.SendPacket(BuildRtpPacket(kSsrc1), PacedPacketInfo()); @@ -255,7 +256,7 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { packets.push_back(BuildRtpPacket(kSsrc1)); return packets; }); - packet_router_.GeneratePadding(kPaddingBytes); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingBytes)); // Send media on second module. Padding should be sent there. packet_router_.SendPacket(BuildRtpPacket(kSsrc2), PacedPacketInfo()); @@ -285,7 +286,7 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { for (int i = 0; i < 2; ++i) { last_send_module = nullptr; - packet_router_.GeneratePadding(kPaddingBytes); + packet_router_.GeneratePadding(DataSize::Bytes(kPaddingBytes)); EXPECT_NE(last_send_module, nullptr); packet_router_.RemoveSendRtpModule(last_send_module); } diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index d460d60048..41eebea229 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc @@ -38,9 +38,8 @@ TaskQueuePacedSender::TaskQueuePacedSender( TimeDelta hold_back_window) : clock_(clock), hold_back_window_(hold_back_window), - packet_router_(packet_router), pacing_controller_(clock, - static_cast(this), + packet_router, event_log, field_trials, PacingController::ProcessMode::kDynamic), @@ -221,17 +220,6 @@ void TaskQueuePacedSender::MaybeProcessPackets( MaybeUpdateStats(false); } -std::vector> -TaskQueuePacedSender::GeneratePadding(DataSize size) { - return packet_router_->GeneratePadding(size.bytes()); -} - -void TaskQueuePacedSender::SendRtpPacket( - std::unique_ptr packet, - const PacedPacketInfo& cluster_info) { - packet_router_->SendPacket(std::move(packet), cluster_info); -} - void TaskQueuePacedSender::MaybeUpdateStats(bool is_scheduled_call) { if (is_shutdown_) { return; diff --git a/modules/pacing/task_queue_paced_sender.h b/modules/pacing/task_queue_paced_sender.h index 3241d3fb63..5e6a1770c2 100644 --- a/modules/pacing/task_queue_paced_sender.h +++ b/modules/pacing/task_queue_paced_sender.h @@ -38,9 +38,7 @@ namespace webrtc { class Clock; class RtcEventLog; -class TaskQueuePacedSender : public RtpPacketPacer, - public RtpPacketSender, - private PacingController::PacketSender { +class TaskQueuePacedSender : public RtpPacketPacer, public RtpPacketSender { public: // The |hold_back_window| parameter sets a lower bound on time to sleep if // there is currently a pacer queue and packets can't immediately be @@ -125,21 +123,11 @@ class TaskQueuePacedSender : public RtpPacketPacer, // method again with desired (finite) scheduled process time. void MaybeProcessPackets(Timestamp scheduled_process_time); - // Methods implementing PacedSenderController:PacketSender. - - void SendRtpPacket(std::unique_ptr packet, - const PacedPacketInfo& cluster_info) override - RTC_RUN_ON(task_queue_); - - std::vector> GeneratePadding( - DataSize size) override RTC_RUN_ON(task_queue_); - void MaybeUpdateStats(bool is_scheduled_call) RTC_RUN_ON(task_queue_); Stats GetStats() const; Clock* const clock_; const TimeDelta hold_back_window_; - PacketRouter* const packet_router_ RTC_GUARDED_BY(task_queue_); PacingController pacing_controller_ RTC_GUARDED_BY(task_queue_); // We want only one (valid) delayed process task in flight at a time. diff --git a/modules/pacing/task_queue_paced_sender_unittest.cc b/modules/pacing/task_queue_paced_sender_unittest.cc index e93f776f38..891936df0f 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -43,7 +43,7 @@ class MockPacketRouter : public PacketRouter { const PacedPacketInfo& cluster_info)); MOCK_METHOD1( GeneratePadding, - std::vector>(size_t target_size_bytes)); + std::vector>(DataSize target_size)); }; } // namespace