From a06e919b9ffb015d1e25905151e674152c4b3aea Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Wed, 7 Mar 2018 18:49:55 +0100 Subject: [PATCH] Removing interface to access pacer via SSCC. SSCC was accessing the pacer just to report values back to RtpTransportControllerSend which already owns the pacer. This CL moves those access methods. To make RtpTransportControllerSend simpler, Call is made responsible to keep track of network status used only as a condition for report the pacer queuing delay. Bug: webrtc:8415 Change-Id: I306bc9fcd3d8dcc7a637d51f2629ececebd48cad Reviewed-on: https://webrtc-review.googlesource.com/60483 Reviewed-by: Stefan Holmer Reviewed-by: Niels Moller Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#22331} --- call/call.cc | 29 +++++++++++++------ call/rtp_transport_controller_send.cc | 4 +-- .../include/send_side_congestion_controller.h | 4 +-- ...end_side_congestion_controller_interface.h | 2 -- .../include/send_side_congestion_controller.h | 2 -- .../rtp/send_side_congestion_controller.cc | 11 ------- ...end_side_congestion_controller_unittest.cc | 16 ---------- 7 files changed, 24 insertions(+), 44 deletions(-) diff --git a/call/call.cc b/call/call.cc index f6aa6d3248..a9e81c4bdc 100644 --- a/call/call.cc +++ b/call/call.cc @@ -268,6 +268,8 @@ class Call : public webrtc::Call, NetworkState audio_network_state_; NetworkState video_network_state_; + rtc::CriticalSection aggregate_network_up_crit_; + bool aggregate_network_up_ RTC_GUARDED_BY(aggregate_network_up_crit_); std::unique_ptr receive_crit_; // Audio, Video, and FlexFEC receive streams are owned by the client that @@ -413,6 +415,7 @@ Call::Call(const Call::Config& config, config_(config), audio_network_state_(kNetworkDown), video_network_state_(kNetworkDown), + aggregate_network_up_(false), receive_crit_(RWLockWrapper::CreateRWLock()), send_crit_(RWLockWrapper::CreateRWLock()), event_log_(config.event_log), @@ -909,7 +912,14 @@ Call::Stats Call::GetStats() const { &ssrcs, &recv_bandwidth); stats.send_bandwidth_bps = send_bandwidth; stats.recv_bandwidth_bps = recv_bandwidth; - stats.pacer_delay_ms = transport_send_->GetPacerQueuingDelayMs(); + // TODO(srte): It is unclear if we only want to report queues if network is + // available. + { + rtc::CritScope cs(&aggregate_network_up_crit_); + stats.pacer_delay_ms = + aggregate_network_up_ ? transport_send_->GetPacerQueuingDelayMs() : 0; + } + stats.rtt_ms = call_stats_->rtcp_rtt_stats()->LastProcessedRtt(); { rtc::CritScope cs(&bitrate_crit_); @@ -1016,16 +1026,17 @@ void Call::UpdateAggregateNetworkState() { have_video = true; } - NetworkState aggregate_state = kNetworkDown; - if ((have_video && video_network_state_ == kNetworkUp) || - (have_audio && audio_network_state_ == kNetworkUp)) { - aggregate_state = kNetworkUp; - } + bool aggregate_network_up = + ((have_video && video_network_state_ == kNetworkUp) || + (have_audio && audio_network_state_ == kNetworkUp)); RTC_LOG(LS_INFO) << "UpdateAggregateNetworkState: aggregate_state=" - << (aggregate_state == kNetworkUp ? "up" : "down"); - - transport_send_->OnNetworkAvailability(aggregate_state == kNetworkUp); + << (aggregate_network_up ? "up" : "down"); + { + rtc::CritScope cs(&aggregate_network_up_crit_); + aggregate_network_up_ = aggregate_network_up; + } + transport_send_->OnNetworkAvailability(aggregate_network_up); } void Call::OnSentPacket(const rtc::SentPacket& sent_packet) { diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 5554127923..80df7654ea 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -145,10 +145,10 @@ bool RtpTransportControllerSend::AvailableBandwidth(uint32_t* bandwidth) const { return send_side_cc_->AvailableBandwidth(bandwidth); } int64_t RtpTransportControllerSend::GetPacerQueuingDelayMs() const { - return send_side_cc_->GetPacerQueuingDelayMs(); + return pacer_.QueueInMs(); } int64_t RtpTransportControllerSend::GetFirstPacketTimeMs() const { - return send_side_cc_->GetFirstPacketTimeMs(); + return pacer_.FirstSentPacketTimeMs(); } void RtpTransportControllerSend::EnablePeriodicAlrProbing(bool enable) { send_side_cc_->EnablePeriodicAlrProbing(enable); diff --git a/modules/congestion_controller/include/send_side_congestion_controller.h b/modules/congestion_controller/include/send_side_congestion_controller.h index 1f579f8c45..7093da4b3d 100644 --- a/modules/congestion_controller/include/send_side_congestion_controller.h +++ b/modules/congestion_controller/include/send_side_congestion_controller.h @@ -88,8 +88,8 @@ class SendSideCongestionController RTC_DEPRECATED RtcpBandwidthObserver* GetBandwidthObserver() const; bool AvailableBandwidth(uint32_t* bandwidth) const override; - int64_t GetPacerQueuingDelayMs() const override; - int64_t GetFirstPacketTimeMs() const override; + virtual int64_t GetPacerQueuingDelayMs() const; + virtual int64_t GetFirstPacketTimeMs() const; TransportFeedbackObserver* GetTransportFeedbackObserver() override; diff --git a/modules/congestion_controller/include/send_side_congestion_controller_interface.h b/modules/congestion_controller/include/send_side_congestion_controller_interface.h index 0ff3f96277..6ea0d06c7d 100644 --- a/modules/congestion_controller/include/send_side_congestion_controller_interface.h +++ b/modules/congestion_controller/include/send_side_congestion_controller_interface.h @@ -59,8 +59,6 @@ class SendSideCongestionControllerInterface : public CallStatsObserver, size_t transport_overhead_bytes_per_packet) = 0; virtual RtcpBandwidthObserver* GetBandwidthObserver() = 0; virtual bool AvailableBandwidth(uint32_t* bandwidth) const = 0; - virtual int64_t GetPacerQueuingDelayMs() const = 0; - virtual int64_t GetFirstPacketTimeMs() const = 0; virtual TransportFeedbackObserver* GetTransportFeedbackObserver() = 0; virtual void EnablePeriodicAlrProbing(bool enable) = 0; virtual void OnSentPacket(const rtc::SentPacket& sent_packet) = 0; diff --git a/modules/congestion_controller/rtp/include/send_side_congestion_controller.h b/modules/congestion_controller/rtp/include/send_side_congestion_controller.h index b69f5c0a82..e3f0c60ec2 100644 --- a/modules/congestion_controller/rtp/include/send_side_congestion_controller.h +++ b/modules/congestion_controller/rtp/include/send_side_congestion_controller.h @@ -95,8 +95,6 @@ class SendSideCongestionController RtcpBandwidthObserver* GetBandwidthObserver() override; bool AvailableBandwidth(uint32_t* bandwidth) const override; - int64_t GetPacerQueuingDelayMs() const override; - int64_t GetFirstPacketTimeMs() const override; TransportFeedbackObserver* GetTransportFeedbackObserver() override; diff --git a/modules/congestion_controller/rtp/send_side_congestion_controller.cc b/modules/congestion_controller/rtp/send_side_congestion_controller.cc index 844d2b1895..64b8d43c61 100644 --- a/modules/congestion_controller/rtp/send_side_congestion_controller.cc +++ b/modules/congestion_controller/rtp/send_side_congestion_controller.cc @@ -398,17 +398,6 @@ void SendSideCongestionController::UpdateStreamsConfig() { controller_->OnStreamsConfig(streams_config_); } -int64_t SendSideCongestionController::GetPacerQueuingDelayMs() const { - // TODO(srte): This should be made less synchronous. Now it grabs a lock in - // the pacer just for stats usage. Some kind of push interface might make - // sense. - return network_available_ ? pacer_->QueueInMs() : 0; -} - -int64_t SendSideCongestionController::GetFirstPacketTimeMs() const { - return pacer_->FirstSentPacketTimeMs(); -} - TransportFeedbackObserver* SendSideCongestionController::GetTransportFeedbackObserver() { return this; diff --git a/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc b/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc index 579c645088..fa5d6ab642 100644 --- a/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc +++ b/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc @@ -331,22 +331,6 @@ TEST_F(SendSideCongestionControllerTest, controller_->Process(); } -TEST_F(SendSideCongestionControllerTest, GetPacerQueuingDelayMs) { - EXPECT_CALL(observer_, OnNetworkChanged(_, _, _, _)).Times(AtLeast(1)); - - const int64_t kQueueTimeMs = 123; - EXPECT_CALL(*pacer_, QueueInMs()).WillRepeatedly(Return(kQueueTimeMs)); - EXPECT_EQ(kQueueTimeMs, controller_->GetPacerQueuingDelayMs()); - - // Expect zero pacer delay when network is down. - controller_->SignalNetworkState(kNetworkDown); - EXPECT_EQ(0, controller_->GetPacerQueuingDelayMs()); - - // Network is up, pacer delay should be reported. - controller_->SignalNetworkState(kNetworkUp); - EXPECT_EQ(kQueueTimeMs, controller_->GetPacerQueuingDelayMs()); -} - TEST_F(SendSideCongestionControllerTest, GetProbingInterval) { clock_.AdvanceTimeMilliseconds(25); controller_->Process();