Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates

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 1cee90a44153137f3ee61b68325ca41aadfaf0c1..298a173ba52c7494adcc0cd049a8de80acf898d0 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -22,73 +22,63 @@ namespace webrtc {
 PacketRouter::PacketRouter()
     : last_remb_time_ms_(rtc::TimeMillis()),
       last_send_bitrate_bps_(0),
+      active_remb_module_(nullptr),
       transport_seq_(0) {}
 
 PacketRouter::~PacketRouter() {
   RTC_DCHECK(rtp_send_modules_.empty());
   RTC_DCHECK(rtp_receive_modules_.empty());
+  RTC_DCHECK(sender_remb_candidates_.empty());
+  RTC_DCHECK(receiver_remb_candidates_.empty());
+  RTC_DCHECK(active_remb_module_ == nullptr);
 }
 
-void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module) {
+void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) {
   rtc::CritScope cs(&modules_crit_);
   RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(),
                        rtp_module) == rtp_send_modules_.end());
-  if (rtp_send_modules_.empty() && !rtp_receive_modules_.empty()) {
-    rtp_receive_modules_.front()->SetREMBStatus(false);
-  }
-
   // 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) {
-    if (!rtp_send_modules_.empty()) {
-      rtp_send_modules_.front()->SetREMBStatus(false);
-    }
     rtp_send_modules_.push_front(rtp_module);
-    rtp_module->SetREMBStatus(true);
   } else {
-    if (rtp_send_modules_.empty()) {
-      rtp_module->SetREMBStatus(true);
-    }
-
     rtp_send_modules_.push_back(rtp_module);
   }
+
+  if (remb_candidate) {
+    AddRembModuleCandidate(rtp_module, true);
+  }
 }
 
 void PacketRouter::RemoveSendRtpModule(RtpRtcp* rtp_module) {
   rtc::CritScope cs(&modules_crit_);
-  RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(),
-                       rtp_module) != rtp_send_modules_.end());
+  MaybeRemoveRembModuleCandidate(rtp_module, /* sender = */ true);
+  const auto& it =

[danilchap, 2017/07/31 13:18:03]

auto it = 
(iterators normally passed by value)

[eladalon, 2017/07/31 14:08:26]

Done.

+      std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module);
+  RTC_DCHECK(it != rtp_send_modules_.end());
   rtp_send_modules_.remove(rtp_module);

[danilchap, 2017/07/31 13:18:04]

may be erase(it) to avoid double lookup

[eladalon, 2017/07/31 14:08:26]

Done.

-  rtp_module->SetREMBStatus(false);
-  if (!rtp_send_modules_.empty()) {
-    rtp_send_modules_.front()->SetREMBStatus(true);
-  } else if (!rtp_receive_modules_.empty()) {
-    rtp_receive_modules_.front()->SetREMBStatus(true);
-  }
 }
 
