Use separate rtp module lists for send and receive in PacketRouter.

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 5fd350834a5e1ec6231966d0e7e207f79b5e5b99..1e69f0713ccd0aa24fe8b9b577e4bdedd38e05d0 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -18,25 +18,56 @@
 
 namespace webrtc {
 
+namespace {
+void AddModule(RtpRtcp* rtp_module, std::list<RtpRtcp*>* rtp_modules) {
+  RTC_DCHECK(std::find(rtp_modules->begin(), rtp_modules->end(), rtp_module) ==
+             rtp_modules->end());
+  rtp_modules->push_back(rtp_module);
+}
+
+bool RemoveModule(RtpRtcp* rtp_module, std::list<RtpRtcp*>* rtp_modules) {
+  auto it = std::find(rtp_modules->begin(), rtp_modules->end(), rtp_module);
+  if (it == rtp_modules->end())
+    return false;
+  rtp_modules->erase(it);

[tommi, 2016/01/23 17:23:26]

if we didn't have to search the receiver list when removing a sender and vice
versa, then we wouldn't need to care if an item exists or not when removing it. 
We could just use std::list<>::remove(), but since there's no "bool sender"
argument to RemoveRtpModule, this is what we end up with.  What about adding a
bool flag to RemoveRtpModule too?

[stefan-webrtc, 2016/01/23 17:46:03]

Right, that would be an option, and I can definitely add a bool sender to the
remove method. The code before this CL also didn't do remove() though for the
reason that we want to be able to DCHECK that no modules are removed which
haven't also been added. Given this, do you still think it's worth doing as it
slightly complicates the API?

[tommi, 2016/01/24 10:30:08]

We can use std::find() inside an RTC_DCHECK to check if the pointer is in the
list.
If we don't want to add a bool to the Remove method, we could do this:

* Make AddRtpModule return a PacketRouter::ModuleRegistration type. (not a
pointer)
* The caller owns this registration instance and can call Unregister() on it if
explicit unregistration is needed, otherwise it will happen automatically in the
dtor.
* ModuleRegistration is a movable (implement ctor w/&&) but not copyable inner
class that has a 'bool sender_' variable and a pointer to the PacketRouter.
* Make PacketRouter::RemoveRtpModule private and only allow ModuleRegistration
to call it.
* In the dtor of ModuleRegistration, it calls
packet_router_->RemoveRtpModule(module_, sender_);

[stefan-webrtc, 2016/01/25 11:54:46]

I think I'll just add a bool sender to RemoveRtpModule, should be easy enough
and avoids making the class complicated.

+  return true;
+}
+
+bool SendFeedback(rtcp::TransportFeedback* packet,
+                  std::list<RtpRtcp*>* rtp_modules) {
+  for (auto* rtp_module : *rtp_modules) {
+    packet->WithPacketSenderSsrc(rtp_module->SSRC());
+    if (rtp_module->SendFeedbackPacket(*packet))
+      return true;
+  }
+  return false;
+}
+}
+
 PacketRouter::PacketRouter() : transport_seq_(0) {
 }
 
 PacketRouter::~PacketRouter() {
-  RTC_DCHECK(rtp_modules_.empty());
+  RTC_DCHECK(send_rtp_modules_.empty());
+  RTC_DCHECK(recv_rtp_modules_.empty());
 }
 
-void PacketRouter::AddRtpModule(RtpRtcp* rtp_module) {
+void PacketRouter::AddRtpModule(RtpRtcp* rtp_module, bool sender) {
   rtc::CritScope cs(&modules_lock_);
-  RTC_DCHECK(std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module) ==
-             rtp_modules_.end());
-  rtp_modules_.push_back(rtp_module);
+
+  if (sender) {

[tommi, 2016/01/23 17:23:26]

is it good to lock both sender and receiver maps with the same lock?  I don't
suppose we can be smarter about when we lock and know about the threads that are
in the picture?

[stefan-webrtc, 2016/01/23 17:46:03]

We could have different locks, but I don't think it would buy us any performance
since what this class does _should_ be very light weight.

I'll give the thread model some thought, but I think at least two threads are
involved here as video engine adds and removes modules in SetSendCodec which
will be called on the encoder thread in the near future.

[tommi, 2016/01/24 10:30:08]

I think that this particular class is very light but it holds the lock while
calling out into other implementations.  Unregistering a receiver might have to
wait for a sender completing some operation that lies outside this class.  I
looked around for what's being done in some of the methods that we're calling
out to and it looks like there are many different implementations and hard for
me to say for sure that from a black box perspective calling the PacketRouter
methods from outside, is always going to be light weight, meaning low/no
contention.

[stefan-webrtc, 2016/01/25 11:54:46]

Agree it's not very clear how expensive it is to send packets. I'll split into
two crit sects as you suggest.

+    AddModule(rtp_module, &send_rtp_modules_);
+  } else {
+    AddModule(rtp_module, &recv_rtp_modules_);
+  }
 }
 
 void PacketRouter::RemoveRtpModule(RtpRtcp* rtp_module) {
   rtc::CritScope cs(&modules_lock_);
-  auto it = std::find(rtp_modules_.begin(), rtp_modules_.end(), rtp_module);
-  RTC_DCHECK(it != rtp_modules_.end());
-  rtp_modules_.erase(it);
+  bool removed = RemoveModule(rtp_module, &send_rtp_modules_);
+  if (!removed)
+    removed = RemoveModule(rtp_module, &recv_rtp_modules_);
+  RTC_DCHECK(removed);
 }
 
 bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
@@ -44,7 +75,7 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
                                     int64_t capture_timestamp,
                                     bool retransmission) {
   rtc::CritScope cs(&modules_lock_);
-  for (auto* rtp_module : rtp_modules_) {
+  for (auto* rtp_module : send_rtp_modules_) {
     if (rtp_module->SendingMedia() && ssrc == rtp_module->SSRC()) {
       return rtp_module->TimeToSendPacket(ssrc, sequence_number,
                                           capture_timestamp, retransmission);
@@ -56,7 +87,7 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
 size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send) {
   size_t total_bytes_sent = 0;
   rtc::CritScope cs(&modules_lock_);
-  for (RtpRtcp* module : rtp_modules_) {
+  for (RtpRtcp* module : send_rtp_modules_) {
     if (module->SendingMedia()) {
       size_t bytes_sent =
           module->TimeToSendPadding(bytes_to_send - total_bytes_sent);
@@ -92,12 +123,9 @@ uint16_t PacketRouter::AllocateSequenceNumber() {
 
 bool PacketRouter::SendFeedback(rtcp::TransportFeedback* packet) {
   rtc::CritScope cs(&modules_lock_);
-  for (auto* rtp_module : rtp_modules_) {
-    packet->WithPacketSenderSsrc(rtp_module->SSRC());
-    if (rtp_module->SendFeedbackPacket(*packet))
-      return true;
-  }
-  return false;
+  if (!::webrtc::SendFeedback(packet, &recv_rtp_modules_))
+    return ::webrtc::SendFeedback(packet, &send_rtp_modules_);
+  return true;

[tommi, 2016/01/23 17:23:25]

nit:

return webrtc::SendFeedback(packet, &recv_rtp_modules_) ||
       webrtc::SendFeedback(packet, &send_rtp_modules_);

it feels to me like the scope of the lock operations is very wide.  We
frequently hold the locks while calling out into other modules.

[stefan-webrtc, 2016/01/23 17:46:04]

Agree, I think it would be better to find the module to send on and then release
the lock, which I think could work.

[tommi, 2016/01/24 10:30:08]

No chance that it could be removed while the call is ongoing?  If we can make
such assumptions, I think we could reduce the lock scope in many methods.

[stefan-webrtc, 2016/01/25 11:54:46]

I thought I could guarantee that, but unfortunately I can't.

 }
 
 }  // namespace webrtc