Reland of "Revert of Update the BWE when the network route changes. (patchset #5 id:180001 of https://… (patchset #1 id:1 of https://codereview.webrtc.org/2098703004/ )
Reason for revert: It turns out this revert was not necessary because the connection-state mapping for turn-turn connections was not done in connection. Original issue's description: > Revert of Revert "Revert of Update the BWE when the network route changes. (patchset #5 id:180001 of https://… (patchset #5 id:120001 of https://codereview.webrtc.org/2041593002/ ) > > Reason for revert: > ReadyToSendMedia did not consider the new presumed_writable state. > > Original issue's description: > > Revert "Revert of Update the BWE when the network route changes. (patchset #5 id:180001 of https://codereview.webrtc.org/2000063003/ )" > > > > This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65. > > > > New change made: > > Do not reset the BWE when the new network route is not ready to send media. > > > > BUG= > > R=pthatcher@webrtc.org, stefan@webrtc.org > > TBR=pthatcher@webrtc.org,stefan@webrtc.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review-Url: https://codereview.webrtc.org/2094863003 Cr-Commit-Position: refs/heads/master@{#13282}
This commit is contained in:
parent
ae4d0d922b
commit
059e183419
@ -30,10 +30,11 @@ struct NetworkRoute {
|
||||
last_sent_packet_id(-1) {}
|
||||
|
||||
// The route is connected if the local and remote network ids are provided.
|
||||
NetworkRoute(uint16_t local_net_id,
|
||||
NetworkRoute(bool connected,
|
||||
uint16_t local_net_id,
|
||||
uint16_t remote_net_id,
|
||||
int last_packet_id)
|
||||
: connected(true),
|
||||
: connected(connected),
|
||||
local_network_id(local_net_id),
|
||||
remote_network_id(remote_net_id),
|
||||
last_sent_packet_id(last_packet_id) {}
|
||||
|
||||
@ -636,9 +636,13 @@ void Call::OnNetworkRouteChanged(const std::string& transport_name,
|
||||
kv->second = network_route;
|
||||
LOG(LS_INFO) << "Network route changed on transport " << transport_name
|
||||
<< ": new local network id " << network_route.local_network_id
|
||||
<< " new remote network id "
|
||||
<< network_route.remote_network_id;
|
||||
// TODO(holmer): Update the BWE bitrates.
|
||||
<< " new remote network id " << network_route.remote_network_id
|
||||
<< " Reset bitrate to "
|
||||
<< config_.bitrate_config.start_bitrate_bps << "bps";
|
||||
congestion_controller_->ResetBweAndBitrates(
|
||||
config_.bitrate_config.start_bitrate_bps,
|
||||
config_.bitrate_config.min_bitrate_bps,
|
||||
config_.bitrate_config.max_bitrate_bps);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -138,6 +138,18 @@ void BitrateControllerImpl::SetBitrates(int start_bitrate_bps,
|
||||
MaybeTriggerOnNetworkChanged();
|
||||
}
|
||||
|
||||
void BitrateControllerImpl::ResetBitrates(int bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) {
|
||||
{
|
||||
rtc::CritScope cs(&critsect_);
|
||||
bandwidth_estimation_ = SendSideBandwidthEstimation();
|
||||
bandwidth_estimation_.SetBitrates(bitrate_bps, min_bitrate_bps,
|
||||
max_bitrate_bps);
|
||||
}
|
||||
MaybeTriggerOnNetworkChanged();
|
||||
}
|
||||
|
||||
void BitrateControllerImpl::SetReservedBitrate(uint32_t reserved_bitrate_bps) {
|
||||
{
|
||||
rtc::CritScope cs(&critsect_);
|
||||
|
||||
@ -46,6 +46,10 @@ class BitrateControllerImpl : public BitrateController {
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) override;
|
||||
|
||||
void ResetBitrates(int bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) override;
|
||||
|
||||
void UpdateDelayBasedEstimate(uint32_t bitrate_bps) override;
|
||||
|
||||
void SetReservedBitrate(uint32_t reserved_bitrate_bps) override;
|
||||
|
||||
@ -70,6 +70,10 @@ class BitrateController : public Module {
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) = 0;
|
||||
|
||||
virtual void ResetBitrates(int bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) = 0;
|
||||
|
||||
virtual void UpdateDelayBasedEstimate(uint32_t bitrate_bps) = 0;
|
||||
|
||||
virtual void SetEventLog(RtcEventLog* event_log) = 0;
|
||||
|
||||
@ -35,6 +35,8 @@ class MockBitrateController : public BitrateController {
|
||||
void(int start_bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps));
|
||||
MOCK_METHOD3(ResetBitrates,
|
||||
void(int bitrate_bps, int min_bitrate_bps, int max_bitrate_bps));
|
||||
MOCK_METHOD1(UpdateDelayBasedEstimate, void(uint32_t bitrate_bps));
|
||||
MOCK_METHOD1(SetEventLog, void(RtcEventLog* event_log));
|
||||
MOCK_CONST_METHOD1(AvailableBandwidth, bool(uint32_t* bandwidth));
|
||||
|
||||
@ -33,6 +33,22 @@ namespace {
|
||||
|
||||
static const uint32_t kTimeOffsetSwitchThreshold = 30;
|
||||
|
||||
// Makes sure that the bitrate and the min, max values are in valid range.
|
||||
static void ClampBitrates(int* bitrate_bps,
|
||||
int* min_bitrate_bps,
|
||||
int* max_bitrate_bps) {
|
||||
// TODO(holmer): We should make sure the default bitrates are set to 10 kbps,
|
||||
// and that we don't try to set the min bitrate to 0 from any applications.
|
||||
// The congestion controller should allow a min bitrate of 0.
|
||||
const int kMinBitrateBps = 10000;
|
||||
if (*min_bitrate_bps < kMinBitrateBps)
|
||||
*min_bitrate_bps = kMinBitrateBps;
|
||||
if (*max_bitrate_bps > 0)
|
||||
*max_bitrate_bps = std::max(*min_bitrate_bps, *max_bitrate_bps);
|
||||
if (*bitrate_bps > 0)
|
||||
*bitrate_bps = std::max(*min_bitrate_bps, *bitrate_bps);
|
||||
}
|
||||
|
||||
class WrappingBitrateEstimator : public RemoteBitrateEstimator {
|
||||
public:
|
||||
WrappingBitrateEstimator(RemoteBitrateObserver* observer, Clock* clock)
|
||||
@ -212,21 +228,10 @@ void CongestionController::Init() {
|
||||
min_bitrate_bps_);
|
||||
}
|
||||
|
||||
|
||||
void CongestionController::SetBweBitrates(int min_bitrate_bps,
|
||||
int start_bitrate_bps,
|
||||
int max_bitrate_bps) {
|
||||
// TODO(holmer): We should make sure the default bitrates are set to 10 kbps,
|
||||
// and that we don't try to set the min bitrate to 0 from any applications.
|
||||
// The congestion controller should allow a min bitrate of 0.
|
||||
const int kMinBitrateBps = 10000;
|
||||
if (min_bitrate_bps < kMinBitrateBps)
|
||||
min_bitrate_bps = kMinBitrateBps;
|
||||
if (max_bitrate_bps > 0)
|
||||
max_bitrate_bps = std::max(min_bitrate_bps, max_bitrate_bps);
|
||||
if (start_bitrate_bps > 0)
|
||||
start_bitrate_bps = std::max(min_bitrate_bps, start_bitrate_bps);
|
||||
|
||||
ClampBitrates(&start_bitrate_bps, &min_bitrate_bps, &max_bitrate_bps);
|
||||
bitrate_controller_->SetBitrates(start_bitrate_bps,
|
||||
min_bitrate_bps,
|
||||
max_bitrate_bps);
|
||||
@ -239,6 +244,28 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps,
|
||||
MaybeTriggerOnNetworkChanged();
|
||||
}
|
||||
|
||||
void CongestionController::ResetBweAndBitrates(int bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps) {
|
||||
ClampBitrates(&bitrate_bps, &min_bitrate_bps, &max_bitrate_bps);
|
||||
// TODO(honghaiz): Recreate this object once the bitrate controller is
|
||||
// no longer exposed outside CongestionController.
|
||||
bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps,
|
||||
max_bitrate_bps);
|
||||
min_bitrate_bps_ = min_bitrate_bps;
|
||||
// TODO(honghaiz): Recreate this object once the remote bitrate estimator is
|
||||
// no longer exposed outside CongestionController.
|
||||
if (remote_bitrate_estimator_)
|
||||
remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps);
|
||||
|
||||
RemoteBitrateEstimator* rbe =
|
||||
new RemoteBitrateEstimatorAbsSendTime(&transport_feedback_adapter_);
|
||||
transport_feedback_adapter_.SetBitrateEstimator(rbe);
|
||||
rbe->SetMinBitrate(min_bitrate_bps);
|
||||
// TODO(holmer): Trigger a new probe once mid-call probing is implemented.
|
||||
MaybeTriggerOnNetworkChanged();
|
||||
}
|
||||
|
||||
BitrateController* CongestionController::GetBitrateController() const {
|
||||
return bitrate_controller_.get();
|
||||
}
|
||||
|
||||
@ -124,6 +124,20 @@ TEST_F(CongestionControllerTest, SignalNetworkState) {
|
||||
controller_->SignalNetworkState(kNetworkDown);
|
||||
}
|
||||
|
||||
TEST_F(CongestionControllerTest, ResetBweAndBitrates) {
|
||||
int new_bitrate = 200000;
|
||||
EXPECT_CALL(observer_, OnNetworkChanged(new_bitrate, _, _));
|
||||
EXPECT_CALL(*pacer_, SetEstimatedBitrate(new_bitrate));
|
||||
controller_->ResetBweAndBitrates(new_bitrate, -1, -1);
|
||||
|
||||
// If the bitrate is reset to -1, the new starting bitrate will be
|
||||
// the minimum default bitrate 10000bps.
|
||||
int min_default_bitrate = 10000;
|
||||
EXPECT_CALL(observer_, OnNetworkChanged(min_default_bitrate, _, _));
|
||||
EXPECT_CALL(*pacer_, SetEstimatedBitrate(min_default_bitrate));
|
||||
controller_->ResetBweAndBitrates(-1, -1, -1);
|
||||
}
|
||||
|
||||
TEST_F(CongestionControllerTest,
|
||||
SignalNetworkStateAndQueueIsFullAndEstimateChange) {
|
||||
// Send queue is full
|
||||
|
||||
@ -70,6 +70,11 @@ class CongestionController : public CallStatsObserver, public Module {
|
||||
virtual void SetBweBitrates(int min_bitrate_bps,
|
||||
int start_bitrate_bps,
|
||||
int max_bitrate_bps);
|
||||
// Resets both the BWE state and the bitrate estimator. Note the first
|
||||
// argument is the bitrate_bps.
|
||||
virtual void ResetBweAndBitrates(int bitrate_bps,
|
||||
int min_bitrate_bps,
|
||||
int max_bitrate_bps);
|
||||
virtual void SignalNetworkState(NetworkState state);
|
||||
virtual BitrateController* GetBitrateController() const;
|
||||
virtual RemoteBitrateEstimator* GetRemoteBitrateEstimator(
|
||||
|
||||
@ -21,6 +21,8 @@ class CandidatePairInterface {
|
||||
|
||||
virtual const Candidate& local_candidate() const = 0;
|
||||
virtual const Candidate& remote_candidate() const = 0;
|
||||
|
||||
virtual bool ReadyToSendMedia() const = 0;
|
||||
};
|
||||
|
||||
} // namespace cricket
|
||||
|
||||
@ -476,6 +476,8 @@ class FakeCandidatePair : public CandidatePairInterface {
|
||||
return remote_candidate_;
|
||||
}
|
||||
|
||||
bool ReadyToSendMedia() const override { return true; }
|
||||
|
||||
private:
|
||||
Candidate local_candidate_;
|
||||
Candidate remote_candidate_;
|
||||
|
||||
@ -1525,7 +1525,7 @@ ProxyConnection::ProxyConnection(Port* port,
|
||||
|
||||
int ProxyConnection::Send(const void* data, size_t size,
|
||||
const rtc::PacketOptions& options) {
|
||||
if (write_state_ == STATE_WRITE_INIT || write_state_ == STATE_WRITE_TIMEOUT) {
|
||||
if (!ReadyToSendMedia()) {
|
||||
error_ = EWOULDBLOCK;
|
||||
return SOCKET_ERROR;
|
||||
}
|
||||
|
||||
@ -461,6 +461,11 @@ class Connection : public CandidatePairInterface,
|
||||
bool active() const {
|
||||
return write_state_ != STATE_WRITE_TIMEOUT;
|
||||
}
|
||||
virtual bool ReadyToSendMedia() const {
|
||||
return write_state_ == STATE_WRITABLE ||
|
||||
write_state_ == STATE_WRITE_UNRELIABLE;
|
||||
}
|
||||
|
||||
// A connection is dead if it can be safely deleted.
|
||||
bool dead(int64_t now) const;
|
||||
|
||||
|
||||
@ -608,6 +608,7 @@ void BaseChannel::OnSelectedCandidatePairChanged(
|
||||
rtc::NetworkRoute network_route;
|
||||
if (selected_candidate_pair) {
|
||||
network_route = rtc::NetworkRoute(
|
||||
selected_candidate_pair->ReadyToSendMedia(),
|
||||
selected_candidate_pair->local_candidate().network_id(),
|
||||
selected_candidate_pair->remote_candidate().network_id(),
|
||||
last_sent_packet_id);
|
||||
|
||||
@ -937,7 +937,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
|
||||
});
|
||||
WaitForThreads();
|
||||
EXPECT_EQ(1, media_channel1->num_network_route_changes());
|
||||
rtc::NetworkRoute expected_network_route(kLocalNetId, kRemoteNetId,
|
||||
rtc::NetworkRoute expected_network_route(true, kLocalNetId, kRemoteNetId,
|
||||
kLastPacketId);
|
||||
EXPECT_EQ(expected_network_route, media_channel1->last_network_route());
|
||||
EXPECT_EQ(kLastPacketId,
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user