diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 157258291a..ab3a90c2ec 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -151,7 +151,10 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - send_queue_is_full_(false) { + last_reported_bitrate_bps_(0), + last_reported_fraction_loss_(0), + last_reported_rtt_(0), + network_state_(kNetworkUp) { Init(); } @@ -170,7 +173,10 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - send_queue_is_full_(false) { + last_reported_bitrate_bps_(0), + last_reported_fraction_loss_(0), + last_reported_rtt_(0), + network_state_(kNetworkUp) { Init(); } @@ -192,7 +198,10 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - send_queue_is_full_(false) { + last_reported_bitrate_bps_(0), + last_reported_fraction_loss_(0), + last_reported_rtt_(0), + network_state_(kNetworkUp) { Init(); } @@ -265,6 +274,11 @@ void CongestionController::SignalNetworkState(NetworkState state) { } else { pacer_->Pause(); } + { + rtc::CritScope cs(&critsect_); + network_state_ = state; + } + MaybeTriggerOnNetworkChanged(); } void CongestionController::OnSentPacket(const rtc::SentPacket& sent_packet) { @@ -297,24 +311,40 @@ void CongestionController::MaybeTriggerOnNetworkChanged() { uint32_t bitrate_bps; uint8_t fraction_loss; int64_t rtt; - bool network_changed = bitrate_controller_->GetNetworkParameters( + bool estimate_changed = bitrate_controller_->GetNetworkParameters( &bitrate_bps, &fraction_loss, &rtt); - if (network_changed) + if (estimate_changed) pacer_->SetEstimatedBitrate(bitrate_bps); - bool send_queue_is_full = - pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs; - bitrate_bps = send_queue_is_full ? 0 : bitrate_bps; - if ((network_changed && !send_queue_is_full) || - UpdateSendQueueStatus(send_queue_is_full)) { + + bitrate_bps = IsNetworkDown() || IsSendQueueFull() ? 0 : bitrate_bps; + + if (HasNetworkParametersToReportChanged(bitrate_bps, fraction_loss, rtt)) { observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt); } } -bool CongestionController::UpdateSendQueueStatus(bool send_queue_is_full) { +bool CongestionController::HasNetworkParametersToReportChanged( + uint32_t bitrate_bps, + uint8_t fraction_loss, + int64_t rtt) { rtc::CritScope cs(&critsect_); - bool result = send_queue_is_full_ != send_queue_is_full; - send_queue_is_full_ = send_queue_is_full; - return result; + bool changed = + last_reported_bitrate_bps_ != bitrate_bps || + (bitrate_bps > 0 && (last_reported_fraction_loss_ != fraction_loss || + last_reported_rtt_ != rtt)); + last_reported_bitrate_bps_ = bitrate_bps; + last_reported_fraction_loss_ = fraction_loss; + last_reported_rtt_ = rtt; + return changed; +} + +bool CongestionController::IsSendQueueFull() const { + return pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs; +} + +bool CongestionController::IsNetworkDown() const { + rtc::CritScope cs(&critsect_); + return network_state_ == kNetworkDown; } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc index 20bf1a88e6..c82c75daf3 100644 --- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -113,5 +113,45 @@ TEST_F(CongestionControllerTest, OnSendQueueFullAndEstimateChange) { controller_->Process(); } +TEST_F(CongestionControllerTest, SignalNetworkState) { + EXPECT_CALL(observer_, OnNetworkChanged(0, _, _)); + controller_->SignalNetworkState(kNetworkDown); + + EXPECT_CALL(observer_, OnNetworkChanged(kInitialBitrateBps, _, _)); + controller_->SignalNetworkState(kNetworkUp); + + EXPECT_CALL(observer_, OnNetworkChanged(0, _, _)); + controller_->SignalNetworkState(kNetworkDown); +} + +TEST_F(CongestionControllerTest, + SignalNetworkStateAndQueueIsFullAndEstimateChange) { + // Send queue is full + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillRepeatedly(Return(PacedSender::kMaxQueueLengthMs + 1)); + EXPECT_CALL(observer_, OnNetworkChanged(0, _, _)); + controller_->Process(); + + // Queue is full and network is down. Expect no bitrate change. + controller_->SignalNetworkState(kNetworkDown); + controller_->Process(); + + // Queue is full but network is up. Expect no bitrate change. + controller_->SignalNetworkState(kNetworkUp); + controller_->Process(); + + // Receive new estimate but let the queue still be full. + EXPECT_CALL(*pacer_, SetEstimatedBitrate(kInitialBitrateBps * 2)); + bandwidth_observer_->OnReceivedEstimatedBitrate(kInitialBitrateBps * 2); + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); + + // Let the pacer not be full next time the controller checks. + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs - 1)); + EXPECT_CALL(observer_, OnNetworkChanged(kInitialBitrateBps * 2, _, _)); + controller_->Process(); +} + } // namespace test } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index a3b672e3bc..84c18a4ae1 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -95,10 +95,12 @@ class CongestionController : public CallStatsObserver, public Module { private: void Init(); void MaybeTriggerOnNetworkChanged(); - // Updates |send_queue_is_full_|. Returns true if |send_queue_is_full_| - // has changed. - bool UpdateSendQueueStatus(bool send_queue_is_full); + bool IsSendQueueFull() const; + bool IsNetworkDown() const; + bool HasNetworkParametersToReportChanged(uint32_t bitrate_bps, + uint8_t fraction_loss, + int64_t rtt); Clock* const clock_; Observer* const observer_; const std::unique_ptr packet_router_; @@ -109,7 +111,10 @@ class CongestionController : public CallStatsObserver, public Module { TransportFeedbackAdapter transport_feedback_adapter_; int min_bitrate_bps_; rtc::CriticalSection critsect_; - bool send_queue_is_full_ GUARDED_BY(critsect_); + uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_); + uint8_t last_reported_fraction_loss_ GUARDED_BY(critsect_); + int64_t last_reported_rtt_ GUARDED_BY(critsect_); + NetworkState network_state_ GUARDED_BY(critsect_); RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(CongestionController); }; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 840991b4ba..f673a98a3e 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -491,21 +491,6 @@ VideoSendStream::~VideoSendStream() { } } -void VideoSendStream::SignalNetworkState(NetworkState state) { - // When network goes up, enable RTCP status before setting transmission state. - // When it goes down, disable RTCP afterwards. This ensures that any packets - // sent due to the network state changed will not be dropped. - if (state == kNetworkUp) { - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) - rtp_rtcp->SetRTCPStatus(config_.rtp.rtcp_mode); - } - vie_encoder_.SetNetworkTransmissionState(state == kNetworkUp); - if (state == kNetworkDown) { - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) - rtp_rtcp->SetRTCPStatus(RtcpMode::kOff); - } -} - bool VideoSendStream::DeliverRtcp(const uint8_t* packet, size_t length) { for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) rtp_rtcp->IncomingRtcpPacket(packet, length); @@ -779,6 +764,13 @@ std::map VideoSendStream::GetRtpStates() const { return rtp_states; } +void VideoSendStream::SignalNetworkState(NetworkState state) { + for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { + rtp_rtcp->SetRTCPStatus(state == kNetworkUp ? config_.rtp.rtcp_mode + : RtcpMode::kOff); + } +} + int VideoSendStream::GetPaddingNeededBps() const { return vie_encoder_.GetPaddingNeededBps(); } diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc index 2ed71106bd..2871c4045c 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc @@ -43,7 +43,6 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores, encoder_config_(), min_transmit_bitrate_bps_(0), last_observed_bitrate_bps_(0), - network_is_transmitting_(true), encoder_paused_(true), encoder_paused_and_dropped_frame_(false), module_process_thread_(module_process_thread), @@ -64,13 +63,6 @@ ViEEncoder::~ViEEncoder() { module_process_thread_->DeRegisterModule(&video_sender_); } -void ViEEncoder::SetNetworkTransmissionState(bool is_transmitting) { - { - rtc::CritScope lock(&data_cs_); - network_is_transmitting_ = is_transmitting; - } -} - void ViEEncoder::Pause() { rtc::CritScope lock(&data_cs_); encoder_paused_ = true; @@ -199,13 +191,9 @@ int ViEEncoder::GetPaddingNeededBps() const { bool ViEEncoder::EncoderPaused() const { // Pause video if paused by caller or as long as the network is down or the // pacer queue has grown too large in buffered mode. - if (encoder_paused_) { - return true; - } - if (video_suspended_ || last_observed_bitrate_bps_ == 0) { - return true; - } - return !network_is_transmitting_; + // If the pacer queue has grown to large or the network is down, + // last_observed_bitrate_bps_ will be 0. + return encoder_paused_ || video_suspended_ || last_observed_bitrate_bps_ == 0; } void ViEEncoder::TraceFrameDropStart() { diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h index f846bdf2f2..556ed44995 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h @@ -63,8 +63,6 @@ class ViEEncoder : public VideoEncoderRateObserver, vcm::VideoSender* video_sender(); - void SetNetworkTransmissionState(bool is_transmitting); - // Returns the id of the owning channel. int Owner() const; @@ -137,7 +135,6 @@ class ViEEncoder : public VideoEncoderRateObserver, VideoCodec encoder_config_ GUARDED_BY(data_cs_); int min_transmit_bitrate_bps_ GUARDED_BY(data_cs_); uint32_t last_observed_bitrate_bps_ GUARDED_BY(data_cs_); - bool network_is_transmitting_ GUARDED_BY(data_cs_); bool encoder_paused_ GUARDED_BY(data_cs_); bool encoder_paused_and_dropped_frame_ GUARDED_BY(data_cs_);