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..8d43a8d9a3ee9d8f0adade9dd3e3e88750adb5b5 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -18,33 +18,64 @@
 
 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);
+}
+
+void RemoveModule(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->remove(rtp_module);
+}
+
+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) {
-  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);
+void PacketRouter::AddRtpModule(RtpRtcp* rtp_module, bool sender) {
+  if (sender) {
+    rtc::CritScope cs(&send_modules_crit_);
+    AddModule(rtp_module, &send_rtp_modules_);
+  } else {
+    rtc::CritScope cs(&recv_modules_crit_);
+    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);
+void PacketRouter::RemoveRtpModule(RtpRtcp* rtp_module, bool sender) {
+  if (sender) {
+    rtc::CritScope cs(&send_modules_crit_);
+    RemoveModule(rtp_module, &send_rtp_modules_);
+  } else {
+    rtc::CritScope cs(&recv_modules_crit_);
+    RemoveModule(rtp_module, &recv_rtp_modules_);
+  }
 }
 
 bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
                                     uint16_t sequence_number,
                                     int64_t capture_timestamp,
                                     bool retransmission) {
-  rtc::CritScope cs(&modules_lock_);
-  for (auto* rtp_module : rtp_modules_) {
+  rtc::CritScope cs(&send_modules_crit_);
+  for (auto* rtp_module : send_rtp_modules_) {
     if (rtp_module->SendingMedia() && ssrc == rtp_module->SSRC()) {

[the sun, 2016/01/28 15:33:45]

Can we rely on the RtpSender::sending_media_ flag to determine whether an
RtpRtcpModule is a sender? The flag is default inited to 'true' currently,
perhaps it should be false, and sending enabled only later.

Sorry to bring this up, but keeping the two lists of modules in the PacketRouter
seems pretty clumsy to me, both in terms of implementation and the PacketRouter
interface.

[stefan-webrtc, 2016/01/28 15:52:00]

I'm not sure that is possible, as modules are added to the packet router before
they have been set as sending media, at least for video. For audio the rtp
modules are always set as sending media, probably because we aren't aware of
which modules are senders or not in channel.cc.

We can likely improve on this as you suggest, but I'd prefer keeping that
separate to not complicate this CL.

[the sun, 2016/01/29 10:07:07]

I thought it would actually make the CL simpler - we could remove the two lists
and basically keep the PacketRouter as before.

ViE disables SendingMedia when the modules are created:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

It seems VoE::Channel should do as ViE and enable SendingMedia in StartSend().

[stefan-webrtc, 2016/01/29 10:59:55]

I think we still should use two lists to reduce contention between send and
receive.

I really think we're better off trying to improve this in a future CL even
though it may mean having to change the API of this class. For instance we have
the possible issue of what would happen if we enable the module for sending
media before it actually has been added to the packet router. We may end up not
routing the first packets and thereby silently dropping them.
Yes, that is possible. Out of scope for this CL though I think.

[the sun, 2016/01/29 12:29:43]

Did you ever figure out how many threads are involved here and the amount of
contention there really is?

To me it looks like the SendFeedback() is called from
RemoteEstimatorProxy::Process() and TimeToSendPacket()/TimeToSendPadding() are
called from PacedSender::Process(), so the competition would then be with the
threads adding/removing the modules.

For audio that would be the worker thread creating/deleting send/recv streams,
and for video in response to VideoSendStream::ReconfigureVideoEncoder(), which
happens on either the worker thread or the capture thread, but only if video
dimensions change.

It thus seems like contention is pretty rare here?
Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
straightforward to simplify.

[stefan-webrtc, 2016/01/29 13:36:33]

Possibly, but both TimeToSendPacket() and SendFeedback() call out from webrtc to
the transport. This should in most cases be fairly cheap assuming we have
non-blocking sockets, but I'm not sure we should make that assumption here? I
don't feel strongly in either direction, both have pros and cons. The way it is
now we are at least fairly sure we don't cause perf issues.

Do you prefer the other way?

[the sun, 2016/01/29 15:38:14]

Sorry, I may have missed something, are you saying we have multiple process
threads? AFAICT TimeToSendPacket(), SendFeedback() and TimeToSendPadding() are
all called from the Process() method of either the PacedSender or
RemoteEstimatorProxy. I was assuming we only have one process thread, meaning
then there would be no competition between those calls.

Or are you concerned about the delay sending a packet on the network may cause
for adding/reconfiguring streams?
I prefer the simple and efficient way, whichever that is. Help me see.

[stefan-webrtc, 2016/01/29 16:09:07]

Yes there are two, one for sending packets and (called from pacer) and one for
general processing, e.g. producing feedback.

It's probably not unreasonable to think feedback may be moved to the pacer
thread at some point though, but I'm not entirely sure about that.

Given this, and the fact that both will anyway be contending for the same socket
most of the time, I think we may be OK with a single lock after all. Let me know
if you agree and I'll revert some of the changes here.

[the sun, 2016/01/29 17:15:11]

Ah, now I understand!
Regardless of the socket, there's a common choke in the MediaChannel base class:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

So between feedback/media packets, having two locks in PacketRouter doesn't
accomplish much. In the case where audio/video go to different sockets, some
contention may be introduced because of a single lock in PacketRouter. I think
it'd be nice if we could keep a single list of modules here and make sure
SendingMedia() is set up correctly for voice channels. That'd be closer to
solving the real problem. But I understand the original reasoning now so I leave
it to you to decide.

       return rtp_module->TimeToSendPacket(ssrc, sequence_number,
                                           capture_timestamp, retransmission);
@@ -55,8 +86,8 @@ 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_) {
+  rtc::CritScope cs(&send_modules_crit_);
+  for (RtpRtcp* module : send_rtp_modules_) {
     if (module->SendingMedia()) {
       size_t bytes_sent =
           module->TimeToSendPadding(bytes_to_send - total_bytes_sent);
@@ -91,10 +122,14 @@ 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))
+  {
+    rtc::CritScope cs(&recv_modules_crit_);
+    if (::webrtc::SendFeedback(packet, &recv_rtp_modules_))
+      return true;
+  }
+  {
+    rtc::CritScope cs(&send_modules_crit_);
+    if (::webrtc::SendFeedback(packet, &send_rtp_modules_))
       return true;
   }
   return false;