Let PacketRouter separate send and receive modules.

webrtc/modules/pacing/packet_router.cc

Index: webrtc/modules/pacing/packet_router.cc
diff --git a/webrtc/modules/pacing/packet_router.cc b/webrtc/modules/pacing/packet_router.cc
index fcb1e4976c5cad62eb202f07ec8b20ea9a7b7899..7437e9a1f7850a204e1ac6d736b735734b71c760 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -23,27 +23,42 @@ PacketRouter::PacketRouter() : transport_seq_(0) {
 }
 
 PacketRouter::~PacketRouter() {
-  RTC_DCHECK(rtp_modules_.empty());
+  RTC_DCHECK(rtp_send_modules_.empty());
+  RTC_DCHECK(rtp_receive_modules_.empty());
 }
 
-void PacketRouter::AddRtpModule(RtpRtcp* rtp_module) {
+void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module) {
   rtc::CritScope cs(&modules_crit_);
-  RTC_DCHECK(std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module) ==
-             rtp_modules_.end());
+  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->RtxSendStatus() & kRtxRedundantPayloads) > 0) {
-    rtp_modules_.push_front(rtp_module);
+    rtp_send_modules_.push_front(rtp_module);
   } else {
-    rtp_modules_.push_back(rtp_module);
+    rtp_send_modules_.push_back(rtp_module);
   }
 }
 
-void PacketRouter::RemoveRtpModule(RtpRtcp* rtp_module) {
+void PacketRouter::RemoveSendRtpModule(RtpRtcp* rtp_module) {
   rtc::CritScope cs(&modules_crit_);
-  RTC_DCHECK(std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module) !=
-             rtp_modules_.end());
-  rtp_modules_.remove(rtp_module);
+  RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(),
+                       rtp_module) != rtp_send_modules_.end());
+  rtp_send_modules_.remove(rtp_module);
+}
+
+void PacketRouter::AddReceiveRtpModule(RtpRtcp* rtp_module) {
+  rtc::CritScope cs(&modules_crit_);
+  RTC_DCHECK(std::find(rtp_receive_modules_.begin(), rtp_receive_modules_.end(),
+                       rtp_module) == rtp_receive_modules_.end());
+  rtp_receive_modules_.push_back(rtp_module);
+}
+
+void PacketRouter::RemoveReceiveRtpModule(RtpRtcp* rtp_module) {
+  rtc::CritScope cs(&modules_crit_);
+  RTC_DCHECK(std::find(rtp_receive_modules_.begin(), rtp_receive_modules_.end(),
+                       rtp_module) != rtp_receive_modules_.end());
+  rtp_receive_modules_.remove(rtp_module);
 }
 
 bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
@@ -53,7 +68,7 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
                                     const PacedPacketInfo& pacing_info) {
   RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread());
   rtc::CritScope cs(&modules_crit_);
-  for (auto* rtp_module : rtp_modules_) {
+  for (auto* rtp_module : rtp_send_modules_) {
     if (!rtp_module->SendingMedia())
       continue;
     if (ssrc == rtp_module->SSRC() || ssrc == rtp_module->FlexfecSsrc()) {
@@ -71,7 +86,7 @@ size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send,
   size_t total_bytes_sent = 0;
   rtc::CritScope cs(&modules_crit_);
   // Rtp modules are ordered by which stream can most benefit from padding.
-  for (RtpRtcp* module : rtp_modules_) {
+  for (RtpRtcp* module : rtp_send_modules_) {
     if (module->SendingMedia() && module->HasBweExtensions()) {
       size_t bytes_sent = module->TimeToSendPadding(
           bytes_to_send - total_bytes_sent, pacing_info);
@@ -107,7 +122,7 @@ uint16_t PacketRouter::AllocateSequenceNumber() {
 
 bool PacketRouter::SendFeedback(rtcp::TransportFeedback* packet) {
   rtc::CritScope cs(&modules_crit_);

[the sun, 2017/03/28 12:21:52]

From which thread is this driven, vs TimeToSendPacket()/TimeToSendPadding()?
Will there be contention on this lock?

[nisse-webrtc, 2017/03/29 13:58:36]

Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|, and
SendFeedback is |Call::module_process_thread_|.

[the sun, 2017/03/31 07:17:13]

Could you add the missing thread checkers to document this?

[nisse-webrtc, 2017/03/31 08:30:54]

After a second look, I think SendFeedback is actually called by the pacer
thread. In which case the lock shouldn't be necessary. I'll test that out in a
separate cl.

[the sun, 2017/03/31 10:38:48]

Nice!

[nisse-webrtc, 2017/03/31 10:51:53]

Unfortunately, the lock is still needed, for synchronization with the methods
modifying the lists. The other cl is https://codereview.webrtc.org/2787283003/,
it just adds a thread check, and that change is incorporated in this cl too.

But at least there shouldn't be any contention once all streams are up and
running.

-  for (auto* rtp_module : rtp_modules_) {
+  for (auto* rtp_module : rtp_receive_modules_) {
     packet->SetSenderSsrc(rtp_module->SSRC());
     if (rtp_module->SendFeedbackPacket(*packet))
       return true;