From c06aef2ad1b3916174eb76a6bf2f0d56fdd97699 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 17 Oct 2019 13:02:27 +0200 Subject: [PATCH] Reland "Use just a lookup map of RTP modules in PacketRouter" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 96f3de094566f32d842be6dd0906f1d13b8c8825 Downstream test is fixed, this is a pure reland. TBR=danilchap@webrtc.org,srte@webrtc.org Original change's description: > Use just a lookup map of RTP modules in PacketRouter > > Since SSRCs of RTP modules are now set at construction time, we can > use just a simple unordered map from SSRC to module in packet router. > > Bug: webrtc:11036 > Change-Id: I0b3527f17c9ee2df9253c778e5b9e3651a70b355 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/155965 > Commit-Queue: Erik Språng > Reviewed-by: Sebastian Jansson > Reviewed-by: Danil Chapovalov > Cr-Commit-Position: refs/heads/master@{#29510} Bug: webrtc:11036 Change-Id: I0731339dfd0781cc7f2f7ca78ac903539f25ff9c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157304 Reviewed-by: Erik Språng Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29514} --- modules/pacing/packet_router.cc | 148 ++++++++++------------- modules/pacing/packet_router.h | 20 ++- modules/pacing/packet_router_unittest.cc | 66 +++++----- modules/rtp_rtcp/include/rtp_rtcp.h | 3 + modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 + modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 4 + modules/rtp_rtcp/source/rtp_rtcp_impl.h | 1 + modules/rtp_rtcp/source/rtp_sender.h | 5 +- video/video_send_stream_tests.cc | 2 +- 9 files changed, 121 insertions(+), 129 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 56922b73a4..8edfd1fe28 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -44,7 +44,8 @@ PacketRouter::PacketRouter(uint16_t start_transport_seq) transport_seq_(start_transport_seq) {} PacketRouter::~PacketRouter() { - RTC_DCHECK(rtp_send_modules_.empty()); + RTC_DCHECK(send_modules_map_.empty()); + RTC_DCHECK(send_modules_list_.empty()); RTC_DCHECK(rtcp_feedback_senders_.empty()); RTC_DCHECK(sender_remb_candidates_.empty()); RTC_DCHECK(receiver_remb_candidates_.empty()); @@ -53,14 +54,17 @@ PacketRouter::~PacketRouter() { void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) { rtc::CritScope cs(&modules_crit_); - RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), - rtp_module) == rtp_send_modules_.end()); - // Put modules which can use regular payload packets (over rtx) instead of - // padding first as it's less of a waste + + AddSendRtpModuleToMap(rtp_module, rtp_module->SSRC()); + if (absl::optional rtx_ssrc = rtp_module->RtxSsrc()) { + AddSendRtpModuleToMap(rtp_module, *rtx_ssrc); + } + if (absl::optional flexfec_ssrc = rtp_module->FlexfecSsrc()) { + AddSendRtpModuleToMap(rtp_module, *flexfec_ssrc); + } + if (rtp_module->SupportsRtxPayloadPadding()) { - rtp_send_modules_.push_front(rtp_module); - } else { - rtp_send_modules_.push_back(rtp_module); + last_send_module_ = rtp_module; } if (remb_candidate) { @@ -68,14 +72,32 @@ void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) { } } +void PacketRouter::AddSendRtpModuleToMap(RtpRtcp* rtp_module, uint32_t ssrc) { + RTC_DCHECK(send_modules_map_.find(ssrc) == send_modules_map_.end()); + send_modules_list_.push_front(rtp_module); + send_modules_map_[ssrc] = std::pair::iterator>( + rtp_module, send_modules_list_.begin()); +} + +void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) { + auto kv = send_modules_map_.find(ssrc); + RTC_DCHECK(kv != send_modules_map_.end()); + send_modules_list_.erase(kv->second.second); + send_modules_map_.erase(kv); +} + void PacketRouter::RemoveSendRtpModule(RtpRtcp* rtp_module) { rtc::CritScope cs(&modules_crit_); - rtp_module_cache_map_.clear(); MaybeRemoveRembModuleCandidate(rtp_module, /* media_sender = */ true); - auto it = - std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module); - RTC_DCHECK(it != rtp_send_modules_.end()); - rtp_send_modules_.erase(it); + + RemoveSendRtpModuleFromMap(rtp_module->SSRC()); + if (absl::optional rtx_ssrc = rtp_module->RtxSsrc()) { + RemoveSendRtpModuleFromMap(*rtx_ssrc); + } + if (absl::optional flexfec_ssrc = rtp_module->FlexfecSsrc()) { + RemoveSendRtpModuleFromMap(*flexfec_ssrc); + } + if (last_send_module_ == rtp_module) { last_send_module_ = nullptr; } @@ -105,25 +127,6 @@ void PacketRouter::RemoveReceiveRtpModule( rtcp_feedback_senders_.erase(it); } -RtpRtcp* PacketRouter::FindRtpModule(uint32_t ssrc) { - auto it = rtp_module_cache_map_.find(ssrc); - if (it != rtp_module_cache_map_.end()) { - if (ssrc == it->second->SSRC() || ssrc == it->second->FlexfecSsrc()) { - return it->second; - } - // This entry is stale due to a changed ssrc - remove it. - rtp_module_cache_map_.erase(it); - } - // Slow path - find and cache matching module - for (RtpRtcp* rtp_module : rtp_send_modules_) { - if (ssrc == rtp_module->SSRC() || ssrc == rtp_module->FlexfecSsrc()) { - rtp_module_cache_map_[ssrc] = rtp_module; - return rtp_module; - } - } - return nullptr; -} - void PacketRouter::SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info) { rtc::CritScope cs(&modules_crit_); @@ -133,26 +136,27 @@ void PacketRouter::SendPacket(std::unique_ptr packet, packet->SetExtension(AllocateSequenceNumber()); } - auto it = rtp_module_cache_map_.find(packet->Ssrc()); - if (it != rtp_module_cache_map_.end()) { - if (TrySendPacket(packet.get(), cluster_info, it->second)) { - return; - } - // Entry is stale, remove it. - rtp_module_cache_map_.erase(it); + uint32_t ssrc = packet->Ssrc(); + auto kv = send_modules_map_.find(ssrc); + if (kv == send_modules_map_.end()) { + RTC_LOG(LS_WARNING) + << "Failed to send packet, matching RTP module not found " + "or transport error. SSRC = " + << packet->Ssrc() << ", sequence number " << packet->SequenceNumber(); + return; } - // Slow path, find the correct send module. - for (auto* rtp_module : rtp_send_modules_) { - if (TrySendPacket(packet.get(), cluster_info, rtp_module)) { - return; - } + RtpRtcp* rtp_module = kv->second.first; + if (!rtp_module->TrySendPacket(packet.get(), cluster_info)) { + RTC_LOG(LS_WARNING) << "Failed to send packet, rejected by RTP module."; + return; } - RTC_LOG(LS_WARNING) << "Failed to send packet, matching RTP module not found " - "or transport error. SSRC = " - << packet->Ssrc() << ", sequence number " - << packet->SequenceNumber(); + if (rtp_module->SupportsRtxPayloadPadding()) { + // This is now the last module to send media, and has the desired + // properties needed for payload based padding. Cache it for later use. + last_send_module_ = rtp_module; + } } std::vector> PacketRouter::GeneratePadding( @@ -164,25 +168,26 @@ std::vector> PacketRouter::GeneratePadding( // will be more skewed towards the highest bitrate stream. At the very least // this prevents sending payload padding on a disabled stream where it's // guaranteed not to be useful. + std::vector> padding_packets; if (last_send_module_ != nullptr && last_send_module_->SupportsRtxPayloadPadding()) { - RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), - last_send_module_) != rtp_send_modules_.end()); - return last_send_module_->GeneratePadding(target_size_bytes); - } - - // Rtp modules are ordered by which stream can most benefit from padding. - for (RtpRtcp* rtp_module : rtp_send_modules_) { - if (rtp_module->SupportsPadding()) { - auto padding_packets = rtp_module->GeneratePadding(target_size_bytes); - if (!padding_packets.empty()) { - last_send_module_ = rtp_module; - } + padding_packets = last_send_module_->GeneratePadding(target_size_bytes); + if (!padding_packets.empty()) { return padding_packets; } } - return {}; + for (RtpRtcp* rtp_module : send_modules_list_) { + if (rtp_module->SupportsPadding()) { + padding_packets = rtp_module->GeneratePadding(target_size_bytes); + if (!padding_packets.empty()) { + last_send_module_ = rtp_module; + break; + } + } + } + + return padding_packets; } void PacketRouter::SetTransportWideSequenceNumber(uint16_t sequence_number) { @@ -276,7 +281,7 @@ bool PacketRouter::SendCombinedRtcpPacket( rtc::CritScope cs(&modules_crit_); // Prefer send modules. - for (auto* rtp_module : rtp_send_modules_) { + for (RtpRtcp* rtp_module : send_modules_list_) { if (rtp_module->RTCP() == RtcpMode::kOff) { continue; } @@ -352,23 +357,4 @@ void PacketRouter::DetermineActiveRembModule() { active_remb_module_ = new_active_remb_module; } -bool PacketRouter::TrySendPacket(RtpPacketToSend* packet, - const PacedPacketInfo& cluster_info, - RtpRtcp* rtp_module) { - uint32_t ssrc = packet->Ssrc(); - if (rtp_module->TrySendPacket(packet, cluster_info)) { - // Sending succeeded, make sure this SSRC mapping for future use. - rtp_module_cache_map_[ssrc] = rtp_module; - - if (rtp_module->SupportsRtxPayloadPadding()) { - // This is now the last module to send media, and has the desired - // properties needed for payload based padding. Cache it for later use. - last_send_module_ = rtp_module; - } - - return true; - } - return false; -} - } // namespace webrtc diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 3680bce3d9..1359e04332 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include "api/transport/network_types.h" @@ -84,9 +85,6 @@ class PacketRouter : public RemoteBitrateObserver, std::vector> packets) override; private: - RtpRtcp* FindRtpModule(uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); - void AddRembModuleCandidate(RtcpFeedbackSenderInterface* candidate_module, bool media_sender) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); @@ -95,17 +93,17 @@ class PacketRouter : public RemoteBitrateObserver, bool media_sender) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); void UnsetActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); void DetermineActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); - bool TrySendPacket(RtpPacketToSend* packet, - const PacedPacketInfo& cluster_info, - RtpRtcp* rtp_module) + void AddSendRtpModuleToMap(RtpRtcp* rtp_module, uint32_t ssrc) + RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void RemoveSendRtpModuleFromMap(uint32_t ssrc) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); rtc::CriticalSection modules_crit_; - // Rtp and Rtcp modules of the rtp senders. - std::list rtp_send_modules_ RTC_GUARDED_BY(modules_crit_); - // Ssrc to RtpRtcp module cache. - std::unordered_map rtp_module_cache_map_ - RTC_GUARDED_BY(modules_crit_); + // Ssrc to RtpRtcp module and iterator into |send_modules_list_|; + std::unordered_map::iterator>> + send_modules_map_ RTC_GUARDED_BY(modules_crit_); + std::list send_modules_list_ RTC_GUARDED_BY(modules_crit_); // The last module used to send media. RtpRtcp* last_send_module_ RTC_GUARDED_BY(modules_crit_); // Rtcp modules of the rtp receivers. diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 1239201a6c..0c95e7fa76 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -179,8 +179,10 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { // and supports rtx. EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([](size_t target_size_bytes) { - return std::vector>(); + .WillOnce([&](size_t target_size_bytes) { + std::vector> packets; + packets.push_back(BuildRtpPacket(kSsrc2)); + return packets; }); packet_router_.GeneratePadding(kPaddingBytes); @@ -189,41 +191,45 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([](size_t target_size_bytes) { - return std::vector>(); + .WillOnce([&](size_t target_size_bytes) { + std::vector> packets; + packets.push_back(BuildRtpPacket(kSsrc1)); + return packets; }); packet_router_.GeneratePadding(kPaddingBytes); // Send media on second module. Padding should be sent there. packet_router_.SendPacket(BuildRtpPacket(kSsrc2), PacedPacketInfo()); - EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) - .Times(1) - .WillOnce([](size_t target_size_bytes) { - return std::vector>(); - }); - packet_router_.GeneratePadding(kPaddingBytes); - - // Remove second module, padding should now fall back to first module. + // If the last active module is removed, and no module sends media before + // the next padding request, and arbitrary module will be selected. packet_router_.RemoveSendRtpModule(&rtp_2); + + // Send on and then remove all remaining modules. + RtpRtcp* last_send_module; EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([](size_t target_size_bytes) { - return std::vector>(); + .WillOnce([&](size_t target_size_bytes) { + last_send_module = &rtp_1; + std::vector> packets; + packets.push_back(BuildRtpPacket(kSsrc1)); + return packets; }); - packet_router_.GeneratePadding(kPaddingBytes); - - // Remove first module too, leaving only the one without rtx. - packet_router_.RemoveSendRtpModule(&rtp_1); - EXPECT_CALL(rtp_3, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([](size_t target_size_bytes) { - return std::vector>(); + .WillOnce([&](size_t target_size_bytes) { + last_send_module = &rtp_3; + std::vector> packets; + packets.push_back(BuildRtpPacket(kSsrc3)); + return packets; }); - packet_router_.GeneratePadding(kPaddingBytes); - packet_router_.RemoveSendRtpModule(&rtp_3); + for (int i = 0; i < 2; ++i) { + last_send_module = nullptr; + packet_router_.GeneratePadding(kPaddingBytes); + EXPECT_NE(last_send_module, nullptr); + packet_router_.RemoveSendRtpModule(last_send_module); + } } TEST_F(PacketRouterTest, AllocatesTransportSequenceNumbers) { @@ -272,12 +278,11 @@ TEST_F(PacketRouterTest, SendTransportFeedback) { } TEST_F(PacketRouterTest, SendPacketWithoutTransportSequenceNumbers) { - NiceMock rtp_1; - packet_router_.AddSendRtpModule(&rtp_1, false); - const uint16_t kSsrc1 = 1234; + NiceMock rtp_1; ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); + packet_router_.AddSendRtpModule(&rtp_1, false); // Send a packet without TransportSequenceNumber extension registered, // packets sent should not have the extension set. @@ -300,15 +305,15 @@ TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) { NiceMock rtp_1; NiceMock rtp_2; - packet_router_.AddSendRtpModule(&rtp_1, false); - packet_router_.AddSendRtpModule(&rtp_2, false); - const uint16_t kSsrc1 = 1234; const uint16_t kSsrc2 = 2345; ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); + packet_router_.AddSendRtpModule(&rtp_1, false); + packet_router_.AddSendRtpModule(&rtp_2, false); + // Transport sequence numbers start at 1, for historical reasons. uint16_t transport_sequence_number = 1; @@ -327,9 +332,6 @@ TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) { packet = BuildRtpPacket(kSsrc2); EXPECT_TRUE(packet->ReserveExtension()); - // There will be a failed attempt to send on kSsrc1 before trying - // the correct RTP module. - EXPECT_CALL(rtp_1, TrySendPacket).WillOnce(Return(false)); EXPECT_CALL( rtp_2, TrySendPacket( diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 7682b4a628..b877045d81 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -226,6 +226,9 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // a combination of values of the enumerator RtxMode. virtual int RtxSendStatus() const = 0; + // Returns the SSRC used for RTX if set, otherwise a nullopt. + virtual absl::optional RtxSsrc() const = 0; + // Sets the payload type to use when sending RTX packets. Note that this // doesn't enable RTX, only the payload type is set. virtual void SetRtxSendPayloadType(int payload_type, diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 5b81fe18b2..332f243608 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -68,6 +68,7 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(SetCSRCStatus, int32_t(bool include)); MOCK_METHOD1(SetRtxSendStatus, void(int modes)); MOCK_CONST_METHOD0(RtxSendStatus, int()); + MOCK_CONST_METHOD0(RtxSsrc, absl::optional()); MOCK_METHOD1(SetRtxSsrc, void(uint32_t)); MOCK_METHOD2(SetRtxSendPayloadType, void(int, int)); MOCK_CONST_METHOD0(FlexfecSsrc, absl::optional()); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 4ff584e27f..c7cbf5095b 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -180,6 +180,10 @@ void ModuleRtpRtcpImpl::SetRtxSendPayloadType(int payload_type, rtp_sender_->SetRtxPayloadType(payload_type, associated_payload_type); } +absl::optional ModuleRtpRtcpImpl::RtxSsrc() const { + return rtp_sender_ ? rtp_sender_->RtxSsrc() : absl::nullopt; +} + absl::optional ModuleRtpRtcpImpl::FlexfecSsrc() const { if (rtp_sender_) return rtp_sender_->FlexfecSsrc(); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 2d6cfff489..03dd81cd47 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -106,6 +106,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { void SetRtxSendStatus(int mode) override; int RtxSendStatus() const override; + absl::optional RtxSsrc() const override; void SetRtxSendPayloadType(int payload_type, int associated_payload_type) override; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 28512b81ad..50ece5421d 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -114,10 +114,7 @@ class RTPSender { // RTX. void SetRtxStatus(int mode); int RtxStatus() const; - uint32_t RtxSsrc() const { - RTC_DCHECK(rtx_ssrc_); - return *rtx_ssrc_; - } + absl::optional RtxSsrc() const { return rtx_ssrc_; } void SetRtxPayloadType(int payload_type, int associated_payload_type); diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index d769bfe9e4..9e968214ec 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -1572,7 +1572,7 @@ TEST_F(VideoSendStreamTest, PaddingIsPrimarilyRetransmissions) { VideoEncoderConfig* encoder_config) override { // Turn on RTX. send_config->rtp.rtx.payload_type = kFakeVideoSendPayloadType; - send_config->rtp.rtx.ssrcs.push_back(kVideoSendSsrcs[0]); + send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]); } void PerformTest() override {