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}
diff --git a/webrtc/call/bitrate_allocator.cc b/webrtc/call/bitrate_allocator.cc
index 2c5ba6d..645ee3c 100644
--- a/webrtc/call/bitrate_allocator.cc
+++ b/webrtc/call/bitrate_allocator.cc
@@ -48,7 +48,7 @@
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 7fe353a..2f69cf4 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -252,8 +252,8 @@
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 @@
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 e307d9f..024ae5d 100644
--- a/webrtc/media/base/videoengine_unittest.h
+++ b/webrtc/media/base/videoengine_unittest.h
@@ -84,7 +84,6 @@
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 4c8c871..7fab75d 100644
--- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -341,7 +341,6 @@
std::unique_ptr<VideoMediaChannel> channel(
SetUpForExternalEncoderFactory(&encoder_factory, parameters.codecs));
- channel->OnReadyToSend(true);
EXPECT_TRUE(
channel->AddSendStream(cricket::StreamParams::CreateLegacy(kSsrc)));
@@ -869,7 +868,6 @@
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 @@
channel_.reset(
engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions()));
- channel_->OnReadyToSend(true);
channel_->SetSendParameters(send_parameters_);
@@ -1695,7 +1692,6 @@
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 @@
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 @@
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 @@
}
channel_.reset(
engine_.CreateChannel(fake_call_.get(), media_config, VideoOptions()));
- channel_->OnReadyToSend(true);
EXPECT_TRUE(channel_->SetSendParameters(parameters));
@@ -3620,7 +3613,6 @@
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 accc141..0dafdf6 100644
--- a/webrtc/modules/congestion_controller/congestion_controller.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller.cc
@@ -297,7 +297,6 @@
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 272c1eb..7e1d936 100644
--- a/webrtc/modules/congestion_controller/probe_controller.cc
+++ b/webrtc/modules/congestion_controller/probe_controller.cc
@@ -46,12 +46,10 @@
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 @@
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 e60a007..efcc2a1 100644
--- a/webrtc/modules/congestion_controller/probe_controller.h
+++ b/webrtc/modules/congestion_controller/probe_controller.h
@@ -31,12 +31,12 @@
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<int> 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 @@
// Probing is complete.
kProbingComplete,
};
-
- void InitiateExponentialProbing() EXCLUSIVE_LOCKS_REQUIRED(critsect_);
- void InitiateProbing(std::initializer_list<int> 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 3842ded..98e1166 100644
--- a/webrtc/modules/congestion_controller/probe_controller_unittest.cc
+++ b/webrtc/modules/congestion_controller/probe_controller_unittest.cc
@@ -51,17 +51,6 @@
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 8edaab5..ddff552 100644
--- a/webrtc/test/call_test.cc
+++ b/webrtc/test/call_test.cc
@@ -71,10 +71,6 @@
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 b26aadd..18d3ee6 100644
--- a/webrtc/test/direct_transport.cc
+++ b/webrtc/test/direct_transport.cc
@@ -28,10 +28,6 @@
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 ea1ecc2..52ff43e 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -122,11 +122,11 @@
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 @@
}
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::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 @@
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 @@
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 7b1f0b8..f6112f9 100644
--- a/webrtc/video/video_send_stream_tests.cc
+++ b/webrtc/video/video_send_stream_tests.cc
@@ -1657,8 +1657,6 @@
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;
