From 7339c500fe5dec689e69a6cfc745f2508cbcdedf Mon Sep 17 00:00:00 2001 From: perkj Date: Fri, 13 May 2016 01:17:30 -0700 Subject: [PATCH] Revert of Remove ViEEncoder::SetNetworkStatus (patchset #11 id:200001 of https://codereview.webrtc.org/1932683002/ ) Reason for revert: Breaks Chrome FYI using H264. Need to investigate. https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/builds/4170 Original issue's description: > Remove ViEEncoder::SetNetworkStatus > > This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will report that frames can not be sent when the network is down and the BitrateController will report an estimated available bandwidth of 0 bps. > > BUG=webrtc:5687 > NOTRY=True > > Committed: https://crrev.com/50b5c3be844ef571a28b2681c549443a26735d72 > Cr-Commit-Position: refs/heads/master@{#12699} TBR=stefan@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5687 Review-Url: https://codereview.webrtc.org/1978783002 Cr-Commit-Position: refs/heads/master@{#12715} --- .../congestion_controller.cc | 58 +++++-------------- .../congestion_controller_unittest.cc | 40 ------------- .../include/congestion_controller.h | 13 ++--- webrtc/video/video_send_stream.cc | 22 ++++--- webrtc/video/vie_encoder.cc | 18 +++++- webrtc/video/vie_encoder.h | 3 + 6 files changed, 51 insertions(+), 103 deletions(-) diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index ab3a90c2ec..157258291a 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -151,10 +151,7 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - last_reported_bitrate_bps_(0), - last_reported_fraction_loss_(0), - last_reported_rtt_(0), - network_state_(kNetworkUp) { + send_queue_is_full_(false) { Init(); } @@ -173,10 +170,7 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - last_reported_bitrate_bps_(0), - last_reported_fraction_loss_(0), - last_reported_rtt_(0), - network_state_(kNetworkUp) { + send_queue_is_full_(false) { Init(); } @@ -198,10 +192,7 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), - last_reported_bitrate_bps_(0), - last_reported_fraction_loss_(0), - last_reported_rtt_(0), - network_state_(kNetworkUp) { + send_queue_is_full_(false) { Init(); } @@ -274,11 +265,6 @@ void CongestionController::SignalNetworkState(NetworkState state) { } else { pacer_->Pause(); } - { - rtc::CritScope cs(&critsect_); - network_state_ = state; - } - MaybeTriggerOnNetworkChanged(); } void CongestionController::OnSentPacket(const rtc::SentPacket& sent_packet) { @@ -311,40 +297,24 @@ void CongestionController::MaybeTriggerOnNetworkChanged() { uint32_t bitrate_bps; uint8_t fraction_loss; int64_t rtt; - bool estimate_changed = bitrate_controller_->GetNetworkParameters( + bool network_changed = bitrate_controller_->GetNetworkParameters( &bitrate_bps, &fraction_loss, &rtt); - if (estimate_changed) + if (network_changed) pacer_->SetEstimatedBitrate(bitrate_bps); - - bitrate_bps = IsNetworkDown() || IsSendQueueFull() ? 0 : bitrate_bps; - - if (HasNetworkParametersToReportChanged(bitrate_bps, fraction_loss, rtt)) { + 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)) { observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt); } } -bool CongestionController::HasNetworkParametersToReportChanged( - uint32_t bitrate_bps, - uint8_t fraction_loss, - int64_t rtt) { +bool CongestionController::UpdateSendQueueStatus(bool send_queue_is_full) { rtc::CritScope cs(&critsect_); - 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; + bool result = send_queue_is_full_ != send_queue_is_full; + send_queue_is_full_ = send_queue_is_full; + return result; } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc index c82c75daf3..20bf1a88e6 100644 --- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -113,45 +113,5 @@ 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 84c18a4ae1..a3b672e3bc 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -95,12 +95,10 @@ 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_; @@ -111,10 +109,7 @@ class CongestionController : public CallStatsObserver, public Module { TransportFeedbackAdapter transport_feedback_adapter_; int min_bitrate_bps_; rtc::CriticalSection 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_); + bool send_queue_is_full_ 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 f673a98a3e..840991b4ba 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -491,6 +491,21 @@ 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); @@ -764,13 +779,6 @@ 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 2871c4045c..2ed71106bd 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc @@ -43,6 +43,7 @@ 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), @@ -63,6 +64,13 @@ 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; @@ -191,9 +199,13 @@ 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 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; + if (encoder_paused_) { + return true; + } + if (video_suspended_ || last_observed_bitrate_bps_ == 0) { + return true; + } + return !network_is_transmitting_; } void ViEEncoder::TraceFrameDropStart() { diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h index 556ed44995..f846bdf2f2 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h @@ -63,6 +63,8 @@ class ViEEncoder : public VideoEncoderRateObserver, vcm::VideoSender* video_sender(); + void SetNetworkTransmissionState(bool is_transmitting); + // Returns the id of the owning channel. int Owner() const; @@ -135,6 +137,7 @@ 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_);