-void PacketRouter::AddReceiveRtpModule(RtpRtcp* rtp_module) {
+void PacketRouter::AddReceiveRtpModule(RtpRtcp* rtp_module,
+                                       bool remb_candidate) {
   rtc::CritScope cs(&modules_crit_);
   RTC_DCHECK(std::find(rtp_receive_modules_.begin(), rtp_receive_modules_.end(),
                        rtp_module) == rtp_receive_modules_.end());
-  if (rtp_send_modules_.empty() && rtp_receive_modules_.empty()) {
-    rtp_module->SetREMBStatus(true);
-  }
+
   rtp_receive_modules_.push_back(rtp_module);
+
+  if (remb_candidate) {
+    AddRembModuleCandidate(rtp_module, false);
+  }
 }
 
 void PacketRouter::RemoveReceiveRtpModule(RtpRtcp* rtp_module) {
   rtc::CritScope cs(&modules_crit_);
+  MaybeRemoveRembModuleCandidate(rtp_module, /* sender = */ false);
   const auto& it = std::find(rtp_receive_modules_.begin(),
                              rtp_receive_modules_.end(), rtp_module);
   RTC_DCHECK(it != rtp_receive_modules_.end());
   rtp_receive_modules_.erase(it);
-  if (rtp_send_modules_.empty()) {
-    rtp_module->SetREMBStatus(false);
-    if (!rtp_receive_modules_.empty()) {
-      rtp_receive_modules_.front()->SetREMBStatus(true);
-    }
-  }
 }
 
 bool PacketRouter::TimeToSendPacket(uint32_t ssrc,
@@ -190,18 +180,17 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
 bool PacketRouter::SendRemb(uint32_t bitrate_bps,
                             const std::vector<uint32_t>& ssrcs) {
   rtc::CritScope lock(&modules_crit_);
-  RtpRtcp* remb_module;
-  if (!rtp_send_modules_.empty())
-    remb_module = rtp_send_modules_.front();
-  else if (!rtp_receive_modules_.empty())
-    remb_module = rtp_receive_modules_.front();
-  else
+
+  if (!active_remb_module_) {
     return false;
+  }
+
   // The Add* and Remove* methods above ensure that this (and only this) module
   // has REMB enabled. REMB should be disabled on all other modules, because
   // otherwise, they will send REMB with stale info.
-  RTC_DCHECK(remb_module->REMB());
-  remb_module->SetREMBData(bitrate_bps, ssrcs);
+  RTC_DCHECK(active_remb_module_->REMB());
+  active_remb_module_->SetREMBData(bitrate_bps, ssrcs);
+
   return true;
 }
 
@@ -222,4 +211,66 @@ bool PacketRouter::SendTransportFeedback(rtcp::TransportFeedback* packet) {
   return false;
 }
 
+void PacketRouter::AddRembModuleCandidate(RtpRtcp* candidate_module,
+                                          bool sender) {
+  RTC_DCHECK(candidate_module);
+  std::vector<RtpRtcp*>& candidates_vector =

[danilchap, 2017/07/31 13:18:04]

may be just candidates
(function is short enough to see it is a vector)

[eladalon, 2017/07/31 14:08:26]

Done.

+      sender ? sender_remb_candidates_ : receiver_remb_candidates_;
+  RTC_DCHECK(std::find(candidates_vector.cbegin(), candidates_vector.cend(),
+                       candidate_module) == candidates_vector.cend());
+  candidates_vector.push_back(candidate_module);
+  DetermineActiveRembModule();
+}
+
+void PacketRouter::MaybeRemoveRembModuleCandidate(RtpRtcp* candidate_module,
+                                                  bool sender) {
+  RTC_DCHECK(candidate_module);
+  std::vector<RtpRtcp*>& candidates_vector =
+      sender ? sender_remb_candidates_ : receiver_remb_candidates_;
+  auto it = std::find(candidates_vector.begin(), candidates_vector.end(),
+                      candidate_module);
+
+  if (it == candidates_vector.end()) {
+    return;  // Function called due to removal of non-REMB-candidate module.
+  }
+
+  if (*it == active_remb_module_) {

[danilchap, 2017/07/31 13:18:04]

is this block needed? (DetermineActiveRembModule seems already deactivate the
active module)

[eladalon, 2017/07/31 14:08:26]

You're right that it would work, but I find it to be a bit confusing to only
have:
  candidates.erase(it);
  DetermineActiveRembModule();
Because that splits the removal between two functions. With this (somewhat
redundant) block, though, removal is done completely within
MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only sets the
new candidate (if any), as its name suggests.

[danilchap, 2017/07/31 14:37:17]

yep, removal (and addition) are naturally split between two functions:
Add/Remove are responsible to update candidates_ containers
where DetermineActiveRembModule responsible for updating active_remb_module_.

It is easy to imaging that the way you turn remb off in the module might change.
Currently you have to remember to update 2 places. With this block remove there
will be single block that is responsible for deactivating module.

[eladalon, 2017/07/31 14:51:20]

That's one side of the coin. The other side is that by removing this block, I
rely the update of |candidates| to be sufficient to trigger the right behavior
in DetermineActiveRembModule().

Let's try to have the best of both worlds by adding a helper function,
UnsetActiveRembModule(). Wdyt of that? (See in code.)

[danilchap, 2017/07/31 15:10:05]

Looking at implmentation of the DetermineActiveRembModule, this guarantee seems
easy to provide: active_remb_module_ shouldn't be deleted. Everything else is
responsibility of that last helper.
I find it about same, but that my taste you do not need to agree with.

counter-proposal: Will it look better if you rename DetermingActiveRembModule
into e.g. UpdateActiveRembModule?
(or some better name that hints it responsible both for unsetting old value and
configuring new one)

[eladalon, 2017/07/31 15:22:32]

My reservation against Update vs. Determine is that it makes it less clear that
the following two are also an option:
1. The active has not changed (e.g. when adding a new candidate which is not of
higher priority than the current).
2. Last candidate removed - "update" (weakly) suggests that there would be an
active module after the function terminates.

I prefer to keep the removal here. I'm interpreting your latest comment as "this
is my opinion, but I don't mind you landing this with your version." I'll land,
then. Let me know if I misunderstood, and we can fix this.

+    RTC_DCHECK(active_remb_module_->REMB());
+    active_remb_module_->SetREMBStatus(false);
+    active_remb_module_ = nullptr;
+  }
+
+  candidates_vector.erase(it);
+
+  DetermineActiveRembModule();
+}
+
+void PacketRouter::DetermineActiveRembModule() {
+  // Sender modules take precedence over receiver modules, because SRs (sender
+  // reports) are sent more frequently than RR (receiver reports).
+  // When adding the first sender module, we should change the active REMB
+  // module to be that. Otherwise, we remain with the current active module.
+
+  RtpRtcp* previous_active_remb_module = active_remb_module_;
+
+  if (!sender_remb_candidates_.empty()) {
+    active_remb_module_ = sender_remb_candidates_.front();
+  } else if (!receiver_remb_candidates_.empty()) {
+    active_remb_module_ = receiver_remb_candidates_.front();
+  } else {
+    active_remb_module_ = nullptr;
+  }
+
+  if (active_remb_module_ != previous_active_remb_module) {
+    if (previous_active_remb_module) {
+      RTC_DCHECK(previous_active_remb_module->REMB());
+      previous_active_remb_module->SetREMBStatus(false);
+    }
+    if (active_remb_module_) {
+      RTC_DCHECK(!active_remb_module_->REMB());
+      active_remb_module_->SetREMBStatus(true);
+    }
+  }
+}
+
 }  // namespace webrtc