Start probes only after network is connected.

webrtc/modules/congestion_controller/probe_controller.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/congestion_controller/probe_controller.h"
 
 #include <algorithm>
 #include <initializer_list>
 
 #include "webrtc/base/logging.h"
+#include "webrtc/base/networkroute.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
 
 namespace {
 
 // Number of deltas between probes per cluster. On the very first cluster,
 // we will need kProbeDeltasPerCluster + 1 probes, but on a cluster following
 // another, we need kProbeDeltasPerCluster probes.
 constexpr int kProbeDeltasPerCluster = 5;
@@ skipping 15 matching lines @@
 
 }  // namespace
 
 ProbeController::ProbeController(PacedSender* pacer, Clock* clock)
     : pacer_(pacer),
       clock_(clock),
       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()) {}
 
-void ProbeController::SetBitrates(int min_bitrate_bps,
-                                  int start_bitrate_bps,
+void ProbeController::SetBitrates(int start_bitrate_bps,
+                                  int min_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);
-  }
+
+  start_bitrate_bps_ =
+      start_bitrate_bps > 0 ? start_bitrate_bps : min_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
   //     we actually want to increase the |max_bitrate_bps_|.
-  if (state_ != State::kWaitingForProbingResult &&
+  if (state_ == State::kProbingComplete &&

[stefan-webrtc, 2016/10/31 10:58:27]

Could you comment on this change?

[Sergey Ulanov, 2016/10/31 18:07:27]

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.

[stefan-webrtc, 2016/11/02 13:27:17]

Acknowledged.

       estimated_bitrate_bps_ != 0 && estimated_bitrate_bps_ < max_bitrate_bps &&

[philipel, 2016/11/02 16:15:03]

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?

[Sergey Ulanov, 2016/11/02 21:12:43]

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.

[philipel, 2016/11/08 09:48:18]

Ah, right :)

       max_bitrate_bps > max_bitrate_bps_) {
     InitiateProbing({max_bitrate_bps}, kExponentialProbingDisabled);
   }
   max_bitrate_bps_ = max_bitrate_bps;
 }
 
+void ProbeController::OnNetworkRouteChanged(const rtc::NetworkRoute& route) {
+  rtc::CritScope cs(&critsect_);
+  if (route.connected && state_ == State::kInit) {

[philipel, 2016/11/02 16:15:03]

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.

[Sergey Ulanov, 2016/11/02 21:12:43]

I'd like to work on mid-call probing later, in a separate CL. This CL just
improves behavior for the initial probing.

[philipel, 2016/11/08 09:48:18]

Acknowledged.

+    // 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) {
     if ((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) >
         kMaxWaitingTimeForProbingResultMs) {
       LOG(LS_INFO) << "kWaitingForProbingResult: timeout";
       state_ = State::kProbingComplete;
       min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
     } else {
       // Continue probing if probing results indicate channel has greater
@@ skipping 51 matching lines @@
   }
   min_bitrate_to_probe_further_bps_ = min_bitrate_to_probe_further_bps;
   time_last_probing_initiated_ms_ = clock_->TimeInMilliseconds();
   if (min_bitrate_to_probe_further_bps == kExponentialProbingDisabled)
     state_ = State::kProbingComplete;
   else
     state_ = State::kWaitingForProbingResult;
 }
 
 }  // namespace webrtc