diff --git a/webrtc/modules/BUILD.gn b/webrtc/modules/BUILD.gn index b3bfbac46f..ccc681c02f 100644 --- a/webrtc/modules/BUILD.gn +++ b/webrtc/modules/BUILD.gn @@ -375,7 +375,6 @@ if (rtc_include_tests) { "congestion_controller/delay_based_bwe_unittest_helper.h", "congestion_controller/probe_bitrate_estimator_unittest.cc", "congestion_controller/probe_controller_unittest.cc", - "congestion_controller/transport_feedback_adapter_unittest.cc", "media_file/media_file_unittest.cc", "module_common_types_unittest.cc", "pacing/bitrate_prober_unittest.cc", @@ -395,6 +394,7 @@ if (rtc_include_tests) { "remote_bitrate_estimator/test/bwe_unittest.cc", "remote_bitrate_estimator/test/estimators/nada_unittest.cc", "remote_bitrate_estimator/test/metric_recorder_unittest.cc", + "remote_bitrate_estimator/transport_feedback_adapter_unittest.cc", "rtp_rtcp/source/byte_io_unittest.cc", "rtp_rtcp/source/fec_receiver_unittest.cc", "rtp_rtcp/source/fec_test_helper.cc", diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc index b3363679bd..bc2f1f63e6 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc @@ -186,18 +186,22 @@ void BitrateControllerImpl::OnReceiverEstimatedBitrate(uint32_t bitrate) { MaybeTriggerOnNetworkChanged(); } -void BitrateControllerImpl::OnDelayBasedBweResult( - const DelayBasedBwe::Result& result) { - if (!result.updated) - return; +void BitrateControllerImpl::OnProbeBitrate(uint32_t bitrate_bps) { { rtc::CritScope cs(&critsect_); - if (result.probe) { - bandwidth_estimation_.SetSendBitrate(result.target_bitrate_bps); - } else { - bandwidth_estimation_.UpdateDelayBasedEstimate( - clock_->TimeInMilliseconds(), result.target_bitrate_bps); - } + bandwidth_estimation_.SetSendBitrate(bitrate_bps); + } + MaybeTriggerOnNetworkChanged(); +} + +// TODO(isheriff): Perhaps need new interface for invocation from DelayBasedBwe. +void BitrateControllerImpl::OnReceiveBitrateChanged( + const std::vector& ssrcs, + uint32_t bitrate_bps) { + { + rtc::CritScope cs(&critsect_); + bandwidth_estimation_.UpdateDelayBasedEstimate(clock_->TimeInMilliseconds(), + bitrate_bps); } MaybeTriggerOnNetworkChanged(); } diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h index 7ee6b19380..c8bb10282b 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h @@ -61,7 +61,10 @@ class BitrateControllerImpl : public BitrateController { uint8_t* fraction_loss, int64_t* rtt) override; - void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) override; + // RemoteBitrateObserver overrides. + void OnReceiveBitrateChanged(const std::vector& ssrcs, + uint32_t bitrate_bps) override; + void OnProbeBitrate(uint32_t bitrate_bps) override; int64_t TimeUntilNextProcess() override; void Process() override; diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc index 4b298cc152..7da947b7b2 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc @@ -169,8 +169,7 @@ TEST_F(BitrateControllerTest, OneBitrateObserverOneRtcpObserver) { EXPECT_EQ(300000, bitrate_observer_.last_bitrate_); // Test that a low delay-based estimate limits the combined estimate. - webrtc::DelayBasedBwe::Result result(false, 280000); - controller_->OnDelayBasedBweResult(result); + controller_->OnReceiveBitrateChanged({0}, 280000); EXPECT_EQ(280000, bitrate_observer_.last_bitrate_); // Test that a low REMB limits the combined estimate. diff --git a/webrtc/modules/bitrate_controller/include/bitrate_controller.h b/webrtc/modules/bitrate_controller/include/bitrate_controller.h index 7400d7d032..90b6471390 100644 --- a/webrtc/modules/bitrate_controller/include/bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/bitrate_controller.h @@ -17,9 +17,9 @@ #include -#include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/include/module.h" #include "webrtc/modules/pacing/paced_sender.h" +#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -44,7 +44,7 @@ class BitrateObserver { virtual ~BitrateObserver() {} }; -class BitrateController : public Module { +class BitrateController : public Module, public RemoteBitrateObserver { // This class collects feedback from all streams sent to a peer (via // RTCPBandwidthObservers). It does one aggregated send side bandwidth // estimation and divide the available bitrate between all its registered @@ -78,8 +78,6 @@ class BitrateController : public Module { int min_bitrate_bps, int max_bitrate_bps) = 0; - virtual void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) = 0; - // Gets the available payload bandwidth in bits per second. Note that // this bandwidth excludes packet headers. virtual bool AvailableBandwidth(uint32_t* bandwidth) const = 0; diff --git a/webrtc/modules/congestion_controller/BUILD.gn b/webrtc/modules/congestion_controller/BUILD.gn index 1069d624ff..758fba59d8 100644 --- a/webrtc/modules/congestion_controller/BUILD.gn +++ b/webrtc/modules/congestion_controller/BUILD.gn @@ -18,8 +18,6 @@ rtc_static_library("congestion_controller") { "probe_bitrate_estimator.h", "probe_controller.cc", "probe_controller.h", - "transport_feedback_adapter.cc", - "transport_feedback_adapter.h", ] # TODO(jschuh): Bug 1348: fix this warning. @@ -34,5 +32,6 @@ rtc_static_library("congestion_controller") { deps = [ "../bitrate_controller", "../pacing", + "../remote_bitrate_estimator", ] } diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index f7e7e5625a..c0ec3da0f9 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -21,6 +21,7 @@ #include "webrtc/base/socket.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" +#include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/congestion_controller/probe_controller.h" #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h" #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h" @@ -170,7 +171,7 @@ CongestionController::CongestionController( retransmission_rate_limiter_( new RateLimiter(clock, kRetransmitWindowSizeMs)), remote_estimator_proxy_(clock_, packet_router_.get()), - transport_feedback_adapter_(clock_, bitrate_controller_.get()), + transport_feedback_adapter_(clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), max_bitrate_bps_(0), last_reported_bitrate_bps_(0), @@ -201,7 +202,7 @@ CongestionController::CongestionController( retransmission_rate_limiter_( new RateLimiter(clock, kRetransmitWindowSizeMs)), remote_estimator_proxy_(clock_, packet_router_.get()), - transport_feedback_adapter_(clock_, bitrate_controller_.get()), + transport_feedback_adapter_(clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), max_bitrate_bps_(0), last_reported_bitrate_bps_(0), @@ -214,8 +215,10 @@ CongestionController::CongestionController( CongestionController::~CongestionController() {} void CongestionController::Init() { - transport_feedback_adapter_.InitBwe(); - transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); + transport_feedback_adapter_.SetBitrateEstimator( + new DelayBasedBwe(bitrate_controller_.get(), clock_)); + transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( + min_bitrate_bps_); } void CongestionController::SetBweBitrates(int min_bitrate_bps, @@ -233,7 +236,8 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps, if (remote_bitrate_estimator_) remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; - transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); + transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( + min_bitrate_bps_); MaybeTriggerOnNetworkChanged(); } @@ -252,8 +256,10 @@ void CongestionController::ResetBweAndBitrates(int bitrate_bps, if (remote_bitrate_estimator_) remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); - transport_feedback_adapter_.InitBwe(); - transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps); + RemoteBitrateEstimator* rbe = + new DelayBasedBwe(bitrate_controller_.get(), clock_); + transport_feedback_adapter_.SetBitrateEstimator(rbe); + rbe->SetMinBitrate(min_bitrate_bps); // TODO(holmer): Trigger a new probe once mid-call probing is implemented. MaybeTriggerOnNetworkChanged(); } diff --git a/webrtc/modules/congestion_controller/congestion_controller.gypi b/webrtc/modules/congestion_controller/congestion_controller.gypi index 3fcddfb248..5c7ddf2784 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.gypi +++ b/webrtc/modules/congestion_controller/congestion_controller.gypi @@ -14,6 +14,7 @@ 'dependencies': [ '<(webrtc_root)/modules/modules.gyp:bitrate_controller', '<(webrtc_root)/modules/modules.gyp:paced_sender', + '<(webrtc_root)/modules/modules.gyp:remote_bitrate_estimator', ], 'sources': [ 'congestion_controller.cc', @@ -24,8 +25,6 @@ 'probe_bitrate_estimator.h', 'probe_controller.cc', 'probe_controller.h', - 'transport_feedback_adapter.cc', - 'transport_feedback_adapter.h', ], # TODO(jschuh): Bug 1348: fix size_t to int truncations. 'msvs_disabled_warnings': [ 4267, ], diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.cc b/webrtc/modules/congestion_controller/delay_based_bwe.cc index db3ae582f1..87dc502b28 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc @@ -21,6 +21,7 @@ #include "webrtc/modules/congestion_controller/include/congestion_controller.h" #include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" +#include "webrtc/system_wrappers/include/critical_section_wrapper.h" #include "webrtc/system_wrappers/include/metrics.h" #include "webrtc/typedefs.h" @@ -39,8 +40,9 @@ constexpr uint32_t kFixedSsrc = 0; namespace webrtc { -DelayBasedBwe::DelayBasedBwe(Clock* clock) +DelayBasedBwe::DelayBasedBwe(RemoteBitrateObserver* observer, Clock* clock) : clock_(clock), + observer_(observer), inter_arrival_(), estimator_(), detector_(OverUseDetectorOptions()), @@ -48,10 +50,11 @@ DelayBasedBwe::DelayBasedBwe(Clock* clock) last_update_ms_(-1), last_seen_packet_ms_(-1), uma_recorded_(false) { + RTC_DCHECK(observer_); network_thread_.DetachFromThread(); } -DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( +void DelayBasedBwe::IncomingPacketFeedbackVector( const std::vector& packet_feedback_vector) { RTC_DCHECK(network_thread_.CalledOnValidThread()); if (!uma_recorded_) { @@ -60,89 +63,87 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( BweNames::kBweNamesMax); uma_recorded_ = true; } - Result aggregated_result; for (const auto& packet_info : packet_feedback_vector) { - Result result = IncomingPacketInfo(packet_info); - if (result.updated) - aggregated_result = result; + IncomingPacketInfo(packet_info); } - return aggregated_result; } -DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo( - const PacketInfo& info) { - // printf("Acked: %ld\n", info.payload_size); +void DelayBasedBwe::IncomingPacketInfo(const PacketInfo& info) { int64_t now_ms = clock_->TimeInMilliseconds(); incoming_bitrate_.Update(info.payload_size, info.arrival_time_ms); - Result result; - // Reset if the stream has timed out. - if (last_seen_packet_ms_ == -1 || - now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { - inter_arrival_.reset(new InterArrival( - (kTimestampGroupLengthMs << kInterArrivalShift) / 1000, - kTimestampToMs, true)); - estimator_.reset(new OveruseEstimator(OverUseDetectorOptions())); - } - last_seen_packet_ms_ = now_ms; + bool delay_based_bwe_changed = false; + uint32_t target_bitrate_bps = 0; + { + rtc::CritScope lock(&crit_); - uint32_t send_time_24bits = - static_cast(((static_cast(info.send_time_ms) - << kAbsSendTimeFraction) + - 500) / - 1000) & - 0x00FFFFFF; - // Shift up send time to use the full 32 bits that inter_arrival works with, - // so wrapping works properly. - uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; - - uint32_t ts_delta = 0; - int64_t t_delta = 0; - int size_delta = 0; - if (inter_arrival_->ComputeDeltas(timestamp, info.arrival_time_ms, now_ms, - info.payload_size, &ts_delta, &t_delta, - &size_delta)) { - double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); - estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(), - info.arrival_time_ms); - detector_.Detect(estimator_->offset(), ts_delta_ms, - estimator_->num_of_deltas(), info.arrival_time_ms); - } - - int probing_bps = 0; - if (info.probe_cluster_id != PacketInfo::kNotAProbe) { - probing_bps = - probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); - } - - // Currently overusing the bandwidth. - if (detector_.State() == kBwOverusing) { - rtc::Optional incoming_rate = - incoming_bitrate_.Rate(info.arrival_time_ms); - if (incoming_rate && - remote_rate_.TimeToReduceFurther(now_ms, *incoming_rate)) { - result.updated = UpdateEstimate(info.arrival_time_ms, now_ms, - &result.target_bitrate_bps); + // Reset if the stream has timed out. + if (last_seen_packet_ms_ == -1 || + now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) { + inter_arrival_.reset(new InterArrival( + (kTimestampGroupLengthMs << kInterArrivalShift) / 1000, + kTimestampToMs, true)); + estimator_.reset(new OveruseEstimator(OverUseDetectorOptions())); } - } else if (probing_bps > 0) { - // No overuse, but probing measured a bitrate. - remote_rate_.SetEstimate(probing_bps, info.arrival_time_ms); - result.probe = true; - result.updated = UpdateEstimate(info.arrival_time_ms, now_ms, - &result.target_bitrate_bps); - } - rtc::Optional incoming_rate = - incoming_bitrate_.Rate(info.arrival_time_ms); - if (!result.updated && - (last_update_ms_ == -1 || - now_ms - last_update_ms_ > remote_rate_.GetFeedbackInterval())) { - result.updated = UpdateEstimate(info.arrival_time_ms, now_ms, - &result.target_bitrate_bps); - } - if (result.updated) - last_update_ms_ = now_ms; + last_seen_packet_ms_ = now_ms; - return result; + uint32_t send_time_24bits = + static_cast(((static_cast(info.send_time_ms) + << kAbsSendTimeFraction) + + 500) / + 1000) & + 0x00FFFFFF; + // Shift up send time to use the full 32 bits that inter_arrival works with, + // so wrapping works properly. + uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; + + uint32_t ts_delta = 0; + int64_t t_delta = 0; + int size_delta = 0; + if (inter_arrival_->ComputeDeltas(timestamp, info.arrival_time_ms, now_ms, + info.payload_size, &ts_delta, &t_delta, + &size_delta)) { + double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); + estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(), + info.arrival_time_ms); + detector_.Detect(estimator_->offset(), ts_delta_ms, + estimator_->num_of_deltas(), info.arrival_time_ms); + } + + int probing_bps = 0; + if (info.probe_cluster_id != PacketInfo::kNotAProbe) { + probing_bps = + probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); + } + + // Currently overusing the bandwidth. + if (detector_.State() == kBwOverusing) { + rtc::Optional incoming_rate = + incoming_bitrate_.Rate(info.arrival_time_ms); + if (incoming_rate && + remote_rate_.TimeToReduceFurther(now_ms, *incoming_rate)) { + delay_based_bwe_changed = + UpdateEstimate(info.arrival_time_ms, now_ms, &target_bitrate_bps); + } + } else if (probing_bps > 0) { + // No overuse, but probing measured a bitrate. + remote_rate_.SetEstimate(probing_bps, info.arrival_time_ms); + observer_->OnProbeBitrate(probing_bps); + delay_based_bwe_changed = + UpdateEstimate(info.arrival_time_ms, now_ms, &target_bitrate_bps); + } + if (!delay_based_bwe_changed && + (last_update_ms_ == -1 || + now_ms - last_update_ms_ > remote_rate_.GetFeedbackInterval())) { + delay_based_bwe_changed = + UpdateEstimate(info.arrival_time_ms, now_ms, &target_bitrate_bps); + } + } + + if (delay_based_bwe_changed) { + last_update_ms_ = now_ms; + observer_->OnReceiveBitrateChanged({kFixedSsrc}, target_bitrate_bps); + } } bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms, @@ -159,10 +160,20 @@ bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms, return remote_rate_.ValidEstimate(); } +void DelayBasedBwe::Process() {} + +int64_t DelayBasedBwe::TimeUntilNextProcess() { + const int64_t kDisabledModuleTime = 1000; + return kDisabledModuleTime; +} + void DelayBasedBwe::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { + rtc::CritScope lock(&crit_); remote_rate_.SetRtt(avg_rtt_ms); } +void DelayBasedBwe::RemoveStream(uint32_t ssrc) {} + bool DelayBasedBwe::LatestEstimate(std::vector* ssrcs, uint32_t* bitrate_bps) const { // Currently accessed from both the process thread (see @@ -171,6 +182,7 @@ bool DelayBasedBwe::LatestEstimate(std::vector* ssrcs, // thread. RTC_DCHECK(ssrcs); RTC_DCHECK(bitrate_bps); + rtc::CritScope lock(&crit_); if (!remote_rate_.ValidEstimate()) return false; @@ -182,6 +194,7 @@ bool DelayBasedBwe::LatestEstimate(std::vector* ssrcs, void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) { // Called from both the configuration thread and the network thread. Shouldn't // be called from the network thread in the future. + rtc::CritScope lock(&crit_); remote_rate_.SetMinBitrate(min_bitrate_bps); } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.h b/webrtc/modules/congestion_controller/delay_based_bwe.h index 7e2c8bdc52..3e0a0149a4 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.h +++ b/webrtc/modules/congestion_controller/delay_based_bwe.h @@ -18,6 +18,7 @@ #include "webrtc/base/checks.h" #include "webrtc/base/constructormagic.h" +#include "webrtc/base/criticalsection.h" #include "webrtc/base/rate_statistics.h" #include "webrtc/base/thread_checker.h" #include "webrtc/modules/congestion_controller/probe_bitrate_estimator.h" @@ -26,42 +27,46 @@ #include "webrtc/modules/remote_bitrate_estimator/inter_arrival.h" #include "webrtc/modules/remote_bitrate_estimator/overuse_detector.h" #include "webrtc/modules/remote_bitrate_estimator/overuse_estimator.h" +#include "webrtc/system_wrappers/include/critical_section_wrapper.h" namespace webrtc { -class DelayBasedBwe { +class DelayBasedBwe : public RemoteBitrateEstimator { public: - static const int64_t kStreamTimeOutMs = 2000; - - struct Result { - Result() : updated(false), probe(false), target_bitrate_bps(0) {} - Result(bool probe, uint32_t target_bitrate_bps) - : updated(true), probe(probe), target_bitrate_bps(target_bitrate_bps) {} - bool updated; - bool probe; - uint32_t target_bitrate_bps; - }; - - explicit DelayBasedBwe(Clock* clock); + DelayBasedBwe(RemoteBitrateObserver* observer, Clock* clock); virtual ~DelayBasedBwe() {} - Result IncomingPacketFeedbackVector( - const std::vector& packet_feedback_vector); - void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms); + void IncomingPacketFeedbackVector( + const std::vector& packet_feedback_vector) override; + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; bool LatestEstimate(std::vector* ssrcs, - uint32_t* bitrate_bps) const; - void SetMinBitrate(int min_bitrate_bps); + uint32_t* bitrate_bps) const override; + void SetMinBitrate(int min_bitrate_bps) override; + + // Required by RemoteBitrateEstimator but does nothing. + void Process() override; + // Required by RemoteBitrateEstimator but does nothing. + int64_t TimeUntilNextProcess() override; + // Required by RemoteBitrateEstimator but does nothing. + void RemoveStream(uint32_t ssrc) override; + void IncomingPacket(int64_t arrival_time_ms, + size_t payload_size, + const RTPHeader& header) override { + RTC_NOTREACHED(); + } private: - Result IncomingPacketInfo(const PacketInfo& info); + void IncomingPacketInfo(const PacketInfo& info); // Updates the current remote rate estimate and returns true if a valid // estimate exists. bool UpdateEstimate(int64_t packet_arrival_time_ms, int64_t now_ms, - uint32_t* target_bitrate_bps); + uint32_t* target_bitrate_bps) + EXCLUSIVE_LOCKS_REQUIRED(crit_); rtc::ThreadChecker network_thread_; Clock* const clock_; + RemoteBitrateObserver* const observer_; std::unique_ptr inter_arrival_; std::unique_ptr estimator_; OveruseDetector detector_; @@ -69,8 +74,10 @@ class DelayBasedBwe { int64_t last_update_ms_; int64_t last_seen_packet_ms_; bool uma_recorded_; - AimdRateControl remote_rate_; - ProbeBitrateEstimator probe_bitrate_estimator_; + + rtc::CriticalSection crit_; + AimdRateControl remote_rate_ GUARDED_BY(&crit_); + ProbeBitrateEstimator probe_bitrate_estimator_ GUARDED_BY(&crit_); RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(DelayBasedBwe); }; diff --git a/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc index e751013a5d..7220967be2 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc @@ -32,7 +32,7 @@ TEST_F(DelayBasedBweTest, ProbeDetection) { now_ms = clock_.TimeInMilliseconds(); IncomingFeedback(now_ms, now_ms, seq_num++, 1000, 0); } - EXPECT_TRUE(bitrate_observer_.updated()); + EXPECT_TRUE(bitrate_observer_->updated()); // Second burst sent at 8 * 1000 / 5 = 1600 kbps. for (int i = 0; i < kNumProbes; ++i) { @@ -41,8 +41,8 @@ TEST_F(DelayBasedBweTest, ProbeDetection) { IncomingFeedback(now_ms, now_ms, seq_num++, 1000, 1); } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_GT(bitrate_observer_.latest_bitrate(), 1500000u); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_GT(bitrate_observer_->latest_bitrate(), 1500000u); } TEST_F(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { @@ -61,8 +61,8 @@ TEST_F(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { PacketInfo::kNotAProbe); } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_GT(bitrate_observer_.latest_bitrate(), 800000u); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_GT(bitrate_observer_->latest_bitrate(), 800000u); } TEST_F(DelayBasedBweTest, ProbeDetectionFasterArrival) { @@ -78,7 +78,7 @@ TEST_F(DelayBasedBweTest, ProbeDetectionFasterArrival) { IncomingFeedback(now_ms, send_time_ms, seq_num++, 1000, 0); } - EXPECT_FALSE(bitrate_observer_.updated()); + EXPECT_FALSE(bitrate_observer_->updated()); } TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrival) { @@ -94,8 +94,8 @@ TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrival) { IncomingFeedback(now_ms, send_time_ms, seq_num++, 1000, 1); } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_NEAR(bitrate_observer_.latest_bitrate(), 1140000u, 10000u); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_NEAR(bitrate_observer_->latest_bitrate(), 1140000u, 10000u); } TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrivalHighBitrate) { @@ -111,8 +111,8 @@ TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrivalHighBitrate) { IncomingFeedback(now_ms, send_time_ms, seq_num++, 1000, 1); } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_NEAR(bitrate_observer_.latest_bitrate(), 4000000u, 10000u); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_NEAR(bitrate_observer_->latest_bitrate(), 4000000u, 10000u); } TEST_F(DelayBasedBweTest, InitialBehavior) { diff --git a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc index a3a1893cbe..34692511b9 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc @@ -150,7 +150,8 @@ int64_t StreamGenerator::GenerateFrame(std::vector* packets, DelayBasedBweTest::DelayBasedBweTest() : clock_(100000000), - bitrate_estimator_(&clock_), + bitrate_observer_(new test::TestBitrateObserver), + bitrate_estimator_(new DelayBasedBwe(bitrate_observer_.get(), &clock_)), stream_generator_(new test::StreamGenerator(1e6, // Capacity. clock_.TimeInMicroseconds())), arrival_time_offset_ms_(0) {} @@ -181,13 +182,7 @@ void DelayBasedBweTest::IncomingFeedback(int64_t arrival_time_ms, sequence_number, payload_size, probe_cluster_id); std::vector packets; packets.push_back(packet); - DelayBasedBwe::Result result = - bitrate_estimator_.IncomingPacketFeedbackVector(packets); - const uint32_t kDummySsrc = 0; - if (result.updated) { - bitrate_observer_.OnReceiveBitrateChanged({kDummySsrc}, - result.target_bitrate_bps); - } + bitrate_estimator_->IncomingPacketFeedbackVector(packets); } // Generates a frame of packets belonging to a stream at a given bitrate and @@ -206,20 +201,17 @@ bool DelayBasedBweTest::GenerateAndProcessFrame(uint32_t ssrc, return false; bool overuse = false; - bitrate_observer_.Reset(); + bitrate_observer_->Reset(); clock_.AdvanceTimeMicroseconds(1000 * packets.back().arrival_time_ms - clock_.TimeInMicroseconds()); for (auto& packet : packets) { RTC_CHECK_GE(packet.arrival_time_ms + arrival_time_offset_ms_, 0); packet.arrival_time_ms += arrival_time_offset_ms_; } - DelayBasedBwe::Result result = - bitrate_estimator_.IncomingPacketFeedbackVector(packets); - const uint32_t kDummySsrc = 0; - if (result.updated) { - bitrate_observer_.OnReceiveBitrateChanged({kDummySsrc}, - result.target_bitrate_bps); - if (result.target_bitrate_bps < bitrate_bps) + bitrate_estimator_->IncomingPacketFeedbackVector(packets); + + if (bitrate_observer_->updated()) { + if (bitrate_observer_->latest_bitrate() < bitrate_bps) overuse = true; } @@ -243,13 +235,13 @@ uint32_t DelayBasedBweTest::SteadyStateRun(uint32_t ssrc, for (int i = 0; i < max_number_of_frames; ++i) { bool overuse = GenerateAndProcessFrame(ssrc, bitrate_bps); if (overuse) { - EXPECT_LT(bitrate_observer_.latest_bitrate(), max_bitrate); - EXPECT_GT(bitrate_observer_.latest_bitrate(), min_bitrate); - bitrate_bps = bitrate_observer_.latest_bitrate(); + EXPECT_LT(bitrate_observer_->latest_bitrate(), max_bitrate); + EXPECT_GT(bitrate_observer_->latest_bitrate(), min_bitrate); + bitrate_bps = bitrate_observer_->latest_bitrate(); bitrate_update_seen = true; - } else if (bitrate_observer_.updated()) { - bitrate_bps = bitrate_observer_.latest_bitrate(); - bitrate_observer_.Reset(); + } else if (bitrate_observer_->updated()) { + bitrate_bps = bitrate_observer_->latest_bitrate(); + bitrate_observer_->Reset(); } if (bitrate_update_seen && bitrate_bps > target_bitrate) { break; @@ -267,12 +259,13 @@ void DelayBasedBweTest::InitialBehaviorTestHelper( int64_t send_time_ms = 0; uint16_t sequence_number = 0; std::vector ssrcs; - EXPECT_FALSE(bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_bps)); + EXPECT_FALSE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); EXPECT_EQ(0u, ssrcs.size()); clock_.AdvanceTimeMilliseconds(1000); - EXPECT_FALSE(bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_bps)); - EXPECT_FALSE(bitrate_observer_.updated()); - bitrate_observer_.Reset(); + bitrate_estimator_->Process(); + EXPECT_FALSE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); + EXPECT_FALSE(bitrate_observer_->updated()); + bitrate_observer_->Reset(); clock_.AdvanceTimeMilliseconds(1000); // Inserting packets for 5 seconds to get a valid estimate. for (int i = 0; i < 5 * kFramerate + 1 + kNumInitialPackets; ++i) { @@ -281,23 +274,25 @@ void DelayBasedBweTest::InitialBehaviorTestHelper( int cluster_id = i < kInitialProbingPackets ? 0 : PacketInfo::kNotAProbe; if (i == kNumInitialPackets) { - EXPECT_FALSE(bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_bps)); + bitrate_estimator_->Process(); + EXPECT_FALSE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); EXPECT_EQ(0u, ssrcs.size()); - EXPECT_FALSE(bitrate_observer_.updated()); - bitrate_observer_.Reset(); + EXPECT_FALSE(bitrate_observer_->updated()); + bitrate_observer_->Reset(); } IncomingFeedback(clock_.TimeInMilliseconds(), send_time_ms, sequence_number++, kMtu, cluster_id); clock_.AdvanceTimeMilliseconds(1000 / kFramerate); send_time_ms += kFrameIntervalMs; } - EXPECT_TRUE(bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_bps)); + bitrate_estimator_->Process(); + EXPECT_TRUE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); ASSERT_EQ(1u, ssrcs.size()); EXPECT_EQ(kDefaultSsrc, ssrcs.front()); EXPECT_NEAR(expected_converge_bitrate, bitrate_bps, kAcceptedBitrateErrorBps); - EXPECT_TRUE(bitrate_observer_.updated()); - bitrate_observer_.Reset(); - EXPECT_EQ(bitrate_observer_.latest_bitrate(), bitrate_bps); + EXPECT_TRUE(bitrate_observer_->updated()); + bitrate_observer_->Reset(); + EXPECT_EQ(bitrate_observer_->latest_bitrate(), bitrate_bps); } void DelayBasedBweTest::RateIncreaseReorderingTestHelper( @@ -316,16 +311,17 @@ void DelayBasedBweTest::RateIncreaseReorderingTestHelper( // as it doesn't do anything in Process(). if (i == kNumInitialPackets) { // Process after we have enough frames to get a valid input rate estimate. - - EXPECT_FALSE(bitrate_observer_.updated()); // No valid estimate. + bitrate_estimator_->Process(); + EXPECT_FALSE(bitrate_observer_->updated()); // No valid estimate. } IncomingFeedback(clock_.TimeInMilliseconds(), send_time_ms, sequence_number++, kMtu, cluster_id); clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); send_time_ms += kFrameIntervalMs; } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_.latest_bitrate(), + bitrate_estimator_->Process(); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_->latest_bitrate(), kAcceptedBitrateErrorBps); for (int i = 0; i < 10; ++i) { clock_.AdvanceTimeMilliseconds(2 * kFrameIntervalMs); @@ -337,8 +333,9 @@ void DelayBasedBweTest::RateIncreaseReorderingTestHelper( 1000); sequence_number += 2; } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_.latest_bitrate(), + bitrate_estimator_->Process(); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_->latest_bitrate(), kAcceptedBitrateErrorBps); } @@ -356,15 +353,15 @@ void DelayBasedBweTest::RateIncreaseRtpTimestampsTestHelper( while (bitrate_bps < 5e5) { bool overuse = GenerateAndProcessFrame(kDefaultSsrc, bitrate_bps); if (overuse) { - EXPECT_GT(bitrate_observer_.latest_bitrate(), bitrate_bps); - bitrate_bps = bitrate_observer_.latest_bitrate(); - bitrate_observer_.Reset(); - } else if (bitrate_observer_.updated()) { - bitrate_bps = bitrate_observer_.latest_bitrate(); - bitrate_observer_.Reset(); + EXPECT_GT(bitrate_observer_->latest_bitrate(), bitrate_bps); + bitrate_bps = bitrate_observer_->latest_bitrate(); + bitrate_observer_->Reset(); + } else if (bitrate_observer_->updated()) { + bitrate_bps = bitrate_observer_->latest_bitrate(); + bitrate_observer_->Reset(); } ++iterations; - // ASSERT_LE(iterations, expected_iterations); + ASSERT_LE(iterations, expected_iterations); } ASSERT_EQ(expected_iterations, iterations); } @@ -408,7 +405,7 @@ void DelayBasedBweTest::CapacityDropTestHelper( kDefaultSsrc, steady_state_time * kFramerate, kStartBitrate, kMinExpectedBitrate, kMaxExpectedBitrate, kInitialCapacityBps); EXPECT_NEAR(kInitialCapacityBps, bitrate_bps, 130000u); - bitrate_observer_.Reset(); + bitrate_observer_->Reset(); // Add an offset to make sure the BWE can handle it. arrival_time_offset_ms_ += receiver_clock_offset_change_ms; @@ -420,11 +417,11 @@ void DelayBasedBweTest::CapacityDropTestHelper( for (int i = 0; i < 100 * number_of_streams; ++i) { GenerateAndProcessFrame(kDefaultSsrc, bitrate_bps); if (bitrate_drop_time == -1 && - bitrate_observer_.latest_bitrate() <= kReducedCapacityBps) { + bitrate_observer_->latest_bitrate() <= kReducedCapacityBps) { bitrate_drop_time = clock_.TimeInMilliseconds(); } - if (bitrate_observer_.updated()) - bitrate_bps = bitrate_observer_.latest_bitrate(); + if (bitrate_observer_->updated()) + bitrate_bps = bitrate_observer_->latest_bitrate(); } EXPECT_NEAR(expected_bitrate_drop_delta, @@ -442,11 +439,12 @@ void DelayBasedBweTest::TestTimestampGroupingTestHelper() { IncomingFeedback(clock_.TimeInMilliseconds(), send_time_ms, sequence_number++, 1000); + bitrate_estimator_->Process(); clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); send_time_ms += kFrameIntervalMs; } - EXPECT_TRUE(bitrate_observer_.updated()); - EXPECT_GE(bitrate_observer_.latest_bitrate(), 400000u); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_GE(bitrate_observer_->latest_bitrate(), 400000u); // Insert batches of frames which were sent very close in time. Also simulate // capacity over-use to see that we back off correctly. @@ -463,10 +461,11 @@ void DelayBasedBweTest::TestTimestampGroupingTestHelper() { // Increase time until next batch to simulate over-use. clock_.AdvanceTimeMilliseconds(10); send_time_ms += kFrameIntervalMs - kTimestampGroupLength; + bitrate_estimator_->Process(); } - EXPECT_TRUE(bitrate_observer_.updated()); + EXPECT_TRUE(bitrate_observer_->updated()); // Should have reduced the estimate. - EXPECT_LT(bitrate_observer_.latest_bitrate(), 400000u); + EXPECT_LT(bitrate_observer_->latest_bitrate(), 400000u); } void DelayBasedBweTest::TestWrappingHelper(int silence_time_s) { @@ -480,22 +479,25 @@ void DelayBasedBweTest::TestWrappingHelper(int silence_time_s) { sequence_number++, 1000); clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); send_time_ms += kFrameIntervalMs; + bitrate_estimator_->Process(); } uint32_t bitrate_before = 0; std::vector ssrcs; - bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_before); + bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_before); clock_.AdvanceTimeMilliseconds(silence_time_s * 1000); send_time_ms += silence_time_s * 1000; + bitrate_estimator_->Process(); for (size_t i = 0; i < 21; ++i) { IncomingFeedback(clock_.TimeInMilliseconds(), send_time_ms, sequence_number++, 1000); clock_.AdvanceTimeMilliseconds(2 * kFrameIntervalMs); send_time_ms += kFrameIntervalMs; + bitrate_estimator_->Process(); } uint32_t bitrate_after = 0; - bitrate_estimator_.LatestEstimate(&ssrcs, &bitrate_after); + bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_after); EXPECT_LT(bitrate_after, bitrate_before); } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h index add9fb3277..f97cd5272d 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h @@ -19,7 +19,6 @@ #include "webrtc/test/gtest.h" #include "webrtc/base/constructormagic.h" -#include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "webrtc/system_wrappers/include/clock.h" @@ -161,8 +160,8 @@ class DelayBasedBweTest : public ::testing::Test { static const uint32_t kDefaultSsrc; SimulatedClock clock_; // Time at the receiver. - test::TestBitrateObserver bitrate_observer_; - DelayBasedBwe bitrate_estimator_; + std::unique_ptr bitrate_observer_; + std::unique_ptr bitrate_estimator_; std::unique_ptr stream_generator_; int64_t arrival_time_offset_ms_; diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index eb6db33712..862f2cd6be 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -15,12 +15,12 @@ #include "webrtc/base/constructormagic.h" #include "webrtc/common_types.h" -#include "webrtc/modules/congestion_controller/transport_feedback_adapter.h" #include "webrtc/modules/include/module.h" #include "webrtc/modules/include/module_common_types.h" #include "webrtc/modules/pacing/packet_router.h" #include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h" +#include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" namespace rtc { struct SentPacket; diff --git a/webrtc/modules/remote_bitrate_estimator/BUILD.gn b/webrtc/modules/remote_bitrate_estimator/BUILD.gn index 2a7c68f773..ea0e214bdc 100644 --- a/webrtc/modules/remote_bitrate_estimator/BUILD.gn +++ b/webrtc/modules/remote_bitrate_estimator/BUILD.gn @@ -30,6 +30,8 @@ rtc_static_library("remote_bitrate_estimator") { "remote_estimator_proxy.h", "send_time_history.cc", "test/bwe_test_logging.h", + "transport_feedback_adapter.cc", + "transport_feedback_adapter.h", ] if (!rtc_include_tests) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi index e719f2ff9d..3a2e9ea915 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi @@ -38,6 +38,8 @@ 'remote_estimator_proxy.cc', 'remote_estimator_proxy.h', 'send_time_history.cc', + 'transport_feedback_adapter.cc', + 'transport_feedback_adapter.h', 'test/bwe_test_logging.h', ], # source 'conditions': [ diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc similarity index 82% rename from webrtc/modules/congestion_controller/transport_feedback_adapter.cc rename to webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc index 8875806e3e..66ef7f0495 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc +++ b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc @@ -8,15 +8,14 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "webrtc/modules/congestion_controller/transport_feedback_adapter.h" +#include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" #include #include #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" -#include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" -#include "webrtc/modules/congestion_controller/delay_based_bwe.h" +#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "webrtc/modules/utility/include/process_thread.h" @@ -40,19 +39,20 @@ class PacketInfoComparator { }; TransportFeedbackAdapter::TransportFeedbackAdapter( - Clock* clock, - BitrateController* bitrate_controller) + Clock* clock) : send_time_history_(clock, kSendTimeHistoryWindowMs), clock_(clock), current_offset_ms_(kNoTimestamp), - last_timestamp_us_(kNoTimestamp), - bitrate_controller_(bitrate_controller) {} + last_timestamp_us_(kNoTimestamp) {} -TransportFeedbackAdapter::~TransportFeedbackAdapter() {} +TransportFeedbackAdapter::~TransportFeedbackAdapter() { +} -void TransportFeedbackAdapter::InitBwe() { - rtc::CritScope cs(&bwe_lock_); - delay_based_bwe_.reset(new DelayBasedBwe(clock_)); +void TransportFeedbackAdapter::SetBitrateEstimator( + RemoteBitrateEstimator* rbe) { + if (bitrate_estimator_.get() != rbe) { + bitrate_estimator_.reset(rbe); + } } void TransportFeedbackAdapter::AddPacket(uint16_t sequence_number, @@ -68,11 +68,6 @@ void TransportFeedbackAdapter::OnSentPacket(uint16_t sequence_number, send_time_history_.OnSentPacket(sequence_number, send_time_ms); } -void TransportFeedbackAdapter::SetMinBitrate(int min_bitrate_bps) { - rtc::CritScope cs(&bwe_lock_); - delay_based_bwe_->SetMinBitrate(min_bitrate_bps); -} - std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( const rtcp::TransportFeedback& feedback) { int64_t timestamp_us = feedback.GetBaseTimeUs(); @@ -134,14 +129,9 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( void TransportFeedbackAdapter::OnTransportFeedback( const rtcp::TransportFeedback& feedback) { last_packet_feedback_vector_ = GetPacketFeedbackVector(feedback); - DelayBasedBwe::Result result; - { - rtc::CritScope cs(&bwe_lock_); - delay_based_bwe_->IncomingPacketFeedbackVector( + if (bitrate_estimator_.get()) + bitrate_estimator_->IncomingPacketFeedbackVector( last_packet_feedback_vector_); - } - if (result.updated) - bitrate_controller_->OnDelayBasedBweResult(result); } std::vector TransportFeedbackAdapter::GetTransportFeedbackVector() @@ -151,8 +141,8 @@ std::vector TransportFeedbackAdapter::GetTransportFeedbackVector() void TransportFeedbackAdapter::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { - rtc::CritScope cs(&bwe_lock_); - delay_based_bwe_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); + RTC_DCHECK(bitrate_estimator_.get() != nullptr); + bitrate_estimator_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.h b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h similarity index 67% rename from webrtc/modules/congestion_controller/transport_feedback_adapter.h rename to webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h index 6422736c5c..2db6603984 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.h +++ b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h @@ -8,63 +8,58 @@ * be found in the AUTHORS file in the root of the source tree. */ -#ifndef WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ -#define WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ +#ifndef WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ +#define WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ #include #include #include "webrtc/base/criticalsection.h" #include "webrtc/base/thread_annotations.h" -#include "webrtc/base/thread_checker.h" -#include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/include/module_common_types.h" +#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h" namespace webrtc { -class BitrateController; class ProcessThread; class TransportFeedbackAdapter : public TransportFeedbackObserver, public CallStatsObserver { public: - TransportFeedbackAdapter(Clock* clock, BitrateController* bitrate_controller); + explicit TransportFeedbackAdapter(Clock* clock); virtual ~TransportFeedbackAdapter(); - void InitBwe(); + void SetBitrateEstimator(RemoteBitrateEstimator* rbe); + RemoteBitrateEstimator* GetBitrateEstimator() const { + return bitrate_estimator_.get(); + } + // Implements TransportFeedbackObserver. void AddPacket(uint16_t sequence_number, size_t length, int probe_cluster_id) override; void OnSentPacket(uint16_t sequence_number, int64_t send_time_ms); - // TODO(holmer): This method should return DelayBasedBwe::Result so that we - // can get rid of the dependency on BitrateController. Requires changes - // to the CongestionController interface. void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override; std::vector GetTransportFeedbackVector() const override; // Implements CallStatsObserver. void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; - void SetMinBitrate(int min_bitrate_bps); - private: std::vector GetPacketFeedbackVector( const rtcp::TransportFeedback& feedback); rtc::CriticalSection lock_; - rtc::CriticalSection bwe_lock_; SendTimeHistory send_time_history_ GUARDED_BY(&lock_); - std::unique_ptr delay_based_bwe_ GUARDED_BY(&bwe_lock_); + std::unique_ptr bitrate_estimator_; Clock* const clock_; int64_t current_offset_ms_; int64_t last_timestamp_us_; - BitrateController* const bitrate_controller_; std::vector last_packet_feedback_vector_; }; } // namespace webrtc -#endif // WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ +#endif // WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc similarity index 77% rename from webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc rename to webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc index 60c851442a..e2755b3994 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc @@ -12,12 +12,13 @@ #include #include -#include "webrtc/test/gmock.h" -#include "webrtc/test/gtest.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" #include "webrtc/base/checks.h" #include "webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h" -#include "webrtc/modules/congestion_controller/transport_feedback_adapter.h" +#include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h" +#include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "webrtc/system_wrappers/include/clock.h" @@ -31,16 +32,23 @@ namespace test { class TransportFeedbackAdapterTest : public ::testing::Test { public: TransportFeedbackAdapterTest() - : clock_(0), bitrate_controller_(this), receiver_estimated_bitrate_(0) {} + : clock_(0), + bitrate_estimator_(nullptr), + bitrate_controller_(this), + receiver_estimated_bitrate_(0) {} virtual ~TransportFeedbackAdapterTest() {} virtual void SetUp() { - adapter_.reset(new TransportFeedbackAdapter(&clock_, &bitrate_controller_)); - adapter_->InitBwe(); + adapter_.reset(new TransportFeedbackAdapter(&clock_)); + + bitrate_estimator_ = new MockRemoteBitrateEstimator(); + adapter_->SetBitrateEstimator(bitrate_estimator_); } - virtual void TearDown() { adapter_.reset(); } + virtual void TearDown() { + adapter_.reset(); + } protected: // Proxy class used since TransportFeedbackAdapter will own the instance @@ -52,8 +60,9 @@ class TransportFeedbackAdapterTest : public ::testing::Test { ~MockBitrateControllerAdapter() override {} - void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) override { - owner_->receiver_estimated_bitrate_ = result.target_bitrate_bps; + void OnReceiveBitrateChanged(const std::vector& ssrcs, + uint32_t bitrate_bps) override { + owner_->receiver_estimated_bitrate_ = bitrate_bps; } TransportFeedbackAdapterTest* const owner_; @@ -97,6 +106,7 @@ class TransportFeedbackAdapterTest : public ::testing::Test { } SimulatedClock clock_; + MockRemoteBitrateEstimator* bitrate_estimator_; MockBitrateControllerAdapter bitrate_controller_; std::unique_ptr adapter_; @@ -125,8 +135,13 @@ TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) { feedback.Build(); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke( + [packets, this](const std::vector& feedback_vector) { + ComparePacketVectors(packets, feedback_vector); + })); adapter_->OnTransportFeedback(feedback); - ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, HandlesDroppedPackets) { @@ -162,9 +177,13 @@ TEST_F(TransportFeedbackAdapterTest, HandlesDroppedPackets) { packets.begin() + kSendSideDropBefore, packets.begin() + kReceiveSideDropAfter + 1); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke([expected_packets, + this](const std::vector& feedback_vector) { + ComparePacketVectors(expected_packets, feedback_vector); + })); adapter_->OnTransportFeedback(feedback); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { @@ -198,9 +217,13 @@ TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { std::vector expected_packets; expected_packets.push_back(packets[i]); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke([expected_packets, this]( + const std::vector& feedback_vector) { + ComparePacketVectors(expected_packets, feedback_vector); + })); adapter_->OnTransportFeedback(*feedback.get()); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); } } @@ -228,9 +251,13 @@ TEST_F(TransportFeedbackAdapterTest, HandlesReordering) { feedback.Build(); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke([expected_packets, + this](const std::vector& feedback_vector) { + ComparePacketVectors(expected_packets, feedback_vector); + })); adapter_->OnTransportFeedback(feedback); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { @@ -267,6 +294,14 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { info.arrival_time_ms += (kLargePositiveDeltaUs + 1000) / 1000; ++info.sequence_number; + // Expected to be ordered on arrival time when the feedback message has been + // parsed. + std::vector expected_packets; + expected_packets.push_back(sent_packets[0]); + expected_packets.push_back(sent_packets[3]); + expected_packets.push_back(sent_packets[1]); + expected_packets.push_back(sent_packets[2]); + // Packets will be added to send history. for (const PacketInfo& packet : sent_packets) OnSentPacket(packet); @@ -292,18 +327,14 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { std::vector received_feedback; EXPECT_TRUE(feedback.get() != nullptr); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke([expected_packets, &received_feedback]( + const std::vector& feedback_vector) { + EXPECT_EQ(expected_packets.size(), feedback_vector.size()); + received_feedback = feedback_vector; + })); adapter_->OnTransportFeedback(*feedback.get()); - { - // Expected to be ordered on arrival time when the feedback message has been - // parsed. - std::vector expected_packets; - expected_packets.push_back(sent_packets[0]); - expected_packets.push_back(sent_packets[3]); - expected_packets.push_back(sent_packets[1]); - expected_packets.push_back(sent_packets[2]); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); - } // Create a new feedback message and add the trailing item. feedback.reset(new rtcp::TransportFeedback()); @@ -315,13 +346,18 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { rtcp::TransportFeedback::ParseFrom(raw_packet.data(), raw_packet.size()); EXPECT_TRUE(feedback.get() != nullptr); + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke( + [&received_feedback](const std::vector& feedback_vector) { + EXPECT_EQ(1u, feedback_vector.size()); + received_feedback.push_back(feedback_vector[0]); + })); adapter_->OnTransportFeedback(*feedback.get()); - { - std::vector expected_packets; - expected_packets.push_back(info); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); - } + + expected_packets.push_back(info); + + ComparePacketVectors(expected_packets, received_feedback); } } // namespace test diff --git a/webrtc/tools/DEPS b/webrtc/tools/DEPS index 507106a063..cc33de1f96 100644 --- a/webrtc/tools/DEPS +++ b/webrtc/tools/DEPS @@ -4,7 +4,6 @@ include_rules = [ "+webrtc/common_video", "+webrtc/modules/audio_device", "+webrtc/modules/audio_processing", - "+webrtc/modules/bitrate_controller", "+webrtc/modules/congestion_controller", "+webrtc/modules/rtp_rtcp", "+webrtc/system_wrappers", diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc index d1dec22f0a..260975b4b5 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc @@ -24,7 +24,6 @@ #include "webrtc/base/rate_statistics.h" #include "webrtc/call.h" #include "webrtc/common_types.h" -#include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" #include "webrtc/modules/congestion_controller/include/congestion_controller.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" @@ -1054,34 +1053,6 @@ void EventLogAnalyzer::CreateBweSimulationGraph(Plot* plot) { plot->SetTitle("Simulated BWE behavior"); } -// TODO(holmer): Remove once TransportFeedbackAdapter no longer needs a -// BitrateController. -class NullBitrateController : public BitrateController { - public: - ~NullBitrateController() override {} - RtcpBandwidthObserver* CreateRtcpBandwidthObserver() override { - return nullptr; - } - void SetStartBitrate(int start_bitrate_bps) override {} - void SetMinMaxBitrate(int min_bitrate_bps, int max_bitrate_bps) override {} - void SetBitrates(int start_bitrate_bps, - int min_bitrate_bps, - int max_bitrate_bps) override {} - void ResetBitrates(int bitrate_bps, - int min_bitrate_bps, - int max_bitrate_bps) override {} - void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) override {} - bool AvailableBandwidth(uint32_t* bandwidth) const override { return false; } - void SetReservedBitrate(uint32_t reserved_bitrate_bps) override {} - bool GetNetworkParameters(uint32_t* bitrate, - uint8_t* fraction_loss, - int64_t* rtt) override { - return false; - } - int64_t TimeUntilNextProcess() override { return 0; } - void Process() override {} -}; - void EventLogAnalyzer::CreateNetworkDelayFeedbackGraph(Plot* plot) { std::map outgoing_rtp; std::map incoming_rtcp; @@ -1102,8 +1073,7 @@ void EventLogAnalyzer::CreateNetworkDelayFeedbackGraph(Plot* plot) { } SimulatedClock clock(0); - NullBitrateController null_controller; - TransportFeedbackAdapter feedback_adapter(&clock, &null_controller); + TransportFeedbackAdapter feedback_adapter(&clock); TimeSeries time_series; time_series.label = "Network Delay Change"; diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index cce29558f1..656aadce37 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -1135,44 +1135,20 @@ TEST_F(VideoSendStreamTest, MinTransmitBitrateRespectsRemb) { RunBaseTest(&test); } -TEST_F(VideoSendStreamTest, ChangingNetworkRoute) { - static const int kStartBitrateBps = 300000; - static const int kNewMaxBitrateBps = 1234567; - static const uint8_t kExtensionId = 13; +TEST_F(VideoSendStreamTest, DISABLED_ChangingNetworkRoute) { class ChangingNetworkRouteTest : public test::EndToEndTest { public: + const int kStartBitrateBps = 300000; + const int kNewMaxBitrateBps = 1234567; + ChangingNetworkRouteTest() - : EndToEndTest(test::CallTest::kDefaultTimeoutMs), call_(nullptr) { - EXPECT_TRUE(parser_->RegisterRtpHeaderExtension( - kRtpExtensionTransportSequenceNumber, kExtensionId)); - } + : EndToEndTest(test::CallTest::kDefaultTimeoutMs), + call_(nullptr) {} void OnCallsCreated(Call* sender_call, Call* receiver_call) override { call_ = sender_call; } - void ModifyVideoConfigs( - VideoSendStream::Config* send_config, - std::vector* receive_configs, - VideoEncoderConfig* encoder_config) override { - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); - (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; - (*receive_configs)[0].rtp.transport_cc = true; - } - - void ModifyAudioConfigs( - AudioSendStream::Config* send_config, - std::vector* receive_configs) override { - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); - (*receive_configs)[0].rtp.extensions.clear(); - (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; - (*receive_configs)[0].rtp.transport_cc = true; - } - Action OnSendRtp(const uint8_t* packet, size_t length) override { if (call_->GetStats().send_bandwidth_bps > kStartBitrateBps) { observation_complete_.Set();