Resolve cyclic dependency in remote bitrate estimator

Access SendTransportFeedback function through new interface to break rbe -> pacing -> rbe cycle
Depend on rtp_rtcp_format source set to break rbe -> rtp_rtcp -> rbe cycle.

Bug: webrtc:6828
Change-Id: Iae1c463a71871c0055485e2eca9b2235d770afec
Reviewed-on: https://webrtc-review.googlesource.com/1620
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#19947}

modules/pacing/packet_router.h

@@ -37,7 +37,8 @@ class TransportFeedback;
 // receive modules.
 class PacketRouter : public PacedSender::PacketSender,
                      public TransportSequenceNumberAllocator,
-                     public RemoteBitrateObserver {
+                     public RemoteBitrateObserver,
+                     public TransportFeedbackSenderInterface {
  public:
   PacketRouter();
   ~PacketRouter() override;
@@ -92,7 +93,7 @@ class PacketRouter : public PacedSender::PacketSender,
                         const std::vector<uint32_t>& ssrcs);
 
   // Send transport feedback packet to send-side.
-  virtual bool SendTransportFeedback(rtcp::TransportFeedback* packet);
+  bool SendTransportFeedback(rtcp::TransportFeedback* packet) override;
 
  private:
   void AddRembModuleCandidate(RtpRtcp* candidate_module, bool sender)

modules/remote_bitrate_estimator/BUILD.gn

@@ -9,11 +9,6 @@
 import("../../webrtc.gni")
 
 rtc_static_library("remote_bitrate_estimator") {
-  # TODO(mbonadei): Remove (bugs.webrtc.org/6828)
-  # Errors on cyclic dependency with:
-  # rtp_rtcp:rtp_rtcp if enabled.
-  check_includes = false
-
   sources = [
     "aimd_rate_control.cc",
     "aimd_rate_control.h",
@@ -51,7 +46,9 @@ rtc_static_library("remote_bitrate_estimator") {
 
   deps = [
     "../..:webrtc_common",
-    "../../rtc_base:rtc_base",
+    "../../api:optional",
+    "../../modules:module_api",
+    "../../modules/rtp_rtcp:rtp_rtcp_format",
     "../../rtc_base:rtc_base_approved",
     "../../system_wrappers",
   ]

modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h

@@ -23,6 +23,9 @@
 #include "typedefs.h"  // NOLINT(build/include)
 
 namespace webrtc {
+namespace rtcp {
+class TransportFeedback;
+}  // namespace rtcp
 
 class Clock;
 
@@ -38,6 +41,12 @@ class RemoteBitrateObserver {
   virtual ~RemoteBitrateObserver() {}
 };
 
+class TransportFeedbackSenderInterface {
+ public:
+  virtual ~TransportFeedbackSenderInterface() = default;
+  virtual bool SendTransportFeedback(rtcp::TransportFeedback* packet) = 0;
+};
+
 // TODO(holmer): Remove when all implementations have been updated.
 struct ReceiveBandwidthEstimatorStats {};
 

modules/remote_bitrate_estimator/overuse_detector.cc

@@ -19,7 +19,6 @@
 
 #include "modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h"
-#include "modules/rtp_rtcp/source/rtp_utility.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
 #include "rtc_base/safe_minmax.h"

modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc

@@ -14,7 +14,6 @@
 
 #include <algorithm>
 
-#include "modules/pacing/paced_sender.h"
 #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/constructormagic.h"

modules/remote_bitrate_estimator/remote_estimator_proxy.cc

@@ -13,8 +13,6 @@
 #include <limits>
 #include <algorithm>
 
-#include "modules/pacing/packet_router.h"
-#include "modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
@@ -34,10 +32,11 @@ const int RemoteEstimatorProxy::kDefaultSendIntervalMs = 100;
 static constexpr int64_t kMaxTimeMs =
     std::numeric_limits<int64_t>::max() / 1000;
 
-RemoteEstimatorProxy::RemoteEstimatorProxy(const Clock* clock,
-                                           PacketRouter* packet_router)
+RemoteEstimatorProxy::RemoteEstimatorProxy(
+    const Clock* clock,
+    TransportFeedbackSenderInterface* feedback_sender)
     : clock_(clock),
-      packet_router_(packet_router),
+      feedback_sender_(feedback_sender),
       last_process_time_ms_(-1),
       media_ssrc_(0),
       feedback_sequence_(0),
@@ -83,8 +82,8 @@ void RemoteEstimatorProxy::Process() {
   while (more_to_build) {
     rtcp::TransportFeedback feedback_packet;
     if (BuildFeedbackPacket(&feedback_packet)) {
-      RTC_DCHECK(packet_router_ != nullptr);
-      packet_router_->SendTransportFeedback(&feedback_packet);
+      RTC_DCHECK(feedback_sender_ != nullptr);
+      feedback_sender_->SendTransportFeedback(&feedback_packet);
     } else {
       more_to_build = false;
     }

modules/remote_bitrate_estimator/remote_estimator_proxy.h

@@ -32,7 +32,8 @@ class TransportFeedback;
 
 class RemoteEstimatorProxy : public RemoteBitrateEstimator {
  public:
-  RemoteEstimatorProxy(const Clock* clock, PacketRouter* packet_router);
+  RemoteEstimatorProxy(const Clock* clock,
+                       TransportFeedbackSenderInterface* feedback_sender);
   virtual ~RemoteEstimatorProxy();
 
   void IncomingPacket(int64_t arrival_time_ms,
@@ -58,7 +59,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
   bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packet);
 
   const Clock* const clock_;
-  PacketRouter* const packet_router_;
+  TransportFeedbackSenderInterface* const feedback_sender_;
   int64_t last_process_time_ms_;
 
   rtc::CriticalSection lock_;

modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc

@@ -50,7 +50,7 @@ std::vector<int64_t> TimestampsMs(
   return timestamps;
 }
 
-class MockPacketRouter : public PacketRouter {
+class MockTransportFeedbackSender : public TransportFeedbackSenderInterface {
  public:
   MOCK_METHOD1(SendTransportFeedback,
                bool(rtcp::TransportFeedback* feedback_packet));
@@ -76,7 +76,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test {
   }
 
   SimulatedClock clock_;
-  testing::StrictMock<MockPacketRouter> router_;
+  testing::StrictMock<MockTransportFeedbackSender> router_;
   RemoteEstimatorProxy proxy_;
 };