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}

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(rtp_send_modules_.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());
@@ -54,17 +53,14 @@ 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(),
-  AddSendRtpModuleToMap(rtp_module, rtp_module->SSRC());
+                       rtp_module) == rtp_send_modules_.end());
-  if (absl::optional<uint32_t> rtx_ssrc = rtp_module->RtxSsrc()) {
+  // Put modules which can use regular payload packets (over rtx) instead of
-    AddSendRtpModuleToMap(rtp_module, *rtx_ssrc);
+  // padding first as it's less of a waste
-  }
-  if (absl::optional<uint32_t> flexfec_ssrc = rtp_module->FlexfecSsrc()) {
-    AddSendRtpModuleToMap(rtp_module, *flexfec_ssrc);
-  }
-
   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<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);
-
+  auto it =
-  RemoveSendRtpModuleFromMap(rtp_module->SSRC());
+      std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module);
-  if (absl::optional<uint32_t> rtx_ssrc = rtp_module->RtxSsrc()) {
+  RTC_DCHECK(it != rtp_send_modules_.end());
-    RemoveSendRtpModuleFromMap(*rtx_ssrc);
+  rtp_send_modules_.erase(it);
-  }
-  if (absl::optional<uint32_t> flexfec_ssrc = rtp_module->FlexfecSsrc()) {
-    RemoveSendRtpModuleFromMap(*flexfec_ssrc);
-  }
-
   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<RtpPacketToSend> packet,
                               const PacedPacketInfo& cluster_info) {
   rtc::CritScope cs(&modules_crit_);
@@ -136,27 +133,26 @@ void PacketRouter::SendPacket(std::unique_ptr<RtpPacketToSend> packet,
     packet->SetExtension<TransportSequenceNumber>(AllocateSequenceNumber());
   }
 
-  uint32_t ssrc = packet->Ssrc();
+  auto it = rtp_module_cache_map_.find(packet->Ssrc());
-  auto kv = send_modules_map_.find(ssrc);
+  if (it != rtp_module_cache_map_.end()) {
-  if (kv == send_modules_map_.end()) {
+    if (TrySendPacket(packet.get(), cluster_info, it->second)) {
-    RTC_LOG(LS_WARNING)
+      return;
-        << "Failed to send packet, matching RTP module not found "
+    }
+    // Entry is stale, remove it.
+    rtp_module_cache_map_.erase(it);
+  }
+
+  // Slow path, find the correct send module.
+  for (auto* rtp_module : rtp_send_modules_) {
+    if (TrySendPacket(packet.get(), cluster_info, 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();
+                      << packet->Ssrc() << ", sequence number "
-    return;
+                      << packet->SequenceNumber();
-  }
-
-  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;
-  }
-
-  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<std::unique_ptr<RtpPacketToSend>> PacketRouter::GeneratePadding(
@@ -168,26 +164,25 @@ std::vector<std::unique_ptr<RtpPacketToSend>> 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<std::unique_ptr<RtpPacketToSend>> padding_packets;
   if (last_send_module_ != nullptr &&
       last_send_module_->SupportsRtxPayloadPadding()) {
-    padding_packets = last_send_module_->GeneratePadding(target_size_bytes);
+    RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(),
-    if (!padding_packets.empty()) {
+                         last_send_module_) != rtp_send_modules_.end());
-      return padding_packets;
+    return last_send_module_->GeneratePadding(target_size_bytes);
-    }
   }
 
-  for (RtpRtcp* rtp_module : send_modules_list_) {
+  // Rtp modules are ordered by which stream can most benefit from padding.
+  for (RtpRtcp* rtp_module : rtp_send_modules_) {
     if (rtp_module->SupportsPadding()) {
-      padding_packets = rtp_module->GeneratePadding(target_size_bytes);
+      auto padding_packets = rtp_module->GeneratePadding(target_size_bytes);
       if (!padding_packets.empty()) {
         last_send_module_ = rtp_module;
-        break;
       }
+      return padding_packets;
     }
   }
 
-  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

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 @@ class PacketRouter : public RemoteBitrateObserver,
       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 @@ 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)
+  bool TrySendPacket(RtpPacketToSend* packet,
-      RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
+                     const PacedPacketInfo& cluster_info,
-  void RemoveSendRtpModuleFromMap(uint32_t ssrc)
+                     RtpRtcp* rtp_module)
       RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
 
   rtc::CriticalSection modules_crit_;
-  // Ssrc to RtpRtcp module and iterator into |send_modules_list_|;
+  // Rtp and Rtcp modules of the rtp senders.
-  std::unordered_map<uint32_t,
+  std::list<RtpRtcp*> rtp_send_modules_ RTC_GUARDED_BY(modules_crit_);
-                     std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>>
+  // Ssrc to RtpRtcp module cache.
-      send_modules_map_ RTC_GUARDED_BY(modules_crit_);
+  std::unordered_map<uint32_t, RtpRtcp*> rtp_module_cache_map_
-  std::list<RtpRtcp*> send_modules_list_ RTC_GUARDED_BY(modules_crit_);
+      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.

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) {
+      .WillOnce([](size_t target_size_bytes) {
-        std::vector<std::unique_ptr<RtpPacketToSend>> packets;
+        return std::vector<std::unique_ptr<RtpPacketToSend>>();
-        packets.push_back(BuildRtpPacket(kSsrc2));
-        return packets;
       });
   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) {
+      .WillOnce([](size_t target_size_bytes) {
-        std::vector<std::unique_ptr<RtpPacketToSend>> packets;
+        return std::vector<std::unique_ptr<RtpPacketToSend>>();
-        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());
 
-  // If the last active module is removed, and no module sends media before
+  EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes))
-  // the next padding request, and arbitrary module will be selected.
+      .Times(1)
-  packet_router_.RemoveSendRtpModule(&rtp_2);
+      .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.
+  // Remove second module, padding should now fall back to first module.
-  RtpRtcp* last_send_module;
+  packet_router_.RemoveSendRtpModule(&rtp_2);
   EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes))
       .Times(1)
-      .WillOnce([&](size_t target_size_bytes) {
+      .WillOnce([](size_t target_size_bytes) {
-        last_send_module = &rtp_1;
+        return std::vector<std::unique_ptr<RtpPacketToSend>>();
-        std::vector<std::unique_ptr<RtpPacketToSend>> 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) {
+      .WillOnce([](size_t target_size_bytes) {
-        last_send_module = &rtp_3;
+        return std::vector<std::unique_ptr<RtpPacketToSend>>();
-        std::vector<std::unique_ptr<RtpPacketToSend>> packets;
-        packets.push_back(BuildRtpPacket(kSsrc3));
-        return packets;
       });
-
-  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<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 @@ TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) {
   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 @@ TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) {
   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(

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<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,

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<uint32_t>());
   MOCK_METHOD1(SetRtxSsrc, void(uint32_t));
   MOCK_METHOD2(SetRtxSendPayloadType, void(int, int));
   MOCK_CONST_METHOD0(FlexfecSsrc, absl::optional<uint32_t>());

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<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();

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<uint32_t> RtxSsrc() const override;
 
   void SetRtxSendPayloadType(int payload_type,
                              int associated_payload_type) override;

modules/rtp_rtcp/source/rtp_sender.h

@@ -114,7 +114,10 @@ class RTPSender {
   // 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);
 

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 {