From fbe84ef80fe8f3095809c16b094132b5e46d8d9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 17 Oct 2019 11:17:06 +0000 Subject: [PATCH] Revert "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 reverts commit 96f3de094566f32d842be6dd0906f1d13b8c8825. Reason for revert: Downstream test is borked. 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} TBR=danilchap@webrtc.org,sprang@webrtc.org,srte@webrtc.org Change-Id: I31330fd68ab809ff3951573791e9a79b81599958 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:11036 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157281 Reviewed-by: Erik Språng Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29511} --- 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, 129 insertions(+), 121 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 8edfd1fe28..56922b73a4 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -44,8 +44,7 @@ PacketRouter::PacketRouter(uint16_t start_transport_seq) transport_seq_(start_transport_seq) {} PacketRouter::~PacketRouter() { - RTC_DCHECK(send_modules_map_.empty()); - RTC_DCHECK(send_modules_list_.empty()); + RTC_DCHECK(rtp_send_modules_.empty()); RTC_DCHECK(rtcp_feedback_senders_.empty()); RTC_DCHECK(sender_remb_candidates_.empty()); RTC_DCHECK(receiver_remb_candidates_.empty()); @@ -54,17 +53,14 @@ PacketRouter::~PacketRouter() { void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) { rtc::CritScope cs(&modules_crit_); - - 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); - } - + 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 if (rtp_module->SupportsRtxPayloadPadding()) { - last_send_module_ = rtp_module; + rtp_send_modules_.push_front(rtp_module); + } else { + rtp_send_modules_.push_back(rtp_module); } if (remb_candidate) { @@ -72,32 +68,14 @@ 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); - - 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); - } - + 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); if (last_send_module_ == rtp_module) { last_send_module_ = nullptr; } @@ -127,6 +105,25 @@ 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_); @@ -136,27 +133,26 @@ void PacketRouter::SendPacket(std::unique_ptr packet, packet->SetExtension(AllocateSequenceNumber()); } - 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; + 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); } - 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; + // Slow path, find the correct send module. + for (auto* rtp_module : rtp_send_modules_) { + if (TrySendPacket(packet.get(), cluster_info, rtp_module)) { + return; + } } - 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; - } + RTC_LOG(LS_WARNING) << "Failed to send packet, matching RTP module not found " + "or transport error. SSRC = " + << packet->Ssrc() << ", sequence number " + << packet->SequenceNumber(); } std::vector> PacketRouter::GeneratePadding( @@ -168,26 +164,25 @@ 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()) { - padding_packets = last_send_module_->GeneratePadding(target_size_bytes); - if (!padding_packets.empty()) { + 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; + } return padding_packets; } } - 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; + return {}; } void PacketRouter::SetTransportWideSequenceNumber(uint16_t sequence_number) { @@ -281,7 +276,7 @@ bool PacketRouter::SendCombinedRtcpPacket( rtc::CritScope cs(&modules_crit_); // Prefer send modules. - for (RtpRtcp* rtp_module : send_modules_list_) { + for (auto* rtp_module : rtp_send_modules_) { if (rtp_module->RTCP() == RtcpMode::kOff) { continue; } @@ -357,4 +352,23 @@ 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 1359e04332..3680bce3d9 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -17,7 +17,6 @@ #include #include #include -#include #include #include "api/transport/network_types.h" @@ -85,6 +84,9 @@ 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_); @@ -93,17 +95,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_); - void AddSendRtpModuleToMap(RtpRtcp* rtp_module, uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); - void RemoveSendRtpModuleFromMap(uint32_t ssrc) + bool TrySendPacket(RtpPacketToSend* packet, + const PacedPacketInfo& cluster_info, + RtpRtcp* rtp_module) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); rtc::CriticalSection 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_); + // 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_); // 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 0c95e7fa76..1239201a6c 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -179,10 +179,8 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { // and supports rtx. EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([&](size_t target_size_bytes) { - std::vector> packets; - packets.push_back(BuildRtpPacket(kSsrc2)); - return packets; + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); }); packet_router_.GeneratePadding(kPaddingBytes); @@ -191,45 +189,41 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([&](size_t target_size_bytes) { - std::vector> packets; - packets.push_back(BuildRtpPacket(kSsrc1)); - return packets; + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); }); packet_router_.GeneratePadding(kPaddingBytes); // Send media on second module. Padding should be sent there. packet_router_.SendPacket(BuildRtpPacket(kSsrc2), PacedPacketInfo()); - // 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); + EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) + .Times(1) + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); - // Send on and then remove all remaining modules. - RtpRtcp* last_send_module; + // Remove second module, padding should now fall back to first module. + packet_router_.RemoveSendRtpModule(&rtp_2); EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce([&](size_t target_size_bytes) { - last_send_module = &rtp_1; - std::vector> packets; - packets.push_back(BuildRtpPacket(kSsrc1)); - return packets; + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); }); + 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) { - last_send_module = &rtp_3; - std::vector> packets; - packets.push_back(BuildRtpPacket(kSsrc3)); - return packets; + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); }); + packet_router_.GeneratePadding(kPaddingBytes); - 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); - } + packet_router_.RemoveSendRtpModule(&rtp_3); } TEST_F(PacketRouterTest, AllocatesTransportSequenceNumbers) { @@ -278,11 +272,12 @@ TEST_F(PacketRouterTest, SendTransportFeedback) { } TEST_F(PacketRouterTest, SendPacketWithoutTransportSequenceNumbers) { - const uint16_t kSsrc1 = 1234; NiceMock rtp_1; + packet_router_.AddSendRtpModule(&rtp_1, false); + + const uint16_t kSsrc1 = 1234; 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. @@ -305,15 +300,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; @@ -332,6 +327,9 @@ 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 b877045d81..7682b4a628 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -226,9 +226,6 @@ 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 332f243608..5b81fe18b2 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -68,7 +68,6 @@ 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 c7cbf5095b..4ff584e27f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -180,10 +180,6 @@ 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 03dd81cd47..2d6cfff489 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -106,7 +106,6 @@ 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 50ece5421d..28512b81ad 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -114,7 +114,10 @@ class RTPSender { // RTX. void SetRtxStatus(int mode); int RtxStatus() const; - absl::optional RtxSsrc() const { return rtx_ssrc_; } + uint32_t RtxSsrc() const { + RTC_DCHECK(rtx_ssrc_); + 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 9e968214ec..d769bfe9e4 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(kSendRtxSsrcs[0]); + send_config->rtp.rtx.ssrcs.push_back(kVideoSendSsrcs[0]); } void PerformTest() override {