From b93598465d85a7df2f601058260eb2edc26b93ab Mon Sep 17 00:00:00 2001 From: nisse Date: Thu, 19 Jan 2017 05:41:25 -0800 Subject: [PATCH] Revert of Move congestion controller processing to the pacer thread. (patchset #5 id:80001 of https://codereview.webrtc.org/2637783003/ ) Reason for revert: Speculative revert for perf regression related to ramp-up on android. See https://bugs.chromium.org/p/chromium/issues/detail?id=682611 Original issue's description: > Move congestion controller processing to the pacer thread. > > Also rename it from pacer_thread_ to congestion_controller_thread_. > > BUG=webrtc:6847 > > Review-Url: https://codereview.webrtc.org/2637783003 > Cr-Commit-Position: refs/heads/master@{#16134} > Committed: https://chromium.googlesource.com/external/webrtc/+/b3dc2b7b1e02970c49064a61a4bd0fc941249c93 TBR=danilchap@webrtc.org,stefan@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:6847 Review-Url: https://codereview.webrtc.org/2644603003 Cr-Commit-Position: refs/heads/master@{#16163} --- webrtc/call/call.cc | 19 ++++++++++++------- .../congestion_controller.cc | 10 +++------- webrtc/modules/pacing/alr_detector.cc | 4 ---- webrtc/modules/pacing/paced_sender.cc | 8 +++----- 4 files changed, 18 insertions(+), 23 deletions(-) diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index cd31b028bd..48072d0a0b 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -173,7 +173,7 @@ class Call : public webrtc::Call, const int num_cpu_cores_; const std::unique_ptr module_process_thread_; - const std::unique_ptr congestion_controller_thread_; + const std::unique_ptr pacer_thread_; const std::unique_ptr call_stats_; const std::unique_ptr bitrate_allocator_; Call::Config config_; @@ -277,8 +277,7 @@ Call::Call(const Call::Config& config) : clock_(Clock::GetRealTimeClock()), num_cpu_cores_(CpuInfo::DetectNumberOfCores()), module_process_thread_(ProcessThread::Create("ModuleProcessThread")), - congestion_controller_thread_( - ProcessThread::Create("CongestionControllerThread")), + pacer_thread_(ProcessThread::Create("PacerThread")), call_stats_(new CallStats(clock_)), bitrate_allocator_(new BitrateAllocator(this)), config_(config), @@ -325,8 +324,11 @@ Call::Call(const Call::Config& config) module_process_thread_->Start(); module_process_thread_->RegisterModule(call_stats_.get()); - congestion_controller_thread_->RegisterModule(congestion_controller_.get()); - congestion_controller_thread_->Start(); + module_process_thread_->RegisterModule(congestion_controller_.get()); + pacer_thread_->RegisterModule(congestion_controller_->pacer()); + pacer_thread_->RegisterModule( + congestion_controller_->GetRemoteBitrateEstimator(true)); + pacer_thread_->Start(); } Call::~Call() { @@ -340,8 +342,11 @@ Call::~Call() { RTC_CHECK(video_receive_ssrcs_.empty()); RTC_CHECK(video_receive_streams_.empty()); - congestion_controller_thread_->Stop(); - congestion_controller_thread_->DeRegisterModule(congestion_controller_.get()); + pacer_thread_->Stop(); + pacer_thread_->DeRegisterModule(congestion_controller_->pacer()); + pacer_thread_->DeRegisterModule( + congestion_controller_->GetRemoteBitrateEstimator(true)); + module_process_thread_->DeRegisterModule(congestion_controller_.get()); module_process_thread_->DeRegisterModule(call_stats_.get()); module_process_thread_->Stop(); call_stats_->DeregisterStatsObserver(congestion_controller_.get()); diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index cbe3f2fe8a..94b3d8c6da 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -327,19 +327,15 @@ void CongestionController::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { } int64_t CongestionController::TimeUntilNextProcess() { - return std::min({bitrate_controller_->TimeUntilNextProcess(), - remote_bitrate_estimator_->TimeUntilNextProcess(), - pacer_->TimeUntilNextProcess(), - remote_estimator_proxy_.TimeUntilNextProcess()}); + return std::min(bitrate_controller_->TimeUntilNextProcess(), + remote_bitrate_estimator_->TimeUntilNextProcess()); } void CongestionController::Process() { bitrate_controller_->Process(); remote_bitrate_estimator_->Process(); - MaybeTriggerOnNetworkChanged(); probe_controller_->Process(); - pacer_->Process(); - remote_estimator_proxy_.Process(); + MaybeTriggerOnNetworkChanged(); } void CongestionController::MaybeTriggerOnNetworkChanged() { diff --git a/webrtc/modules/pacing/alr_detector.cc b/webrtc/modules/pacing/alr_detector.cc index 9a917983be..b31d4482bb 100644 --- a/webrtc/modules/pacing/alr_detector.cc +++ b/webrtc/modules/pacing/alr_detector.cc @@ -36,10 +36,6 @@ AlrDetector::AlrDetector() AlrDetector::~AlrDetector() {} void AlrDetector::OnBytesSent(size_t bytes_sent, int64_t now_ms) { - // TODO(nisse): It's unclear what guarantees there are that this - // function isn't called before SetEstimatedBitrate. Document that - // guarantee, if it exists. Or else, remove the DCHECK and tolerate - // that current estimate is zero, e.g., by just returning false. RTC_DCHECK(estimated_bitrate_bps_); rate_.Update(bytes_sent, now_ms); diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index 55a060cbc3..a0d7eda73f 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -450,11 +450,9 @@ void PacedSender::Process() { bytes_sent += SendPadding(padding_needed, probe_cluster_id); } } - if (bytes_sent > 0) { - if (is_probing) - prober_->ProbeSent(clock_->TimeInMilliseconds(), bytes_sent); - alr_detector_->OnBytesSent(bytes_sent, now_us / 1000); - } + if (is_probing && bytes_sent > 0) + prober_->ProbeSent(clock_->TimeInMilliseconds(), bytes_sent); + alr_detector_->OnBytesSent(bytes_sent, now_us / 1000); } bool PacedSender::SendPacket(const paced_sender::Packet& packet,