Reland: Fix race / crash in OnNetworkRouteChanged(). To achieve this some refactoring was done to make it possible to synchronize access to the DelayBasedBwe in TransportFeedbackAdapter: - The callback was removed from DelayBasedBwe, it now instead returns its result. - TransportFeedbackAdapter was moved to modules/congestion_controller to avoid unnecessary dependencies. Reenables previously disabled flaky test. Can no longer reproduce flakiness with gtest-parallel and asan/tsan builds. BUG=webrtc:6427, webrtc:6422 R=terelius@webrtc.org Review URL: https://codereview.webrtc.org/2378103005 . Cr-Commit-Position: refs/heads/master@{#14452}
diff --git a/webrtc/modules/BUILD.gn b/webrtc/modules/BUILD.gn index ccc681c..b3bfbac 100644 --- a/webrtc/modules/BUILD.gn +++ b/webrtc/modules/BUILD.gn
@@ -375,6 +375,7 @@ "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", @@ -394,7 +395,6 @@ "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 bc2f1f6..b336367 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
@@ -186,22 +186,18 @@ MaybeTriggerOnNetworkChanged(); } -void BitrateControllerImpl::OnProbeBitrate(uint32_t bitrate_bps) { +void BitrateControllerImpl::OnDelayBasedBweResult( + const DelayBasedBwe::Result& result) { + if (!result.updated) + return; { rtc::CritScope cs(&critsect_); - bandwidth_estimation_.SetSendBitrate(bitrate_bps); - } - MaybeTriggerOnNetworkChanged(); -} - -// TODO(isheriff): Perhaps need new interface for invocation from DelayBasedBwe. -void BitrateControllerImpl::OnReceiveBitrateChanged( - const std::vector<uint32_t>& ssrcs, - uint32_t bitrate_bps) { - { - rtc::CritScope cs(&critsect_); - bandwidth_estimation_.UpdateDelayBasedEstimate(clock_->TimeInMilliseconds(), - bitrate_bps); + if (result.probe) { + bandwidth_estimation_.SetSendBitrate(result.target_bitrate_bps); + } else { + bandwidth_estimation_.UpdateDelayBasedEstimate( + clock_->TimeInMilliseconds(), result.target_bitrate_bps); + } } MaybeTriggerOnNetworkChanged(); }
diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h index c8bb102..7ee6b19 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h
@@ -61,10 +61,7 @@ uint8_t* fraction_loss, int64_t* rtt) override; - // RemoteBitrateObserver overrides. - void OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs, - uint32_t bitrate_bps) override; - void OnProbeBitrate(uint32_t bitrate_bps) override; + void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) 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 7da947b..4b298cc 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc
@@ -169,7 +169,8 @@ EXPECT_EQ(300000, bitrate_observer_.last_bitrate_); // Test that a low delay-based estimate limits the combined estimate. - controller_->OnReceiveBitrateChanged({0}, 280000); + webrtc::DelayBasedBwe::Result result(false, 280000); + controller_->OnDelayBasedBweResult(result); 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 90b6471..7400d7d 100644 --- a/webrtc/modules/bitrate_controller/include/bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/bitrate_controller.h
@@ -17,9 +17,9 @@ #include <map> +#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 @@ virtual ~BitrateObserver() {} }; -class BitrateController : public Module, public RemoteBitrateObserver { +class BitrateController : public Module { // 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,6 +78,8 @@ 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 758fba5..1069d62 100644 --- a/webrtc/modules/congestion_controller/BUILD.gn +++ b/webrtc/modules/congestion_controller/BUILD.gn
@@ -18,6 +18,8 @@ "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. @@ -32,6 +34,5 @@ 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 c0ec3da..f7e7e56 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc
@@ -21,7 +21,6 @@ #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" @@ -171,7 +170,7 @@ retransmission_rate_limiter_( new RateLimiter(clock, kRetransmitWindowSizeMs)), remote_estimator_proxy_(clock_, packet_router_.get()), - transport_feedback_adapter_(clock_), + transport_feedback_adapter_(clock_, bitrate_controller_.get()), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), max_bitrate_bps_(0), last_reported_bitrate_bps_(0), @@ -202,7 +201,7 @@ retransmission_rate_limiter_( new RateLimiter(clock, kRetransmitWindowSizeMs)), remote_estimator_proxy_(clock_, packet_router_.get()), - transport_feedback_adapter_(clock_), + transport_feedback_adapter_(clock_, bitrate_controller_.get()), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), max_bitrate_bps_(0), last_reported_bitrate_bps_(0), @@ -215,10 +214,8 @@ CongestionController::~CongestionController() {} void CongestionController::Init() { - transport_feedback_adapter_.SetBitrateEstimator( - new DelayBasedBwe(bitrate_controller_.get(), clock_)); - transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( - min_bitrate_bps_); + transport_feedback_adapter_.InitBwe(); + transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); } void CongestionController::SetBweBitrates(int min_bitrate_bps, @@ -236,8 +233,7 @@ if (remote_bitrate_estimator_) remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; - transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( - min_bitrate_bps_); + transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); MaybeTriggerOnNetworkChanged(); } @@ -256,10 +252,8 @@ if (remote_bitrate_estimator_) remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); - RemoteBitrateEstimator* rbe = - new DelayBasedBwe(bitrate_controller_.get(), clock_); - transport_feedback_adapter_.SetBitrateEstimator(rbe); - rbe->SetMinBitrate(min_bitrate_bps); + transport_feedback_adapter_.InitBwe(); + transport_feedback_adapter_.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 5c7ddf2..3fcddfb 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.gypi +++ b/webrtc/modules/congestion_controller/congestion_controller.gypi
@@ -14,7 +14,6 @@ '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', @@ -25,6 +24,8 @@ '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 87dc502..34b8242 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc
@@ -21,7 +21,6 @@ #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" @@ -40,9 +39,8 @@ namespace webrtc { -DelayBasedBwe::DelayBasedBwe(RemoteBitrateObserver* observer, Clock* clock) +DelayBasedBwe::DelayBasedBwe(Clock* clock) : clock_(clock), - observer_(observer), inter_arrival_(), estimator_(), detector_(OverUseDetectorOptions()), @@ -50,11 +48,10 @@ last_update_ms_(-1), last_seen_packet_ms_(-1), uma_recorded_(false) { - RTC_DCHECK(observer_); network_thread_.DetachFromThread(); } -void DelayBasedBwe::IncomingPacketFeedbackVector( +DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( const std::vector<PacketInfo>& packet_feedback_vector) { RTC_DCHECK(network_thread_.CalledOnValidThread()); if (!uma_recorded_) { @@ -63,87 +60,87 @@ BweNames::kBweNamesMax); uma_recorded_ = true; } + Result aggregated_result; for (const auto& packet_info : packet_feedback_vector) { - IncomingPacketInfo(packet_info); + Result result = IncomingPacketInfo(packet_info); + if (result.updated) + aggregated_result = result; } + return aggregated_result; } -void DelayBasedBwe::IncomingPacketInfo(const PacketInfo& info) { +DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo( + const PacketInfo& info) { int64_t now_ms = clock_->TimeInMilliseconds(); incoming_bitrate_.Update(info.payload_size, info.arrival_time_ms); - bool delay_based_bwe_changed = false; - uint32_t target_bitrate_bps = 0; - { - rtc::CritScope lock(&crit_); + 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; - // 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; + uint32_t send_time_24bits = + static_cast<uint32_t>( + ((static_cast<uint64_t>(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 send_time_24bits = - static_cast<uint32_t>(((static_cast<uint64_t>(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<uint32_t> 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); - } + 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); } - if (delay_based_bwe_changed) { + 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<uint32_t> 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); + } + } 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<uint32_t> 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; - observer_->OnReceiveBitrateChanged({kFixedSsrc}, target_bitrate_bps); - } + + return result; } bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms, @@ -160,20 +157,10 @@ 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<uint32_t>* ssrcs, uint32_t* bitrate_bps) const { // Currently accessed from both the process thread (see @@ -182,7 +169,6 @@ // thread. RTC_DCHECK(ssrcs); RTC_DCHECK(bitrate_bps); - rtc::CritScope lock(&crit_); if (!remote_rate_.ValidEstimate()) return false; @@ -194,7 +180,6 @@ 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 3e0a014..7e2c8bd 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.h +++ b/webrtc/modules/congestion_controller/delay_based_bwe.h
@@ -18,7 +18,6 @@ #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" @@ -27,46 +26,42 @@ #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 : public RemoteBitrateEstimator { +class DelayBasedBwe { public: - DelayBasedBwe(RemoteBitrateObserver* observer, Clock* clock); + 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); virtual ~DelayBasedBwe() {} - void IncomingPacketFeedbackVector( - const std::vector<PacketInfo>& packet_feedback_vector) override; - void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; + Result IncomingPacketFeedbackVector( + const std::vector<PacketInfo>& packet_feedback_vector); + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms); bool LatestEstimate(std::vector<uint32_t>* ssrcs, - 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(); - } + uint32_t* bitrate_bps) const; + void SetMinBitrate(int min_bitrate_bps); private: - void IncomingPacketInfo(const PacketInfo& info); + Result 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) - EXCLUSIVE_LOCKS_REQUIRED(crit_); + uint32_t* target_bitrate_bps); rtc::ThreadChecker network_thread_; Clock* const clock_; - RemoteBitrateObserver* const observer_; std::unique_ptr<InterArrival> inter_arrival_; std::unique_ptr<OveruseEstimator> estimator_; OveruseDetector detector_; @@ -74,10 +69,8 @@ int64_t last_update_ms_; int64_t last_seen_packet_ms_; bool uma_recorded_; - - rtc::CriticalSection crit_; - AimdRateControl remote_rate_ GUARDED_BY(&crit_); - ProbeBitrateEstimator probe_bitrate_estimator_ GUARDED_BY(&crit_); + AimdRateControl remote_rate_; + ProbeBitrateEstimator probe_bitrate_estimator_; 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 7220967..e751013 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 3469251..a3a1893 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc
@@ -150,8 +150,7 @@ DelayBasedBweTest::DelayBasedBweTest() : clock_(100000000), - bitrate_observer_(new test::TestBitrateObserver), - bitrate_estimator_(new DelayBasedBwe(bitrate_observer_.get(), &clock_)), + bitrate_estimator_(&clock_), stream_generator_(new test::StreamGenerator(1e6, // Capacity. clock_.TimeInMicroseconds())), arrival_time_offset_ms_(0) {} @@ -182,7 +181,13 @@ sequence_number, payload_size, probe_cluster_id); std::vector<PacketInfo> packets; packets.push_back(packet); - bitrate_estimator_->IncomingPacketFeedbackVector(packets); + DelayBasedBwe::Result result = + bitrate_estimator_.IncomingPacketFeedbackVector(packets); + const uint32_t kDummySsrc = 0; + if (result.updated) { + bitrate_observer_.OnReceiveBitrateChanged({kDummySsrc}, + result.target_bitrate_bps); + } } // Generates a frame of packets belonging to a stream at a given bitrate and @@ -201,17 +206,20 @@ 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_; } - bitrate_estimator_->IncomingPacketFeedbackVector(packets); - - if (bitrate_observer_->updated()) { - if (bitrate_observer_->latest_bitrate() < bitrate_bps) + 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) overuse = true; } @@ -235,13 +243,13 @@ 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; @@ -259,13 +267,12 @@ int64_t send_time_ms = 0; uint16_t sequence_number = 0; std::vector<uint32_t> 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); - bitrate_estimator_->Process(); - EXPECT_FALSE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); - EXPECT_FALSE(bitrate_observer_->updated()); - bitrate_observer_->Reset(); + 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) { @@ -274,25 +281,23 @@ int cluster_id = i < kInitialProbingPackets ? 0 : PacketInfo::kNotAProbe; if (i == kNumInitialPackets) { - bitrate_estimator_->Process(); - EXPECT_FALSE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); + 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; } - bitrate_estimator_->Process(); - EXPECT_TRUE(bitrate_estimator_->LatestEstimate(&ssrcs, &bitrate_bps)); + 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( @@ -311,17 +316,16 @@ // as it doesn't do anything in Process(). if (i == kNumInitialPackets) { // Process after we have enough frames to get a valid input rate estimate. - bitrate_estimator_->Process(); - EXPECT_FALSE(bitrate_observer_->updated()); // No valid estimate. + + 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; } - bitrate_estimator_->Process(); - EXPECT_TRUE(bitrate_observer_->updated()); - EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_->latest_bitrate(), + 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); @@ -333,9 +337,8 @@ 1000); sequence_number += 2; } - bitrate_estimator_->Process(); - EXPECT_TRUE(bitrate_observer_->updated()); - EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_->latest_bitrate(), + EXPECT_TRUE(bitrate_observer_.updated()); + EXPECT_NEAR(expected_bitrate_bps, bitrate_observer_.latest_bitrate(), kAcceptedBitrateErrorBps); } @@ -353,15 +356,15 @@ 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); } @@ -405,7 +408,7 @@ 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; @@ -417,11 +420,11 @@ 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, @@ -439,12 +442,11 @@ 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. @@ -461,11 +463,10 @@ // 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) { @@ -479,25 +480,22 @@ sequence_number++, 1000); clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); send_time_ms += kFrameIntervalMs; - bitrate_estimator_->Process(); } uint32_t bitrate_before = 0; std::vector<uint32_t> 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 f97cd52..add9fb3 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h
@@ -19,6 +19,7 @@ #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" @@ -160,8 +161,8 @@ static const uint32_t kDefaultSsrc; SimulatedClock clock_; // Time at the receiver. - std::unique_ptr<test::TestBitrateObserver> bitrate_observer_; - std::unique_ptr<RemoteBitrateEstimator> bitrate_estimator_; + test::TestBitrateObserver bitrate_observer_; + DelayBasedBwe bitrate_estimator_; std::unique_ptr<test::StreamGenerator> 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 862f2cd..eb6db33 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/transport_feedback_adapter.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc similarity index 82% rename from webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc rename to webrtc/modules/congestion_controller/transport_feedback_adapter.cc index 66ef7f0..4663906 100644 --- a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc
@@ -8,14 +8,15 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" +#include "webrtc/modules/congestion_controller/transport_feedback_adapter.h" #include <algorithm> #include <limits> #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" -#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" +#include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" +#include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "webrtc/modules/utility/include/process_thread.h" @@ -39,20 +40,19 @@ }; TransportFeedbackAdapter::TransportFeedbackAdapter( - Clock* clock) + Clock* clock, + BitrateController* bitrate_controller) : send_time_history_(clock, kSendTimeHistoryWindowMs), clock_(clock), current_offset_ms_(kNoTimestamp), - last_timestamp_us_(kNoTimestamp) {} + last_timestamp_us_(kNoTimestamp), + bitrate_controller_(bitrate_controller) {} -TransportFeedbackAdapter::~TransportFeedbackAdapter() { -} +TransportFeedbackAdapter::~TransportFeedbackAdapter() {} -void TransportFeedbackAdapter::SetBitrateEstimator( - RemoteBitrateEstimator* rbe) { - if (bitrate_estimator_.get() != rbe) { - bitrate_estimator_.reset(rbe); - } +void TransportFeedbackAdapter::InitBwe() { + rtc::CritScope cs(&bwe_lock_); + delay_based_bwe_.reset(new DelayBasedBwe(clock_)); } void TransportFeedbackAdapter::AddPacket(uint16_t sequence_number, @@ -68,6 +68,11 @@ 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<PacketInfo> TransportFeedbackAdapter::GetPacketFeedbackVector( const rtcp::TransportFeedback& feedback) { int64_t timestamp_us = feedback.GetBaseTimeUs(); @@ -129,9 +134,14 @@ void TransportFeedbackAdapter::OnTransportFeedback( const rtcp::TransportFeedback& feedback) { last_packet_feedback_vector_ = GetPacketFeedbackVector(feedback); - if (bitrate_estimator_.get()) - bitrate_estimator_->IncomingPacketFeedbackVector( + DelayBasedBwe::Result result; + { + rtc::CritScope cs(&bwe_lock_); + result = delay_based_bwe_->IncomingPacketFeedbackVector( last_packet_feedback_vector_); + } + if (result.updated) + bitrate_controller_->OnDelayBasedBweResult(result); } std::vector<PacketInfo> TransportFeedbackAdapter::GetTransportFeedbackVector() @@ -141,8 +151,8 @@ void TransportFeedbackAdapter::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { - RTC_DCHECK(bitrate_estimator_.get() != nullptr); - bitrate_estimator_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); + rtc::CritScope cs(&bwe_lock_); + delay_based_bwe_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); } } // namespace webrtc
diff --git a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h b/webrtc/modules/congestion_controller/transport_feedback_adapter.h similarity index 67% rename from webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h rename to webrtc/modules/congestion_controller/transport_feedback_adapter.h index 2db6603..6422736 100644 --- a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.h
@@ -8,58 +8,63 @@ * be found in the AUTHORS file in the root of the source tree. */ -#ifndef WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ -#define WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ +#ifndef WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ +#define WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ #include <memory> #include <vector> #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: - explicit TransportFeedbackAdapter(Clock* clock); + TransportFeedbackAdapter(Clock* clock, BitrateController* bitrate_controller); virtual ~TransportFeedbackAdapter(); - void SetBitrateEstimator(RemoteBitrateEstimator* rbe); - RemoteBitrateEstimator* GetBitrateEstimator() const { - return bitrate_estimator_.get(); - } - + void InitBwe(); // 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<PacketInfo> 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<PacketInfo> GetPacketFeedbackVector( const rtcp::TransportFeedback& feedback); rtc::CriticalSection lock_; + rtc::CriticalSection bwe_lock_; SendTimeHistory send_time_history_ GUARDED_BY(&lock_); - std::unique_ptr<RemoteBitrateEstimator> bitrate_estimator_; + std::unique_ptr<DelayBasedBwe> delay_based_bwe_ GUARDED_BY(&bwe_lock_); Clock* const clock_; int64_t current_offset_ms_; int64_t last_timestamp_us_; + BitrateController* const bitrate_controller_; std::vector<PacketInfo> last_packet_feedback_vector_; }; } // namespace webrtc -#endif // WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_TRANSPORT_FEEDBACK_ADAPTER_H_ +#endif // WEBRTC_MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_
diff --git a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc similarity index 78% rename from webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc rename to webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc index b312f96..0c77fe9 100644 --- a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc
@@ -17,8 +17,7 @@ #include "webrtc/base/checks.h" #include "webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.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/congestion_controller/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" @@ -32,23 +31,16 @@ class TransportFeedbackAdapterTest : public ::testing::Test { public: TransportFeedbackAdapterTest() - : clock_(0), - bitrate_estimator_(nullptr), - bitrate_controller_(this), - receiver_estimated_bitrate_(0) {} + : clock_(0), bitrate_controller_(this), target_bitrate_bps_(0) {} virtual ~TransportFeedbackAdapterTest() {} virtual void SetUp() { - adapter_.reset(new TransportFeedbackAdapter(&clock_)); - - bitrate_estimator_ = new MockRemoteBitrateEstimator(); - adapter_->SetBitrateEstimator(bitrate_estimator_); + adapter_.reset(new TransportFeedbackAdapter(&clock_, &bitrate_controller_)); + adapter_->InitBwe(); } - virtual void TearDown() { - adapter_.reset(); - } + virtual void TearDown() { adapter_.reset(); } protected: // Proxy class used since TransportFeedbackAdapter will own the instance @@ -60,9 +52,8 @@ ~MockBitrateControllerAdapter() override {} - void OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs, - uint32_t bitrate_bps) override { - owner_->receiver_estimated_bitrate_ = bitrate_bps; + void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) override { + owner_->target_bitrate_bps_ = result.target_bitrate_bps; } TransportFeedbackAdapterTest* const owner_; @@ -106,11 +97,10 @@ } SimulatedClock clock_; - MockRemoteBitrateEstimator* bitrate_estimator_; MockBitrateControllerAdapter bitrate_controller_; std::unique_ptr<TransportFeedbackAdapter> adapter_; - uint32_t receiver_estimated_bitrate_; + uint32_t target_bitrate_bps_; }; TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) { @@ -135,13 +125,8 @@ feedback.Build(); - EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) - .Times(1) - .WillOnce(Invoke( - [packets, this](const std::vector<PacketInfo>& feedback_vector) { - ComparePacketVectors(packets, feedback_vector); - })); adapter_->OnTransportFeedback(feedback); + ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, HandlesDroppedPackets) { @@ -177,13 +162,9 @@ packets.begin() + kSendSideDropBefore, packets.begin() + kReceiveSideDropAfter + 1); - EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) - .Times(1) - .WillOnce(Invoke([expected_packets, - this](const std::vector<PacketInfo>& feedback_vector) { - ComparePacketVectors(expected_packets, feedback_vector); - })); adapter_->OnTransportFeedback(feedback); + ComparePacketVectors(expected_packets, + adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { @@ -217,13 +198,9 @@ std::vector<PacketInfo> expected_packets; expected_packets.push_back(packets[i]); - EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) - .Times(1) - .WillOnce(Invoke([expected_packets, this]( - const std::vector<PacketInfo>& feedback_vector) { - ComparePacketVectors(expected_packets, feedback_vector); - })); adapter_->OnTransportFeedback(*feedback.get()); + ComparePacketVectors(expected_packets, + adapter_->GetTransportFeedbackVector()); } } @@ -251,13 +228,9 @@ feedback.Build(); - EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) - .Times(1) - .WillOnce(Invoke([expected_packets, - this](const std::vector<PacketInfo>& feedback_vector) { - ComparePacketVectors(expected_packets, feedback_vector); - })); adapter_->OnTransportFeedback(feedback); + ComparePacketVectors(expected_packets, + adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { @@ -294,14 +267,6 @@ 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<PacketInfo> 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); @@ -327,14 +292,18 @@ std::vector<PacketInfo> received_feedback; EXPECT_TRUE(feedback.get() != nullptr); - EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) - .Times(1) - .WillOnce(Invoke([expected_packets, &received_feedback]( - const std::vector<PacketInfo>& 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<PacketInfo> 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()); @@ -346,18 +315,41 @@ 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<PacketInfo>& feedback_vector) { - EXPECT_EQ(1u, feedback_vector.size()); - received_feedback.push_back(feedback_vector[0]); - })); adapter_->OnTransportFeedback(*feedback.get()); + { + std::vector<PacketInfo> expected_packets; + expected_packets.push_back(info); + ComparePacketVectors(expected_packets, + adapter_->GetTransportFeedbackVector()); + } +} - expected_packets.push_back(info); - - ComparePacketVectors(expected_packets, received_feedback); +TEST_F(TransportFeedbackAdapterTest, UpdatesDelayBasedEstimate) { + uint16_t seq_num = 0; + size_t kPayloadSize = 1000; + // The test must run and insert packets/feedback long enough that the + // BWE computes a valid estimate. + const int64_t kRunTimeMs = 6000; + int64_t start_time_ms = clock_.TimeInMilliseconds(); + while (clock_.TimeInMilliseconds() - start_time_ms < kRunTimeMs) { + PacketInfo packet(clock_.TimeInMilliseconds(), clock_.TimeInMilliseconds(), + seq_num, kPayloadSize, PacketInfo::kNotAProbe); + OnSentPacket(packet); + // Create expected feedback and send into adapter. + std::unique_ptr<rtcp::TransportFeedback> feedback( + new rtcp::TransportFeedback()); + feedback->SetBase(packet.sequence_number, packet.arrival_time_ms * 1000); + EXPECT_TRUE(feedback->AddReceivedPacket(packet.sequence_number, + packet.arrival_time_ms * 1000)); + rtc::Buffer raw_packet = feedback->Build(); + feedback = rtcp::TransportFeedback::ParseFrom(raw_packet.data(), + raw_packet.size()); + EXPECT_TRUE(feedback.get() != nullptr); + adapter_->OnTransportFeedback(*feedback.get()); + clock_.AdvanceTimeMilliseconds(50); + ++seq_num; + } + EXPECT_GT(target_bitrate_bps_, 0u); } } // namespace test
diff --git a/webrtc/modules/remote_bitrate_estimator/BUILD.gn b/webrtc/modules/remote_bitrate_estimator/BUILD.gn index ea0e214..2a7c68f 100644 --- a/webrtc/modules/remote_bitrate_estimator/BUILD.gn +++ b/webrtc/modules/remote_bitrate_estimator/BUILD.gn
@@ -30,8 +30,6 @@ "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 3a2e9ea..e719f2f 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi
@@ -38,8 +38,6 @@ '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/tools/DEPS b/webrtc/tools/DEPS index cc33de1..507106a 100644 --- a/webrtc/tools/DEPS +++ b/webrtc/tools/DEPS
@@ -4,6 +4,7 @@ "+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 260975b..d1dec22 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc
@@ -24,6 +24,7 @@ #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" @@ -1053,6 +1054,34 @@ 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<uint64_t, const LoggedRtpPacket*> outgoing_rtp; std::map<uint64_t, const LoggedRtcpPacket*> incoming_rtcp; @@ -1073,7 +1102,8 @@ } SimulatedClock clock(0); - TransportFeedbackAdapter feedback_adapter(&clock); + NullBitrateController null_controller; + TransportFeedbackAdapter feedback_adapter(&clock, &null_controller); 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 656aadc..cce2955 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc
@@ -1135,20 +1135,44 @@ RunBaseTest(&test); } -TEST_F(VideoSendStreamTest, DISABLED_ChangingNetworkRoute) { +TEST_F(VideoSendStreamTest, ChangingNetworkRoute) { + static const int kStartBitrateBps = 300000; + static const int kNewMaxBitrateBps = 1234567; + static const uint8_t kExtensionId = 13; class ChangingNetworkRouteTest : public test::EndToEndTest { public: - const int kStartBitrateBps = 300000; - const int kNewMaxBitrateBps = 1234567; - ChangingNetworkRouteTest() - : EndToEndTest(test::CallTest::kDefaultTimeoutMs), - call_(nullptr) {} + : EndToEndTest(test::CallTest::kDefaultTimeoutMs), call_(nullptr) { + EXPECT_TRUE(parser_->RegisterRtpHeaderExtension( + kRtpExtensionTransportSequenceNumber, kExtensionId)); + } void OnCallsCreated(Call* sender_call, Call* receiver_call) override { call_ = sender_call; } + void ModifyVideoConfigs( + VideoSendStream::Config* send_config, + std::vector<VideoReceiveStream::Config>* 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<AudioReceiveStream::Config>* 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();