Revert "Use just a lookup map of RTP modules in PacketRouter"
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 <sprang@webrtc.org>
> Reviewed-by: Sebastian Jansson <srte@webrtc.org>
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> 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 <sprang@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29511}
diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc
index 8edfd1f..56922b7 100644
--- a/modules/pacing/packet_router.cc
+++ b/modules/pacing/packet_router.cc
@@ -44,8 +44,7 @@
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 @@
void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) {
rtc::CritScope cs(&modules_crit_);
-
- AddSendRtpModuleToMap(rtp_module, rtp_module->SSRC());
- if (absl::optional<uint32_t> rtx_ssrc = rtp_module->RtxSsrc()) {
- AddSendRtpModuleToMap(rtp_module, *rtx_ssrc);
- }
- if (absl::optional<uint32_t> 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::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<RtpRtcp*, std::list<RtpRtcp*>::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<uint32_t> rtx_ssrc = rtp_module->RtxSsrc()) {
- RemoveSendRtpModuleFromMap(*rtx_ssrc);
- }
- if (absl::optional<uint32_t> 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 @@
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<RtpPacketToSend> packet,
const PacedPacketInfo& cluster_info) {
rtc::CritScope cs(&modules_crit_);
@@ -136,27 +133,26 @@
packet->SetExtension<TransportSequenceNumber>(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<std::unique_ptr<RtpPacketToSend>> PacketRouter::GeneratePadding(
@@ -168,26 +164,25 @@
// 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<std::unique_ptr<RtpPacketToSend>> 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 @@
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 @@
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 1359e04..3680bce 100644
--- a/modules/pacing/packet_router.h
+++ b/modules/pacing/packet_router.h
@@ -17,7 +17,6 @@
#include <list>
#include <memory>
#include <unordered_map>
-#include <utility>
#include <vector>
#include "api/transport/network_types.h"
@@ -85,6 +84,9 @@
std::vector<std::unique_ptr<rtcp::RtcpPacket>> 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 @@
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<uint32_t,
- std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>>
- send_modules_map_ RTC_GUARDED_BY(modules_crit_);
- std::list<RtpRtcp*> send_modules_list_ RTC_GUARDED_BY(modules_crit_);
+ // Rtp and Rtcp modules of the rtp senders.
+ std::list<RtpRtcp*> rtp_send_modules_ RTC_GUARDED_BY(modules_crit_);
+ // Ssrc to RtpRtcp module cache.
+ std::unordered_map<uint32_t, RtpRtcp*> 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 0c95e7f..1239201 100644
--- a/modules/pacing/packet_router_unittest.cc
+++ b/modules/pacing/packet_router_unittest.cc
@@ -179,10 +179,8 @@
// and supports rtx.
EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes))
.Times(1)
- .WillOnce([&](size_t target_size_bytes) {
- std::vector<std::unique_ptr<RtpPacketToSend>> packets;
- packets.push_back(BuildRtpPacket(kSsrc2));
- return packets;
+ .WillOnce([](size_t target_size_bytes) {
+ return std::vector<std::unique_ptr<RtpPacketToSend>>();
});
packet_router_.GeneratePadding(kPaddingBytes);
@@ -191,45 +189,41 @@
EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes))
.Times(1)
- .WillOnce([&](size_t target_size_bytes) {
- std::vector<std::unique_ptr<RtpPacketToSend>> packets;
- packets.push_back(BuildRtpPacket(kSsrc1));
- return packets;
+ .WillOnce([](size_t target_size_bytes) {
+ return std::vector<std::unique_ptr<RtpPacketToSend>>();
});
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<std::unique_ptr<RtpPacketToSend>>();
+ });
+ 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<std::unique_ptr<RtpPacketToSend>> packets;
- packets.push_back(BuildRtpPacket(kSsrc1));
- return packets;
+ .WillOnce([](size_t target_size_bytes) {
+ return std::vector<std::unique_ptr<RtpPacketToSend>>();
});
+ 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<std::unique_ptr<RtpPacketToSend>> packets;
- packets.push_back(BuildRtpPacket(kSsrc3));
- return packets;
+ .WillOnce([](size_t target_size_bytes) {
+ return std::vector<std::unique_ptr<RtpPacketToSend>>();
});
+ 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, SendPacketWithoutTransportSequenceNumbers) {
- const uint16_t kSsrc1 = 1234;
NiceMock<MockRtpRtcp> 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 @@
NiceMock<MockRtpRtcp> rtp_1;
NiceMock<MockRtpRtcp> 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 @@
packet = BuildRtpPacket(kSsrc2);
EXPECT_TRUE(packet->ReserveExtension<TransportSequenceNumber>());
+ // 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 b877045..7682b4a 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -226,9 +226,6 @@
// 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<uint32_t> 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 332f243..5b81fe1 100644
--- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
+++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
@@ -68,7 +68,6 @@
MOCK_METHOD1(SetCSRCStatus, int32_t(bool include));
MOCK_METHOD1(SetRtxSendStatus, void(int modes));
MOCK_CONST_METHOD0(RtxSendStatus, int());
- MOCK_CONST_METHOD0(RtxSsrc, absl::optional<uint32_t>());
MOCK_METHOD1(SetRtxSsrc, void(uint32_t));
MOCK_METHOD2(SetRtxSendPayloadType, void(int, int));
MOCK_CONST_METHOD0(FlexfecSsrc, absl::optional<uint32_t>());
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index c7cbf50..4ff584e 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -180,10 +180,6 @@
rtp_sender_->SetRtxPayloadType(payload_type, associated_payload_type);
}
-absl::optional<uint32_t> ModuleRtpRtcpImpl::RtxSsrc() const {
- return rtp_sender_ ? rtp_sender_->RtxSsrc() : absl::nullopt;
-}
-
absl::optional<uint32_t> 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 03dd81c..2d6cfff 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -106,7 +106,6 @@
void SetRtxSendStatus(int mode) override;
int RtxSendStatus() const override;
- absl::optional<uint32_t> 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 50ece54..28512b8 100644
--- a/modules/rtp_rtcp/source/rtp_sender.h
+++ b/modules/rtp_rtcp/source/rtp_sender.h
@@ -114,7 +114,10 @@
// RTX.
void SetRtxStatus(int mode);
int RtxStatus() const;
- absl::optional<uint32_t> 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 9e96821..d769bfe 100644
--- a/video/video_send_stream_tests.cc
+++ b/video/video_send_stream_tests.cc
@@ -1572,7 +1572,7 @@
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 {
