Let PacketRouter separate send and receive modules.

webrtc/modules/pacing/packet_router.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/pacing/packet_router.h"
 
 #include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 
 namespace webrtc {
 
 PacketRouter::PacketRouter() : transport_seq_(0) {
   pacer_thread_checker_.DetachFromThread();
 }
 
 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,
                                     uint16_t sequence_number,
                                     int64_t capture_timestamp,
                                     bool retransmission,
                                     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()) {
       return rtp_module->TimeToSendPacket(ssrc, sequence_number,
                                           capture_timestamp, retransmission,
                                           pacing_info);
     }
   }
   return true;
 }
 
 size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send,
                                        const PacedPacketInfo& pacing_info) {
   RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread());
   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);
       total_bytes_sent += bytes_sent;
       if (total_bytes_sent >= bytes_to_send)
         break;
     }
   }
   return total_bytes_sent;
 }
@@ skipping 14 matching lines @@
     // operation was successful - otherwise we need to retry. Saving the
     // return value saves us a load on retry.
     prev_seq = rtc::AtomicOps::CompareAndSwap(&transport_seq_, desired_prev_seq,
                                               new_seq);
   } while (prev_seq != desired_prev_seq);
 
   return new_seq;
 }
 
 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;
   }
   return false;
 }
 
 }  // namespace webrtc