From 599df8523382e0687f09dcd966d71f7cf9c9d058 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 25 Sep 2017 15:19:35 +0200 Subject: [PATCH] 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 Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#19947} --- modules/pacing/packet_router.h | 5 +++-- modules/remote_bitrate_estimator/BUILD.gn | 9 +++------ .../include/remote_bitrate_estimator.h | 9 +++++++++ .../remote_bitrate_estimator/overuse_detector.cc | 1 - .../remote_bitrate_estimator_abs_send_time.cc | 1 - .../remote_estimator_proxy.cc | 13 ++++++------- .../remote_estimator_proxy.h | 5 +++-- .../remote_estimator_proxy_unittest.cc | 4 ++-- 8 files changed, 26 insertions(+), 21 deletions(-) diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index ae328283f3..da8c177276 100644 --- a/modules/pacing/packet_router.h +++ b/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& 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) diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn index d29969afd2..4784a835f4 100644 --- a/modules/remote_bitrate_estimator/BUILD.gn +++ b/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", ] diff --git a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h index 31227e0dcd..99f4ab8250 100644 --- a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h +++ b/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 {}; diff --git a/modules/remote_bitrate_estimator/overuse_detector.cc b/modules/remote_bitrate_estimator/overuse_detector.cc index 004214579c..55c7b58ec9 100644 --- a/modules/remote_bitrate_estimator/overuse_detector.cc +++ b/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" diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index c521e53724..ce8924d038 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -14,7 +14,6 @@ #include -#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" diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 0c459b39b3..0e6caff41f 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -13,8 +13,6 @@ #include #include -#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::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; } diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h index 7a69177a08..985ddd3eaf 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/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_; diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index 4f1d9e7b13..4197fcc727 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -50,7 +50,7 @@ std::vector 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 router_; + testing::StrictMock router_; RemoteEstimatorProxy proxy_; };