From 906c5dc6b79aee2a6a1fb0a97372838092a030bf Mon Sep 17 00:00:00 2001 From: honghaiz Date: Tue, 15 Nov 2016 14:39:06 -0800 Subject: [PATCH] Revert of Start probes only after network is connected. (patchset #9 id:240001 of https://codereview.webrtc.org/2458863002/ ) Reason for revert: It broke downstream test. Original issue's description: > 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 > > Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e > Cr-Commit-Position: refs/heads/master@{#15094} TBR=philipel@webrtc.org,stefan@webrtc.org,sergeyu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:6332 Review-Url: https://codereview.webrtc.org/2504783002 Cr-Commit-Position: refs/heads/master@{#15098} --- 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 | 57 ++++++------------- .../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, 34 insertions(+), 96 deletions(-) diff --git a/webrtc/call/bitrate_allocator.cc b/webrtc/call/bitrate_allocator.cc index 2c5ba6d76c..645ee3c41c 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_(0), + last_bitrate_bps_(kDefaultBitrateBps), 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 7fe353a9a5..2f69cf4627 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_(kNetworkDown), - video_network_state_(kNetworkDown), + audio_network_state_(kNetworkUp), + video_network_state_(kNetworkUp), receive_crit_(RWLockWrapper::CreateRWLock()), send_crit_(RWLockWrapper::CreateRWLock()), event_log_(config.event_log), @@ -284,7 +284,6 @@ 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 e307d9f703..024ae5d22e 100644 --- a/webrtc/media/base/videoengine_unittest.h +++ b/webrtc/media/base/videoengine_unittest.h @@ -84,7 +84,6 @@ 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 4c8c871594..7fab75df29 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -341,7 +341,6 @@ TEST_F(WebRtcVideoEngine2Test, UseExternalFactoryForVp8WhenSupported) { std::unique_ptr channel( SetUpForExternalEncoderFactory(&encoder_factory, parameters.codecs)); - channel->OnReadyToSend(true); EXPECT_TRUE( channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc))); @@ -869,7 +868,6 @@ 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(); @@ -1685,7 +1683,6 @@ TEST_F(WebRtcVideoChannel2Test, SetMediaConfigSuspendBelowMinBitrate) { channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); - channel_->OnReadyToSend(true); channel_->SetSendParameters(send_parameters_); @@ -1695,7 +1692,6 @@ 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_); @@ -1980,7 +1976,6 @@ 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(); @@ -2055,7 +2050,6 @@ 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(); @@ -2123,7 +2117,6 @@ void WebRtcVideoChannel2Test::TestCpuAdaptation(bool enable_overuse, } channel_.reset( engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions())); - channel_->OnReadyToSend(true); EXPECT_TRUE(channel_->SetSendParameters(parameters)); @@ -3620,7 +3613,6 @@ 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 accc1411f9..0dafdf65fd 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -297,7 +297,6 @@ 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 272c1eb4e5..7e1d936a1e 100644 --- a/webrtc/modules/congestion_controller/probe_controller.cc +++ b/webrtc/modules/congestion_controller/probe_controller.cc @@ -46,12 +46,10 @@ 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()) {} @@ -59,51 +57,30 @@ void ProbeController::SetBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps) { rtc::CritScope cs(&critsect_); - - start_bitrate_bps_ = - start_bitrate_bps > 0 ? start_bitrate_bps : min_bitrate_bps; + 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); + } int old_max_bitrate_bps = max_bitrate_bps_; max_bitrate_bps_ = max_bitrate_bps; - 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; + // 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); } } -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 e60a007b9d..efcc2a1d4a 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,21 +45,13 @@ 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 3842ded5a6..98e116644c 100644 --- a/webrtc/modules/congestion_controller/probe_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/probe_controller_unittest.cc @@ -51,17 +51,6 @@ 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 8edaab5436..ddff552565 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -71,10 +71,6 @@ 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 b26aadd054..18d3ee6362 100644 --- a/webrtc/test/direct_transport.cc +++ b/webrtc/test/direct_transport.cc @@ -28,10 +28,6 @@ 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 ea1ecc2ffb..52ff43e11d 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_up, + MediaType network_to_bring_down, VideoEncoder* encoder, Transport* transport); void VerifyNewVideoReceiveStreamsRespectNetworkState( - MediaType network_to_bring_up, + MediaType network_to_bring_down, Transport* transport); }; @@ -3587,11 +3587,11 @@ TEST_F(EndToEndTest, CallReportsRttForSender) { } void EndToEndTest::VerifyNewVideoSendStreamsRespectNetworkState( - MediaType network_to_bring_up, + MediaType network_to_bring_down, VideoEncoder* encoder, Transport* transport) { CreateSenderCall(Call::Config(&event_log_)); - sender_call_->SignalChannelNetworkState(network_to_bring_up, kNetworkUp); + sender_call_->SignalChannelNetworkState(network_to_bring_down, kNetworkDown); CreateSendConfig(1, 0, 0, transport); video_send_config_.encoder_settings.encoder = encoder; @@ -3607,11 +3607,12 @@ void EndToEndTest::VerifyNewVideoSendStreamsRespectNetworkState( } void EndToEndTest::VerifyNewVideoReceiveStreamsRespectNetworkState( - MediaType network_to_bring_up, + MediaType network_to_bring_down, Transport* transport) { Call::Config config(&event_log_); CreateCalls(config, config); - receiver_call_->SignalChannelNetworkState(network_to_bring_up, kNetworkUp); + receiver_call_->SignalChannelNetworkState(network_to_bring_down, + kNetworkDown); test::DirectTransport sender_transport(sender_call_.get()); sender_transport.SetReceiver(receiver_call_->Receiver()); @@ -3653,7 +3654,7 @@ TEST_F(EndToEndTest, NewVideoSendStreamsRespectVideoNetworkDown) { UnusedEncoder unused_encoder; UnusedTransport unused_transport; VerifyNewVideoSendStreamsRespectNetworkState( - MediaType::AUDIO, &unused_encoder, &unused_transport); + MediaType::VIDEO, &unused_encoder, &unused_transport); } TEST_F(EndToEndTest, NewVideoSendStreamsIgnoreAudioNetworkDown) { @@ -3681,17 +3682,17 @@ TEST_F(EndToEndTest, NewVideoSendStreamsIgnoreAudioNetworkDown) { RequiredTransport required_transport(true /*rtp*/, false /*rtcp*/); RequiredEncoder required_encoder; VerifyNewVideoSendStreamsRespectNetworkState( - MediaType::VIDEO, &required_encoder, &required_transport); + MediaType::AUDIO, &required_encoder, &required_transport); } TEST_F(EndToEndTest, NewVideoReceiveStreamsRespectVideoNetworkDown) { UnusedTransport transport; - VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::AUDIO, &transport); + VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::VIDEO, &transport); } TEST_F(EndToEndTest, NewVideoReceiveStreamsIgnoreAudioNetworkDown) { RequiredTransport transport(false /*rtp*/, true /*rtcp*/); - VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::VIDEO, &transport); + VerifyNewVideoReceiveStreamsRespectNetworkState(MediaType::AUDIO, &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 7b1f0b836e..f6112f9ed9 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -1657,8 +1657,6 @@ 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;