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

webrtc/modules/pacing/packet_router.h

 /*
  *  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.
  */
 
@@ skipping 31 matching lines @@
   PacketRouter();
   ~PacketRouter() override;
 
   // TODO(nisse): Delete, as soon as downstream app is updated.
   RTC_DEPRECATED void AddRtpModule(RtpRtcp* rtp_module) {
     AddReceiveRtpModule(rtp_module);
   }
   RTC_DEPRECATED void RemoveRtpModule(RtpRtcp* rtp_module) {
     RemoveReceiveRtpModule(rtp_module);
   }
-  void AddSendRtpModule(RtpRtcp* rtp_module);
+
+  void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate);
   void RemoveSendRtpModule(RtpRtcp* rtp_module);
+  RTC_DEPRECATED void AddSendRtpModule(RtpRtcp* rtp_module) {
+    constexpr bool remb_candidate = true;  // Default to old behavior.

[danilchap, 2017/07/28 11:35:04]

May be remove this line:
with called function just one line above, repeating parameter name with local
variable is probably overkill.

Comment feels like stating the obvious: to be backward compatible you have to
default to old behavior.

[eladalon, 2017/07/28 15:47:11]

Done.

+    AddSendRtpModule(rtp_module, remb_candidate);
+  }
 
-  void AddReceiveRtpModule(RtpRtcp* rtp_module);
+  void AddReceiveRtpModule(RtpRtcp* rtp_module, bool remb_candidate);
   void RemoveReceiveRtpModule(RtpRtcp* rtp_module);
+  RTC_DEPRECATED void AddReceiveRtpModule(RtpRtcp* rtp_module) {
+    constexpr bool remb_candidate = true;  // Default to old behavior.
+    AddReceiveRtpModule(rtp_module, remb_candidate);
+  }
 
   // Implements PacedSender::Callback.
   bool TimeToSendPacket(uint32_t ssrc,
                         uint16_t sequence_number,
                         int64_t capture_timestamp,
                         bool retransmission,
                         const PacedPacketInfo& packet_info) override;
 
   size_t TimeToSendPadding(size_t bytes,
                            const PacedPacketInfo& packet_info) override;
@@ skipping 10 matching lines @@
                                uint32_t bitrate_bps) override;
 
   // Send REMB feedback.
   virtual bool SendRemb(uint32_t bitrate_bps,
                         const std::vector<uint32_t>& ssrcs);
 
   // Send transport feedback packet to send-side.
   virtual bool SendTransportFeedback(rtcp::TransportFeedback* packet);
 
  private:
+  void AddRembModuleCandidate(RtpRtcp* candidate_module, bool sender)
+      EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
+  void MaybeRemoveRembModuleCandidate(RtpRtcp* candidate_module, bool sender)
+      EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
+  void DetermineActiveRembModule() EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
+
   rtc::RaceChecker pacer_race_;
   rtc::CriticalSection modules_crit_;
   std::list<RtpRtcp*> rtp_send_modules_ GUARDED_BY(modules_crit_);
   std::vector<RtpRtcp*> rtp_receive_modules_ GUARDED_BY(modules_crit_);
 
+  // TODO(eladalon): remb_crit_ only ever held from one function, and it's not

[danilchap, 2017/07/28 11:35:04]

you addressing it in another CL already, so probably remove the TODO from this
one.

[eladalon, 2017/07/28 15:47:11]

1. I think you're referring to receive_cs_? I don't remember handling remb_crit_
(though I might be mistaken).
2. Speaking of receive_cs_, I am not sure when (/if) that CL can be landed,
since it's causing problems with some unit-tests which are multi-threaded. So I
think I should fix the TODO (% fixing my typo over "caleld").

+  // clear if that function can actually be caleld from more than one thread.
   rtc::CriticalSection remb_crit_;
   // The last time a REMB was sent.
   int64_t last_remb_time_ms_ GUARDED_BY(remb_crit_);
   uint32_t last_send_bitrate_bps_ GUARDED_BY(remb_crit_);
   // The last bitrate update.
   uint32_t bitrate_bps_ GUARDED_BY(remb_crit_);
 
+  // Candidates for the REMB module can be RTP sender/receiver modules, with
+  // the sender modules taking precedence.
+  std::vector<RtpRtcp*> sender_remb_candidates_ GUARDED_BY(modules_crit_);
+  std::vector<RtpRtcp*> receiver_remb_candidates_ GUARDED_BY(modules_crit_);
+  RtpRtcp* active_remb_module_ GUARDED_BY(modules_crit_);
+
   volatile int transport_seq_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter);
 };
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_PACING_PACKET_ROUTER_H_