From e2b1501101802ad8f4b7e38cad673a19bc2ff849 Mon Sep 17 00:00:00 2001 From: Sergey Ulanov Date: Tue, 22 Nov 2016 16:08:30 -0800 Subject: [PATCH] Start probes only after network is connected. Previously ProbeController was starting probing as soon as SetBitrates() is called. As result these probes would often timeout while connection is being established. Now ProbeController receives notifications about network route changes. This allows to start probing only when transport is connected. This also makes it possible to restart probing whenever transport route changes (will be done in a separate change). BUG=webrtc:6332 R=honghaiz@webrtc.org, philipel@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/2458863002 . Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e Cr-Original-Commit-Position: refs/heads/master@{#15094} Cr-Commit-Position: refs/heads/master@{#15204} --- webrtc/call/bitrate_allocator.cc | 2 +- webrtc/call/call.cc | 5 +- webrtc/media/base/videoengine_unittest.h | 1 + .../engine/webrtcvideoengine2_unittest.cc | 8 +++ .../congestion_controller.cc | 1 + .../congestion_controller/probe_controller.cc | 58 ++++++++++++++----- .../congestion_controller/probe_controller.h | 14 ++++- .../probe_controller_unittest.cc | 11 ++++ webrtc/test/call_test.cc | 4 ++ webrtc/test/direct_transport.cc | 4 ++ webrtc/video/end_to_end_tests.cc | 21 ++++--- webrtc/video/video_send_stream_tests.cc | 2 + 12 files changed, 98 insertions(+), 33 deletions(-) diff --git a/webrtc/call/bitrate_allocator.cc b/webrtc/call/bitrate_allocator.cc index 645ee3c41c..2c5ba6d76c 100644 --- a/webrtc/call/bitrate_allocator.cc +++ b/webrtc/call/bitrate_allocator.cc @@ -48,7 +48,7 @@ double MediaRatio(uint32_t allocated_bitrate, uint32_t protection_bitrate) { BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer) : limit_observer_(limit_observer), bitrate_observer_configs_(), - last_bitrate_bps_(kDefaultBitrateBps), + last_bitrate_bps_(0), last_non_zero_bitrate_bps_(kDefaultBitrateBps), last_fraction_loss_(0), last_rtt_(0), diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index baa89366c7..d2bc153d32 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -252,8 +252,8 @@ Call::Call(const Call::Config& config) call_stats_(new CallStats(clock_)), bitrate_allocator_(new BitrateAllocator(this)), config_(config), - audio_network_state_(kNetworkUp), - video_network_state_(kNetworkUp), + audio_network_state_(kNetworkDown), + video_network_state_(kNetworkDown), receive_crit_(RWLockWrapper::CreateRWLock()), send_crit_(RWLockWrapper::CreateRWLock()), event_log_(config.event_log), @@ -284,6 +284,7 @@ Call::Call(const Call::Config& config) Trace::CreateTrace(); call_stats_->RegisterStatsObserver(congestion_controller_.get()); + congestion_controller_->SignalNetworkState(kNetworkDown); congestion_controller_->SetBweBitrates( config_.bitrate_config.min_bitrate_bps, config_.bitrate_config.start_bitrate_bps, diff --git a/webrtc/media/base/videoengine_unittest.h b/webrtc/media/base/videoengine_unittest.h index 024ae5d22e..e307d9f703 100644 --- a/webrtc/media/base/videoengine_unittest.h +++ b/webrtc/media/base/videoengine_unittest.h @@ -84,6 +84,7 @@ class VideoMediaChannelTest : public testing::Test, engine_.Init(); channel_.reset(engine_.CreateChannel(call_.get(), cricket::MediaConfig(), cricket::VideoOptions())); + channel_->OnReadyToSend(true); EXPECT_TRUE(channel_.get() != NULL); network_interface_.SetDestination(channel_.get()); channel_->SetInterface(&network_interface_); diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index c473a4099f..ea384e0222 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -353,6 +353,7 @@ TEST_F(WebRtcVideoEngine2Test, UseExternalFactoryForVp8WhenSupported) { std::unique_ptr channel( SetUpForExternalEncoderFactory(&encoder_factory)); + channel->OnReadyToSend(true); EXPECT_TRUE( channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc))); @@ -916,6 +917,7 @@ class WebRtcVideoChannel2Test : public WebRtcVideoEngine2Test { engine_.Init(); channel_.reset( engine_.CreateChannel(fake_call_.get(), MediaConfig(), VideoOptions())); + channel_->OnReadyToSend(true); last_ssrc_ = 123; send_parameters_.codecs = engine_.codecs(); recv_parameters_.codecs = engine_.codecs(); @@ -1731,6 +1733,7 @@ TEST_F(WebRtcVideoChannel2Test, SetMediaConfigSuspendBelowMinBitrate) { channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); + channel_->OnReadyToSend(true); channel_->SetSendParameters(send_parameters_); @@ -1740,6 +1743,7 @@ TEST_F(WebRtcVideoChannel2Test, SetMediaConfigSuspendBelowMinBitrate) { media_config.video.suspend_below_min_bitrate = false; channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); + channel_->OnReadyToSend(true); channel_->SetSendParameters(send_parameters_); @@ -2024,6 +2028,7 @@ TEST_F(WebRtcVideoChannel2Test, AdaptsOnOveruseAndChangeResolution) { MediaConfig media_config = MediaConfig(); channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); + channel_->OnReadyToSend(true); ASSERT_TRUE(channel_->SetSendParameters(parameters)); AddSendStream(); @@ -2098,6 +2103,7 @@ TEST_F(WebRtcVideoChannel2Test, PreviousAdaptationDoesNotApplyToScreenshare) { MediaConfig media_config = MediaConfig(); channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); + channel_->OnReadyToSend(true); ASSERT_TRUE(channel_->SetSendParameters(parameters)); AddSendStream(); @@ -2165,6 +2171,7 @@ void WebRtcVideoChannel2Test::TestCpuAdaptation(bool enable_overuse, } channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); + channel_->OnReadyToSend(true); EXPECT_TRUE(channel_->SetSendParameters(parameters)); @@ -3865,6 +3872,7 @@ class WebRtcVideoChannel2SimulcastTest : public testing::Test { engine_.Init(); channel_.reset( engine_.CreateChannel(&fake_call_, MediaConfig(), VideoOptions())); + channel_->OnReadyToSend(true); last_ssrc_ = 123; } diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 0dafdf65fd..accc1411f9 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -297,6 +297,7 @@ void CongestionController::SignalNetworkState(NetworkState state) { rtc::CritScope cs(&critsect_); network_state_ = state; } + probe_controller_->OnNetworkStateChanged(state); MaybeTriggerOnNetworkChanged(); } diff --git a/webrtc/modules/congestion_controller/probe_controller.cc b/webrtc/modules/congestion_controller/probe_controller.cc index 7e1d936a1e..1749e29ff7 100644 --- a/webrtc/modules/congestion_controller/probe_controller.cc +++ b/webrtc/modules/congestion_controller/probe_controller.cc @@ -46,10 +46,12 @@ constexpr int kAlrProbingIntervalLimitMs = 5000; ProbeController::ProbeController(PacedSender* pacer, Clock* clock) : pacer_(pacer), clock_(clock), + network_state_(kNetworkUp), state_(State::kInit), min_bitrate_to_probe_further_bps_(kExponentialProbingDisabled), time_last_probing_initiated_ms_(0), estimated_bitrate_bps_(0), + start_bitrate_bps_(0), max_bitrate_bps_(0), last_alr_probing_time_(clock_->TimeInMilliseconds()) {} @@ -57,30 +59,54 @@ void ProbeController::SetBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps) { rtc::CritScope cs(&critsect_); - if (state_ == State::kInit) { - // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of - // 1.2 Mbps to continue probing. - InitiateProbing({3 * start_bitrate_bps, 6 * start_bitrate_bps}, - 4 * start_bitrate_bps); + + if (start_bitrate_bps > 0) { + start_bitrate_bps_ = start_bitrate_bps; + } else if (start_bitrate_bps_ == 0) { + start_bitrate_bps_ = min_bitrate_bps; } int old_max_bitrate_bps = max_bitrate_bps_; max_bitrate_bps_ = max_bitrate_bps; - // Only do probing if: - // we are mid-call, which we consider to be if - // exponential probing is not active and - // |estimated_bitrate_bps_| is valid (> 0) and - // the current bitrate is lower than the new |max_bitrate_bps|, and - // |max_bitrate_bps_| was increased. - if (state_ != State::kWaitingForProbingResult && - estimated_bitrate_bps_ != 0 && - estimated_bitrate_bps_ < old_max_bitrate_bps && - max_bitrate_bps_ > old_max_bitrate_bps) { - InitiateProbing({max_bitrate_bps}, kExponentialProbingDisabled); + switch (state_) { + case State::kInit: + if (network_state_ == kNetworkUp) + InitiateExponentialProbing(); + break; + + case State::kWaitingForProbingResult: + break; + + case State::kProbingComplete: + // Initiate probing when |max_bitrate_| was increased mid-call. + if (estimated_bitrate_bps_ != 0 && + estimated_bitrate_bps_ < old_max_bitrate_bps && + max_bitrate_bps_ > old_max_bitrate_bps) { + InitiateProbing({max_bitrate_bps}, kExponentialProbingDisabled); + } + break; } } +void ProbeController::OnNetworkStateChanged(NetworkState network_state) { + rtc::CritScope cs(&critsect_); + network_state_ = network_state; + if (network_state_ == kNetworkUp && state_ == State::kInit) + InitiateExponentialProbing(); +} + +void ProbeController::InitiateExponentialProbing() { + RTC_DCHECK(network_state_ == kNetworkUp); + RTC_DCHECK(state_ == State::kInit); + RTC_DCHECK_GT(start_bitrate_bps_, 0); + + // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of + // 1.2 Mbps to continue probing. + InitiateProbing({3 * start_bitrate_bps_, 6 * start_bitrate_bps_}, + 4 * start_bitrate_bps_); +} + void ProbeController::SetEstimatedBitrate(int bitrate_bps) { rtc::CritScope cs(&critsect_); if (state_ == State::kWaitingForProbingResult) { diff --git a/webrtc/modules/congestion_controller/probe_controller.h b/webrtc/modules/congestion_controller/probe_controller.h index efcc2a1d4a..e60a007b9d 100644 --- a/webrtc/modules/congestion_controller/probe_controller.h +++ b/webrtc/modules/congestion_controller/probe_controller.h @@ -31,12 +31,12 @@ class ProbeController { void SetBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps); + + void OnNetworkStateChanged(NetworkState state); + void SetEstimatedBitrate(int bitrate_bps); private: - void InitiateProbing(std::initializer_list bitrates_to_probe, - int min_bitrate_to_probe_further_bps) - EXCLUSIVE_LOCKS_REQUIRED(critsect_); enum class State { // Initial state where no probing has been triggered yet. kInit, @@ -45,13 +45,21 @@ class ProbeController { // Probing is complete. kProbingComplete, }; + + void InitiateExponentialProbing() EXCLUSIVE_LOCKS_REQUIRED(critsect_); + void InitiateProbing(std::initializer_list bitrates_to_probe, + int min_bitrate_to_probe_further_bps) + EXCLUSIVE_LOCKS_REQUIRED(critsect_); + rtc::CriticalSection critsect_; PacedSender* const pacer_; Clock* const clock_; + NetworkState network_state_ GUARDED_BY(critsect_); State state_ GUARDED_BY(critsect_); int min_bitrate_to_probe_further_bps_ GUARDED_BY(critsect_); int64_t time_last_probing_initiated_ms_ GUARDED_BY(critsect_); int estimated_bitrate_bps_ GUARDED_BY(critsect_); + int start_bitrate_bps_ GUARDED_BY(critsect_); int max_bitrate_bps_ GUARDED_BY(critsect_); int64_t last_alr_probing_time_ GUARDED_BY(critsect_); diff --git a/webrtc/modules/congestion_controller/probe_controller_unittest.cc b/webrtc/modules/congestion_controller/probe_controller_unittest.cc index 98e116644c..3842ded5a6 100644 --- a/webrtc/modules/congestion_controller/probe_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/probe_controller_unittest.cc @@ -51,6 +51,17 @@ TEST_F(ProbeControllerTest, InitiatesProbingAtStart) { kMaxBitrateBps); } +TEST_F(ProbeControllerTest, ProbeOnlyWhenNetworkIsUp) { + probe_controller_->OnNetworkStateChanged(kNetworkDown); + EXPECT_CALL(pacer_, CreateProbeCluster(_, _)).Times(0); + probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, + kMaxBitrateBps); + + testing::Mock::VerifyAndClearExpectations(&pacer_); + EXPECT_CALL(pacer_, CreateProbeCluster(_, _)).Times(AtLeast(2)); + probe_controller_->OnNetworkStateChanged(kNetworkUp); +} + TEST_F(ProbeControllerTest, InitiatesProbingOnMaxBitrateIncrease) { EXPECT_CALL(pacer_, CreateProbeCluster(_, _)).Times(AtLeast(2)); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 28971af986..aaafdb3876 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -75,6 +75,10 @@ void CallTest::RunBaseTest(BaseTest* test) { if (test->ShouldCreateReceivers()) { send_transport_->SetReceiver(receiver_call_->Receiver()); receive_transport_->SetReceiver(sender_call_->Receiver()); + if (num_video_streams_ > 0) + receiver_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp); + if (num_audio_streams_ > 0) + receiver_call_->SignalChannelNetworkState(MediaType::AUDIO, kNetworkUp); } else { // Sender-only call delivers to itself. send_transport_->SetReceiver(sender_call_->Receiver()); diff --git a/webrtc/test/direct_transport.cc b/webrtc/test/direct_transport.cc index 18d3ee6362..b26aadd054 100644 --- a/webrtc/test/direct_transport.cc +++ b/webrtc/test/direct_transport.cc @@ -28,6 +28,10 @@ DirectTransport::DirectTransport(const FakeNetworkPipe::Config& config, shutting_down_(false), fake_network_(clock_, config) { thread_.Start(); + if (send_call_) { + send_call_->SignalChannelNetworkState(MediaType::AUDIO, kNetworkUp); + send_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp); + } } DirectTransport::~DirectTransport() { StopSending(); } diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index d93dd247d1..e406d0fab2 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -122,11 +122,11 @@ class EndToEndTest : public test::CallTest { void TestRtpStatePreservation(bool use_rtx, bool provoke_rtcpsr_before_rtp); void VerifyHistogramStats(bool use_rtx, bool use_red, bool screenshare); void VerifyNewVideoSendStreamsRespectNetworkState( - MediaType network_to_bring_down, + MediaType network_to_bring_up, VideoEncoder* encoder, Transport* transport); void VerifyNewVideoReceiveStreamsRespectNetworkState( - MediaType network_to_bring_down, + MediaType network_to_bring_up, Transport* transport); }; @@ -3671,11 +3671,11 @@ TEST_F(EndToEndTest, CallReportsRttForSender) { } void EndToEndTest::VerifyNewVideoSendStreamsRespectNetworkState( - MediaType network_to_bring_down, + MediaType network_to_bring_up, VideoEncoder* encoder, Transport* transport) { CreateSenderCall(Call::Config(&event_log_)); - sender_call_->SignalChannelNetworkState(network_to_bring_down, kNetworkDown); + sender_call_->SignalChannelNetworkState(network_to_bring_up, kNetworkUp); CreateSendConfig(1, 0, 0, transport); video_send_config_.encoder_settings.encoder = encoder; @@ -3691,12 +3691,11 @@ void EndToEndTest::VerifyNewVideoSendStreamsRespectNetworkState( } void EndToEndTest::VerifyNewVideoReceiveStreamsRespectNetworkState( - MediaType network_to_bring_down, + MediaType network_to_bring_up, Transport* transport) { Call::Config config(&event_log_); CreateCalls(config, config); - receiver_call_->SignalChannelNetworkState(network_to_bring_down, - kNetworkDown); + receiver_call_->SignalChannelNetworkState(network_to_bring_up, kNetworkUp); test::DirectTransport sender_transport(sender_call_.get()); sender_transport.SetReceiver(receiver_call_->Receiver()); @@ -3738,7 +3737,7 @@ TEST_F(EndToEndTest, NewVideoSendStreamsRespectVideoNetworkDown) { UnusedEncoder unused_encoder; UnusedTransport unused_transport; VerifyNewVideoSendStreamsRespectNetworkState( - MediaType::VIDEO, &unused_encoder, &unused_transport); + MediaType::AUDIO, &unused_encoder, &unused_transport); } TEST_F(EndToEndTest, NewVideoSendStreamsIgnoreAudioNetworkDown) { @@ -3766,17 +3765,17 @@ TEST_F(EndToEndTest, NewVideoSendStreamsIgnoreAudioNetworkDown) { RequiredTransport required_transport(true /*rtp*/, false /*rtcp*/); RequiredEncoder required_encoder; VerifyNewVideoSendStreamsRespectNetworkState( - MediaType::AUDIO, &required_encoder, &required_transport); + MediaType::VIDEO, &required_encoder, &required_transport); } TEST_F(EndToEndTest, NewVideoReceiveStreamsRespectVideoNetworkDown) { UnusedTransport transport; - VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::VIDEO, &transport); + VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::AUDIO, &transport); } TEST_F(EndToEndTest, NewVideoReceiveStreamsIgnoreAudioNetworkDown) { RequiredTransport transport(false /*rtp*/, true /*rtcp*/); - VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::AUDIO, &transport); + VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::VIDEO, &transport); } void VerifyEmptyNackConfig(const NackConfig& config) { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index cc76b13515..b4df558980 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -1836,6 +1836,8 @@ TEST_F(VideoSendStreamTest, VideoSendStreamStopSetEncoderRateToZero) { test::NullTransport transport; CreateSendConfig(1, 0, 0, &transport); + sender_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp); + StartStopBitrateObserver encoder; video_send_config_.encoder_settings.encoder = &encoder; video_send_config_.encoder_settings.internal_source = true;