Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458863002:
Start probes only after network is connected. (Closed)
|
Created:
4 years, 1 month ago by Sergey Ulanov Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStart 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
Committed: https://crrev.com/e2b1501101802ad8f4b7e38cad673a19bc2ff849
Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e
Cr-Original-Commit-Position: refs/heads/master@{#15094}
Cr-Commit-Position: refs/heads/master@{#15204}
Patch Set 1 #Patch Set 2 : . #
Total comments: 12
Patch Set 3 : simpler fix #
Total comments: 4
Patch Set 4 : address feedback #Patch Set 5 : comment #
Total comments: 4
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : . #Patch Set 8 : fix tests #Patch Set 9 : sync #Patch Set 10 : sync #Dependent Patchsets: Messages
Total messages: 79 (45 generated)
Description was changed from ========== 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. BUG=webrtc:6332 ========== to ========== 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). Also removed SignalNetworkState() and ResetBweAndBitrates() from CongessionController as they are no longer useful with OnNetworkRouteChanged() BUG=webrtc:6332 ==========
sergeyu@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Description was changed from ========== 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). Also removed SignalNetworkState() and ResetBweAndBitrates() from CongessionController as they are no longer useful with OnNetworkRouteChanged() BUG=webrtc:6332 ========== to ========== 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). Also removed SignalNetworkState() and ResetBweAndBitrates() from CongessionController as they are no longer useful with OnNetworkRouteChanged() BUG=webrtc:6332 ==========
stefan@webrtc.org changed reviewers: + honghaiz@webrtc.org
+honghaiz to comment on the change to OnNetworkRouteChanged and SignalNetworkState. https://codereview.webrtc.org/2458863002/diff/20001/webrtc/base/networkroute.h File webrtc/base/networkroute.h (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/base/networkroute.... webrtc/base/networkroute.h:14: #include <cstdint> Can this be removed?
Overall I think this looks like a pretty nice improvement. https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:71: if (state_ == State::kProbingComplete && Could you comment on this change?
https://codereview.webrtc.org/2458863002/diff/20001/webrtc/base/networkroute.h File webrtc/base/networkroute.h (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/base/networkroute.... webrtc/base/networkroute.h:14: #include <cstdint> On 2016/10/31 10:48:17, stefan-webrtc (holmer) wrote: > Can this be removed? We need this include because this file uses uint16_t, it fails to compile otherwise. https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:71: if (state_ == State::kProbingComplete && On 2016/10/31 10:58:27, stefan-webrtc (holmer) wrote: > Could you comment on this change? We don't want to start probing here when the network is still offline and state_ = kInit. This logic supposed to apply mid-call, but previously it was applied in kInit state, i.e. before transport is not connected, and this is not what we want here.
Philip, Honghai, please take a look. This change looks good, but I want the others to review too. In particular it would be good if Honghai confirmed that OnNetworkRouteChanged can be used to replace SignalNetworkState this way with preserved functionality. https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:71: if (state_ == State::kProbingComplete && On 2016/10/31 18:07:27, Sergey Ulanov wrote: > On 2016/10/31 10:58:27, stefan-webrtc (holmer) wrote: > > Could you comment on this change? > > We don't want to start probing here when the network is still offline and state_ > = kInit. This logic supposed to apply mid-call, but previously it was applied in > kInit state, i.e. before transport is not connected, and this is not what we > want here. Acknowledged.
https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:72: estimated_bitrate_bps_ != 0 && estimated_bitrate_bps_ < max_bitrate_bps && In order for probing to start before we have a network it looks like we both have to have an estimate (|estimated_bitrate_bps_| != 0) and we have to hand the PacedSender a packet to send. Do you know why that occurs? https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:81: if (route.connected && state_ == State::kInit) { I think if we add a bool, something like |is_midcall_|, and set it to true here we could replace the "estimated_bitrate_bps_ != 0" expression with |is_midcall_| instead. I think it is a bit clearer and maybe make the mid-call probing a bit more robust.
https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:72: estimated_bitrate_bps_ != 0 && estimated_bitrate_bps_ < max_bitrate_bps && On 2016/11/02 16:15:03, philipel wrote: > In order for probing to start before we have a network it looks like we both > have to have an estimate (|estimated_bitrate_bps_| != 0) and we have to hand the > PacedSender a packet to send. > > Do you know why that occurs? Previously probing was started by InitiateProbing() we had above, which would happen when SetBitrate() is called in kInit state. This logic is to restart probing whenever max_bitrate is updated. https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:81: if (route.connected && state_ == State::kInit) { On 2016/11/02 16:15:03, philipel wrote: > I think if we add a bool, something like |is_midcall_|, and set it to true here > we could replace the "estimated_bitrate_bps_ != 0" expression with |is_midcall_| > instead. I think it is a bit clearer and maybe make the mid-call probing a bit > more robust. I'd like to work on mid-call probing later, in a separate CL. This CL just improves behavior for the initial probing.
https://codereview.webrtc.org/2458863002/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (left): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/call/call.cc#oldco... webrtc/call/call.cc:718: UpdateAggregateNetworkState(); I don't think it is appropriate to remove this here. It was called when sending failed here: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?rcl=147... If you remove this, when sending failed, it won't update the network state.
Description was changed from ========== 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). Also removed SignalNetworkState() and ResetBweAndBitrates() from CongessionController as they are no longer useful with OnNetworkRouteChanged() BUG=webrtc:6332 ========== to ========== 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 ==========
I've simplified this change to keep SignalNetworkState() for now. ProbeController gets notifications about network state changes and start probing only when network is up.
Patchset #3 (id:40001) has been deleted
ping
https://codereview.webrtc.org/2458863002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2458863002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:284: congestion_controller_->SignalNetworkState(kNetworkDown); The initial values of network state in different classes (here, CongestionController, and ProbeController) are different. Does it make sense to set them all to be kNetworkDown and avoid this call here? https://codereview.webrtc.org/2458863002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:82: MaybeInitiateProbing(); It is a little strange here that you may have called InitiateProbing and then call MaybeInitiateProbing again (I know it won't be called twice because of the state, but it still looks strange). Possible options I would suggest: 1. Do MaybeInitiateProbing under an else clause. 2. Inline the MaybeInitiateProbing and have a switch-case or if-else based on different state_. 3. Or maybe something else to avoid the potential double-calling.
https://codereview.webrtc.org/2458863002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2458863002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:284: congestion_controller_->SignalNetworkState(kNetworkDown); On 2016/11/07 23:31:41, honghaiz3 wrote: > The initial values of network state in different classes (here, > CongestionController, and ProbeController) are different. Does it make sense to > set them all to be kNetworkDown and avoid this call here? There problem is that that CongestionController is used elsewhere without calling SignalNetworkState. Changing CongesionController to be in Down state by default would break that logic. https://codereview.webrtc.org/2458863002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:82: MaybeInitiateProbing(); On 2016/11/07 23:31:41, honghaiz3 wrote: > It is a little strange here that you may have called InitiateProbing and then > call MaybeInitiateProbing again (I know it won't be called twice because of the > state, but it still looks strange). > Possible options I would suggest: > 1. Do MaybeInitiateProbing under an else clause. > 2. Inline the MaybeInitiateProbing and have a switch-case or if-else based on > different state_. > 3. Or maybe something else to avoid the potential double-calling. Replaced MaybeIntiiateProbing() with InitiateExponentialProbing() and added a switch statement.
lgtm. Thanks for addressing my comments.
https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:72: estimated_bitrate_bps_ != 0 && estimated_bitrate_bps_ < max_bitrate_bps && On 2016/11/02 21:12:43, Sergey Ulanov wrote: > On 2016/11/02 16:15:03, philipel wrote: > > In order for probing to start before we have a network it looks like we both > > have to have an estimate (|estimated_bitrate_bps_| != 0) and we have to hand > the > > PacedSender a packet to send. > > > > Do you know why that occurs? > > Previously probing was started by InitiateProbing() we had above, which would > happen when SetBitrate() is called in kInit state. > > This logic is to restart probing whenever max_bitrate is updated. Ah, right :) https://codereview.webrtc.org/2458863002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:81: if (route.connected && state_ == State::kInit) { On 2016/11/02 21:12:43, Sergey Ulanov wrote: > On 2016/11/02 16:15:03, philipel wrote: > > I think if we add a bool, something like |is_midcall_|, and set it to true > here > > we could replace the "estimated_bitrate_bps_ != 0" expression with > |is_midcall_| > > instead. I think it is a bit clearer and maybe make the mid-call probing a bit > > more robust. > > I'd like to work on mid-call probing later, in a separate CL. This CL just > improves behavior for the initial probing. Acknowledged. https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_controller.cc:63: start_bitrate_bps_ = min_bitrate_bps <= start_bitrate_bps <= max_bitrate_bps? https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_controller.cc:89: void ProbeController::OnNetworkStateChanged(NetworkState state) { Maybe rename |state| to |network_state|, slightly confusing that |state_| and |state| are two different types.
https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_controller.cc:63: start_bitrate_bps_ = On 2016/11/08 09:48:18, philipel wrote: > min_bitrate_bps <= start_bitrate_bps <= max_bitrate_bps? This function is called with with start_bitrate_bps = 0 when application doesn't specify start bitrate. Updated call.cc to always pass non-zero start bitrate and added a DCHECK here. https://codereview.webrtc.org/2458863002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_controller.cc:89: void ProbeController::OnNetworkStateChanged(NetworkState state) { On 2016/11/08 09:48:18, philipel wrote: > Maybe rename |state| to |network_state|, slightly confusing that |state_| and > |state| are two different types. Done.
https://codereview.webrtc.org/2458863002/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2458863002/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:701: config_.bitrate_config.start_bitrate_bps, Is this a change in behavior? Did we perhaps previously sometimes call with 0 here to indicate that we didn't want to restart a new start bitrate, but simply update the min and max? I think that is what the comment on line 694-695 is saying.
https://codereview.webrtc.org/2458863002/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2458863002/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:701: config_.bitrate_config.start_bitrate_bps, On 2016/11/09 18:07:55, stefan-webrtc (holmer) wrote: > Is this a change in behavior? Did we perhaps previously sometimes call with 0 > here to indicate that we didn't want to restart a new start bitrate, but simply > update the min and max? > > I think that is what the comment on line 694-695 is saying. Reverted this code back to what it was and added back the check for start_bitrate_bps > 0 in ProbeController::SetBitrates(). It doesn't look right the parameter called start_bitrate_bps actually changes current bitrate. Maybe it shouldn't be called start_bitrate_bps? Or maybe SetBweBitrate() should be updated to use that parameter only to initialize bitrate when we don't have a BW estimate? The problem seems to be in BitrateControllerImpl::SetBitrates(): it uses parameter start_bitrate_bps to reset current bitrate (note that there is also ResetBitrates() which actually resets bitrates).
Stefan: ping
lgtm
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2458863002/#ps140001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18333) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/19927)
lgtm
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20126) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/3153)
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/19996)
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #9 (id:180001) has been deleted
Patchset #8 (id:160001) has been deleted
In the latest patchset updated tests which were previously broken because Call is now initialized in kNetworkDown state
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2458863002/#ps220001 (title: "fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for webrtc/video/end_to_end_tests.cc:
While running git apply --index -p1;
error: patch failed: webrtc/video/end_to_end_tests.cc:3589
error: webrtc/video/end_to_end_tests.cc: patch does not apply
Patch: webrtc/video/end_to_end_tests.cc
Index: webrtc/video/end_to_end_tests.cc
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index
492f55d6ec8bf1863a0bb0f903d2f1f40f32d9fe..1aac4a817e45da3a0ad91602869007059824c217
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);
};
@@ -3589,11 +3589,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, transport);
video_send_config_.encoder_settings.encoder = encoder;
@@ -3609,12 +3609,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());
@@ -3656,7 +3655,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) {
@@ -3684,17 +3683,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) {
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2458863002/#ps240001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e Cr-Commit-Position: refs/heads/master@{#15094}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.webrtc.org/2504783002/ by honghaiz@webrtc.org. The reason for reverting is: It broke downstream test. .
Message was sent while issue was closed.
Description was changed from ========== 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} ========== to ========== 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} ==========
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2458863002/#ps260001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by sergeyu@chromium.org
The CQ bit was unchecked by sergeyu@chromium.org
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
Description was changed from ========== 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} ========== to ========== 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 Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e Cr-Commit-Position: refs/heads/master@{#15094} Committed: https://chromium.googlesource.com/external/webrtc/+/e2b1501101802ad8f4b7e38ca... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) manually as e2b1501101802ad8f4b7e38cad673a19bc2ff849 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== 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 Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e Cr-Commit-Position: refs/heads/master@{#15094} Committed: https://chromium.googlesource.com/external/webrtc/+/e2b1501101802ad8f4b7e38ca... ========== to ========== 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 Committed: https://crrev.com/e2b1501101802ad8f4b7e38cad673a19bc2ff849 Committed: https://crrev.com/5c99c76255ee7bface3c742c25fb5617748ac86e Cr-Original-Commit-Position: refs/heads/master@{#15094} Cr-Commit-Position: refs/heads/master@{#15204} ========== |
