From 1112b2bc68e6c900f56fad79fa7dc1c1796ce9e9 Mon Sep 17 00:00:00 2001 From: stefan Date: Thu, 14 Apr 2016 08:08:15 -0700 Subject: [PATCH] Fix bug when the BWE times out due to no incoming packets. Both InterArrival and OveruseEstimator should be timed out at the same time since otherwise the overuse filter may take a long time to converge. BUG=webrtc:5773 Review URL: https://codereview.webrtc.org/1886783002 Cr-Commit-Position: refs/heads/master@{#12364} --- .../congestion_controller.cc | 5 ++-- .../remote_bitrate_estimator/inter_arrival.cc | 2 +- .../remote_bitrate_estimator_abs_send_time.cc | 26 +++++++++---------- .../remote_bitrate_estimator_abs_send_time.h | 6 ++--- ...itrate_estimator_abs_send_time_unittest.cc | 2 +- ...emote_bitrate_estimator_unittest_helper.cc | 2 +- .../test/estimators/remb.cc | 2 +- .../test/estimators/send_side.cc | 2 +- .../remote_bitrate_estimator/tools/bwe_rtp.cc | 2 +- 9 files changed, 23 insertions(+), 26 deletions(-) diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 11ce46adf3..caf39f07d2 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -115,7 +115,7 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator { // Instantiate RBE for Time Offset or Absolute Send Time extensions. void PickEstimator() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_.get()) { if (using_absolute_send_time_) { - rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_, clock_)); + rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_)); } else { rbe_.reset(new RemoteBitrateEstimatorSingleStream(observer_, clock_)); } @@ -156,8 +156,7 @@ CongestionController::CongestionController( transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps) { transport_feedback_adapter_.SetBitrateEstimator( - new RemoteBitrateEstimatorAbsSendTime(&transport_feedback_adapter_, - clock_)); + new RemoteBitrateEstimatorAbsSendTime(&transport_feedback_adapter_)); transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( min_bitrate_bps_); } diff --git a/webrtc/modules/remote_bitrate_estimator/inter_arrival.cc b/webrtc/modules/remote_bitrate_estimator/inter_arrival.cc index f75bc2b03e..1b7ce07583 100644 --- a/webrtc/modules/remote_bitrate_estimator/inter_arrival.cc +++ b/webrtc/modules/remote_bitrate_estimator/inter_arrival.cc @@ -18,7 +18,7 @@ namespace webrtc { -static const int kBurstDeltaThresholdMs = 5; +static const int kBurstDeltaThresholdMs = 5; InterArrival::InterArrival(uint32_t timestamp_group_length_ticks, double timestamp_to_ms_coeff, diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 7c2abb2f08..a13f6df213 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -14,12 +14,12 @@ #include +#include "webrtc/base/checks.h" #include "webrtc/base/constructormagic.h" #include "webrtc/base/logging.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" -#include "webrtc/system_wrappers/include/clock.h" #include "webrtc/system_wrappers/include/critical_section_wrapper.h" #include "webrtc/typedefs.h" @@ -79,20 +79,17 @@ bool RemoteBitrateEstimatorAbsSendTime::IsWithinClusterBounds( } RemoteBitrateEstimatorAbsSendTime::RemoteBitrateEstimatorAbsSendTime( - RemoteBitrateObserver* observer, - Clock* clock) + RemoteBitrateObserver* observer) : observer_(observer), inter_arrival_(), - estimator_(OverUseDetectorOptions()), + estimator_(), detector_(OverUseDetectorOptions()), incoming_bitrate_(kBitrateWindowMs, 8000), total_probes_received_(0), first_packet_time_ms_(-1), last_update_ms_(-1), - ssrcs_(), - clock_(clock) { + ssrcs_() { RTC_DCHECK(observer_); - RTC_DCHECK(clock_); LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating."; network_thread_.DetachFromThread(); process_thread_.DetachFromThread(); @@ -245,13 +242,13 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; int64_t send_time_ms = static_cast(timestamp) * kTimestampToMs; - int64_t now_ms = clock_->TimeInMilliseconds(); + int64_t now_ms = arrival_time_ms; // TODO(holmer): SSRCs are only needed for REMB, should be broken out from // here. incoming_bitrate_.Update(payload_size, now_ms); if (first_packet_time_ms_ == -1) - first_packet_time_ms_ = clock_->TimeInMilliseconds(); + first_packet_time_ms_ = arrival_time_ms; uint32_t ts_delta = 0; int64_t t_delta = 0; @@ -267,6 +264,8 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( rtc::CritScope lock(&crit_); TimeoutStreams(now_ms); + RTC_DCHECK(inter_arrival_.get()); + RTC_DCHECK(estimator_.get()); ssrcs_[ssrc] = now_ms; if (was_paced && @@ -295,9 +294,9 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( if (inter_arrival_->ComputeDeltas(timestamp, arrival_time_ms, 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()); - detector_.Detect(estimator_.offset(), ts_delta_ms, - estimator_.num_of_deltas(), arrival_time_ms); + estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State()); + detector_.Detect(estimator_->offset(), ts_delta_ms, + estimator_->num_of_deltas(), arrival_time_ms); } if (!update_estimate) { @@ -319,7 +318,7 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( // and the target bitrate is too high compared to what we are receiving. const RateControlInput input(detector_.State(), incoming_bitrate_.Rate(now_ms), - estimator_.var_noise()); + estimator_->var_noise()); remote_rate_.Update(&input, now_ms); target_bitrate_bps = remote_rate_.UpdateBandwidthEstimate(now_ms); update_estimate = remote_rate_.ValidEstimate(); @@ -352,6 +351,7 @@ void RemoteBitrateEstimatorAbsSendTime::TimeoutStreams(int64_t now_ms) { inter_arrival_.reset( new InterArrival((kTimestampGroupLengthMs << kInterArrivalShift) / 1000, kTimestampToMs, true)); + estimator_.reset(new OveruseEstimator(OverUseDetectorOptions())); // We deliberately don't reset the first_packet_time_ms_ here for now since // we only probe for bandwidth in the beginning of a call right now. } diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h index 1f47dc3b24..2d92b2976f 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h @@ -67,8 +67,7 @@ struct Cluster { class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { public: - RemoteBitrateEstimatorAbsSendTime(RemoteBitrateObserver* observer, - Clock* clock); + explicit RemoteBitrateEstimatorAbsSendTime(RemoteBitrateObserver* observer); virtual ~RemoteBitrateEstimatorAbsSendTime() {} void IncomingPacketFeedbackVector( @@ -121,7 +120,7 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { rtc::ThreadChecker network_thread_; RemoteBitrateObserver* const observer_; std::unique_ptr inter_arrival_; - OveruseEstimator estimator_; + std::unique_ptr estimator_; OveruseDetector detector_; RateStatistics incoming_bitrate_; std::vector recent_propagation_delta_ms_; @@ -135,7 +134,6 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { rtc::CriticalSection crit_; Ssrcs ssrcs_ GUARDED_BY(&crit_); AimdRateControl remote_rate_ GUARDED_BY(&crit_); - Clock* const clock_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RemoteBitrateEstimatorAbsSendTime); }; diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc index e8026a5764..8f57bbb58b 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc @@ -20,7 +20,7 @@ class RemoteBitrateEstimatorAbsSendTimeTest : RemoteBitrateEstimatorAbsSendTimeTest() {} virtual void SetUp() { bitrate_estimator_.reset(new RemoteBitrateEstimatorAbsSendTime( - bitrate_observer_.get(), &clock_)); + bitrate_observer_.get())); } protected: RTC_DISALLOW_COPY_AND_ASSIGN(RemoteBitrateEstimatorAbsSendTimeTest); diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc index 8bfb8ed0fd..f04d9e68c0 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc @@ -583,7 +583,7 @@ void RemoteBitrateEstimatorTest::TestWrappingHelper( absolute_send_time = AddAbsSendTime(absolute_send_time, AbsSendTime(silence_time_s, 1)); bitrate_estimator_->Process(); - for (size_t i = 0; i < 10; ++i) { + for (size_t i = 0; i < 21; ++i) { IncomingPacket(kDefaultSsrc, 1000, clock_.TimeInMilliseconds(), timestamp, absolute_send_time, true); timestamp += kFrameIntervalMs; diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc index d469e675e4..6f0c1250a4 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc @@ -69,7 +69,7 @@ RembReceiver::RembReceiver(int flow_id, bool plot) recv_stats_(ReceiveStatistics::Create(&clock_)), latest_estimate_bps_(-1), last_feedback_ms_(-1), - estimator_(new RemoteBitrateEstimatorAbsSendTime(this, &clock_)) { + estimator_(new RemoteBitrateEstimatorAbsSendTime(this)) { std::stringstream ss; ss << "Estimate_" << flow_id_ << "#1"; estimate_log_prefix_ = ss.str(); diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc index 36dff1fb2a..3cf7c752a0 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -23,7 +23,7 @@ const int kFeedbackIntervalMs = 50; FullBweSender::FullBweSender(int kbps, BitrateObserver* observer, Clock* clock) : bitrate_controller_( BitrateController::CreateBitrateController(clock, observer)), - rbe_(new RemoteBitrateEstimatorAbsSendTime(this, clock)), + rbe_(new RemoteBitrateEstimatorAbsSendTime(this)), feedback_observer_(bitrate_controller_->CreateRtcpBandwidthObserver()), clock_(clock), send_time_history_(clock_, 10000), diff --git a/webrtc/modules/remote_bitrate_estimator/tools/bwe_rtp.cc b/webrtc/modules/remote_bitrate_estimator/tools/bwe_rtp.cc index f138035de5..7ae6ede363 100644 --- a/webrtc/modules/remote_bitrate_estimator/tools/bwe_rtp.cc +++ b/webrtc/modules/remote_bitrate_estimator/tools/bwe_rtp.cc @@ -117,7 +117,7 @@ bool ParseArgsAndSetupEstimator(int argc, switch (extension) { case webrtc::kRtpExtensionAbsoluteSendTime: { *estimator = - new webrtc::RemoteBitrateEstimatorAbsSendTime(observer, clock); + new webrtc::RemoteBitrateEstimatorAbsSendTime(observer); *estimator_used = "AbsoluteSendTimeRemoteBitrateEstimator"; break; }