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..69007e2d24133270327b755a06d4cac34ffa28c8 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -23,27 +23,45 @@ 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_.erase(std::remove(rtp_receive_modules_.begin(),

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

This doesn't look right. Did you mean std::find? std::remove will already have
removed all matching elements in the vector, and IIUC we only expect one to be
there. Why then erase from that point to the end of the vector?

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

My understanding is that std::remove moves elements around, but doesn't change
the size of the container. See https://en.wikipedia.org/wiki/Erase–remove_idiom.

Do you know any better and more concise way to remove a single element by value
from a vector?

[the sun, 2017/03/31 10:59:57]

Ah, now I remember! Thanks. (I don't think I'll ever *really* make friends with
STL...)
no, but doesn't std::find() seem more to-the-point here? ...and it would avoid
the one extra copy.

[nisse-webrtc, 2017/03/31 11:14:08]

Since we already have the find call above (in the DCHECK), I guess it makes
sense to use its return value.

Copying will be the same, but at least we should examine the elements preceding
the element to be deleted only once.

+                                         rtp_receive_modules_.end(),
+                                         rtp_module),
+                             rtp_receive_modules_.end());
 }
 
 bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
@@ -53,7 +71,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 +89,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);
@@ -106,8 +124,15 @@ uint16_t PacketRouter::AllocateSequenceNumber() {
 }
 
 bool PacketRouter::SendFeedback(rtcp::TransportFeedback* packet) {
+  RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread());
   rtc::CritScope cs(&modules_crit_);
-  for (auto* rtp_module : rtp_modules_) {
+  // Prefer send modules.
+  for (auto* rtp_module : rtp_send_modules_) {
+    packet->SetSenderSsrc(rtp_module->SSRC());
+    if (rtp_module->SendFeedbackPacket(*packet))
+      return true;
+  }
+  for (auto* rtp_module : rtp_receive_modules_) {
     packet->SetSenderSsrc(rtp_module->SSRC());
     if (rtp_module->SendFeedbackPacket(*packet))
       return true;