Reland "Periodically probe if current estimate lower than a ratio of NetworkState estimate"

This reverts commit e02fbb040e253d9e0449ad2085e32575394f88d8.

Reason for revert: Downstream tests temporalily disabled.

Original change's description:
> Revert "Periodically probe if current estimate lower than a ratio of NetworkState estimate"
>
> This reverts commit c371a13273c399249fb9bf602efed22e70e27166.
>
> Reason for revert: Speculative revert (breaks downstream project)
>
> Original change's description:
> > Periodically probe if current estimate lower than a ratio of NetworkState estimate
> >
> > This replace the immmediate probing if NetworkState estimate change.
> >
> >
> > Bug: webrtc:14392
> > Change-Id: I2cc79c21015a4da2e6cba2098f1bc3c69944821f
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280741
> > Reviewed-by: Diep Bui <diepbp@webrtc.org>
> > Commit-Queue: Per Kjellander <perkj@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#38495}
>
> Bug: webrtc:14392
> Change-Id: I83cc8ab9986171e58971fb443d3e5d83afab3a2c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280948
> Owners-Override: Artem Titov <titovartem@webrtc.org>
> Commit-Queue: Artem Titov <titovartem@webrtc.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Auto-Submit: Artem Titov <titovartem@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#38497}

Bug: webrtc:14392
Change-Id: I211599ab6061d51a825588afb0babf12c5686dfc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281120
Reviewed-by: Diep Bui <diepbp@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#38507}

modules/congestion_controller/goog_cc/probe_controller.cc

@@ -87,9 +87,12 @@ ProbeControllerConfig::ProbeControllerConfig(
       alr_probe_scale("alr_scale", 2),
       network_state_estimate_probing_interval("network_state_interval",
                                               TimeDelta::PlusInfinity()),
-      network_state_estimate_fast_rampup_rate("network_state_fast_rampup_rate",
+      probe_if_estimate_lower_than_network_state_estimate_ratio(
-                                              0),
+          "est_lower_than_network_ratio",
-      network_state_estimate_drop_down_rate("network_state_drop_down_rate", 0),
+          0),
+      estimate_lower_than_network_state_estimate_probing_interval(
+          "est_lower_than_network_interval",
+          TimeDelta::Seconds(3)),
       network_state_probe_scale("network_state_scale", 1.0),
       network_state_probe_duration("network_state_probe_duration",
                                    TimeDelta::Millis(15)),
@@ -111,10 +114,10 @@ ProbeControllerConfig::ProbeControllerConfig(
        &alr_probing_interval, &alr_probe_scale, &first_allocation_probe_scale,
        &second_allocation_probe_scale, &allocation_allow_further_probing,
        &min_probe_duration, &network_state_estimate_probing_interval,
-       &network_state_estimate_fast_rampup_rate,
+       &probe_if_estimate_lower_than_network_state_estimate_ratio,
-       &network_state_estimate_drop_down_rate, &network_state_probe_scale,
+       &estimate_lower_than_network_state_estimate_probing_interval,
-       &network_state_probe_duration, &min_probe_packets_sent,
+       &network_state_probe_scale, &network_state_probe_duration,
-       &limit_probe_target_rate_to_loss_bwe,
+       &min_probe_packets_sent, &limit_probe_target_rate_to_loss_bwe,
        &skip_if_estimate_larger_than_fraction_of_max},
       key_value_config->Lookup("WebRTC-Bwe-ProbingConfiguration"));
 
@@ -367,24 +370,6 @@ void ProbeController::SetMaxBitrate(DataRate max_bitrate) {
 
 void ProbeController::SetNetworkStateEstimate(
     webrtc::NetworkStateEstimate estimate) {
-  if (config_.network_state_estimate_fast_rampup_rate > 0 &&
-      estimated_bitrate_ < estimate.link_capacity_upper &&
-      (!network_estimate_ ||
-       estimate.link_capacity_upper >=
-           config_.network_state_estimate_fast_rampup_rate *
-               network_estimate_->link_capacity_upper)) {
-    send_probe_on_next_process_interval_ = true;
-  }
-  if (config_.network_state_estimate_drop_down_rate > 0 && network_estimate_ &&
-      !estimate.link_capacity_upper.IsZero() &&
-      (estimated_bitrate_ > estimate.link_capacity_upper ||
-       bwe_limited_due_to_packet_loss_) &&
-      estimate.link_capacity_upper <=
-          config_.network_state_estimate_drop_down_rate *
-              network_estimate_->link_capacity_upper) {
-    send_probe_on_next_process_interval_ = true;
-  }
-
   network_estimate_ = estimate;
 }
 
@@ -405,7 +390,6 @@ void ProbeController::Reset(Timestamp at_time) {
   time_of_last_large_drop_ = now;
   bitrate_before_last_large_drop_ = DataRate::Zero();
   max_total_allocated_bitrate_ = DataRate::Zero();
-  send_probe_on_next_process_interval_ = false;
 }
 
 bool ProbeController::TimeForAlrProbe(Timestamp at_time) const {
@@ -419,13 +403,34 @@ bool ProbeController::TimeForAlrProbe(Timestamp at_time) const {
 }
 
 bool ProbeController::TimeForNetworkStateProbe(Timestamp at_time) const {
-  if (config_.network_state_estimate_probing_interval->IsFinite() &&
+  if (!network_estimate_ ||
-      network_estimate_ && network_estimate_->link_capacity_upper.IsFinite() &&
+      network_estimate_->link_capacity_upper.IsInfinite()) {
-      estimated_bitrate_ < network_estimate_->link_capacity_upper) {
+    return false;
+  }
+
+  bool probe_due_to_low_estimate =
+      !bwe_limited_due_to_packet_loss_ &&
+      estimated_bitrate_ <
+          config_.probe_if_estimate_lower_than_network_state_estimate_ratio *
+              network_estimate_->link_capacity_upper;
+  if (probe_due_to_low_estimate &&
+      config_.estimate_lower_than_network_state_estimate_probing_interval
+          ->IsFinite()) {
+    Timestamp next_probe_time =
+        time_last_probing_initiated_ +
+        config_.estimate_lower_than_network_state_estimate_probing_interval;
+    return at_time >= next_probe_time;
+  }
+
+  bool periodic_probe =
+      estimated_bitrate_ < network_estimate_->link_capacity_upper;
+  if (periodic_probe &&
+      config_.network_state_estimate_probing_interval->IsFinite()) {
     Timestamp next_probe_time = time_last_probing_initiated_ +
                                 config_.network_state_estimate_probing_interval;
     return at_time >= next_probe_time;
   }
+
   return false;
 }
 
@@ -443,8 +448,7 @@ std::vector<ProbeClusterConfig> ProbeController::Process(Timestamp at_time) {
   if (estimated_bitrate_.IsZero() || state_ != State::kProbingComplete) {
     return {};
   }
-  if (send_probe_on_next_process_interval_ || TimeForAlrProbe(at_time) ||
+  if (TimeForAlrProbe(at_time) || TimeForNetworkStateProbe(at_time)) {
-      TimeForNetworkStateProbe(at_time)) {
     return InitiateProbing(
         at_time, {estimated_bitrate_ * config_.alr_probe_scale}, true);
   }
@@ -491,8 +495,6 @@ std::vector<ProbeClusterConfig> ProbeController::InitiateProbing(
         std::min(max_probe_bitrate, max_total_allocated_bitrate_ * 2);
   }
 
-  send_probe_on_next_process_interval_ = false;
-
   std::vector<ProbeClusterConfig> pending_probes;
   for (DataRate bitrate : bitrates_to_probe) {
     RTC_DCHECK(!bitrate.IsZero());

modules/congestion_controller/goog_cc/probe_controller.h

@@ -50,12 +50,12 @@ struct ProbeControllerConfig {
 
   // Configures how often we send probes if NetworkStateEstimate is available.
   FieldTrialParameter<TimeDelta> network_state_estimate_probing_interval;
-  // If the network state estimate increase more than this rate, a probe is sent
+  // Periodically probe as long as the the ratio beteeen current estimate and
-  // the next process interval.
+  // NetworkStateEstimate is lower then this.
-  FieldTrialParameter<double> network_state_estimate_fast_rampup_rate;
+  FieldTrialParameter<double>
-  // If the network state estimate decreases more than this rate, a probe is
+      probe_if_estimate_lower_than_network_state_estimate_ratio;
-  // sent the next process interval.
+  FieldTrialParameter<TimeDelta>
-  FieldTrialParameter<double> network_state_estimate_drop_down_rate;
+      estimate_lower_than_network_state_estimate_probing_interval;
   FieldTrialParameter<double> network_state_probe_scale;
   // Overrides min_probe_duration if network_state_estimate_probing_interval
   // is set and a network state estimate is known.
@@ -155,7 +155,6 @@ class ProbeController {
   DataRate min_bitrate_to_probe_further_ = DataRate::PlusInfinity();
   Timestamp time_last_probing_initiated_ = Timestamp::MinusInfinity();
   DataRate estimated_bitrate_ = DataRate::Zero();
-  bool send_probe_on_next_process_interval_;
   absl::optional<webrtc::NetworkStateEstimate> network_estimate_;
   DataRate start_bitrate_ = DataRate::Zero();
   DataRate max_bitrate_ = DataRate::PlusInfinity();

modules/congestion_controller/goog_cc/probe_controller_unittest.cc

@@ -657,88 +657,12 @@ TEST(ProbeControllerTest, CanSetLongerProbeDurationAfterNetworkStateEstimate) {
   EXPECT_EQ(probes[0].target_duration, TimeDelta::Millis(100));
 }
 
-TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateIncrease) {
+TEST(ProbeControllerTest, ProbeIfEstimateLowerThanNetworkStateEstimate) {
+  // Periodic probe every 1 second if estimate is lower than 50% of the
+  // NetworkStateEstimate.
   ProbeControllerFixture fixture(
-      "WebRTC-Bwe-ProbingConfiguration/"
+      "WebRTC-Bwe-ProbingConfiguration/est_lower_than_network_interval:1s,"
-      "network_state_interval:5s,network_state_fast_rampup_rate:2.0/");
+      "est_lower_than_network_ratio:0.5,limit_probe_"
-  std::unique_ptr<ProbeController> probe_controller =
-      fixture.CreateController();
-
-  auto probes = probe_controller->SetBitrates(
-      kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime());
-  probes = probe_controller->SetEstimatedBitrate(
-      kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false,
-      fixture.CurrentTime());
-  // Need to wait at least one second before process can trigger a new probe.
-  fixture.AdvanceTime(TimeDelta::Millis(1100));
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  NetworkStateEstimate state_estimate;
-  state_estimate.link_capacity_upper = kStartBitrate;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  // No probe since NetworkStateEstimate is not higher than the set
-  // estimated bitrate.
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  // If NetworkState increase just a bit, dont expect the probe to be sent
-  // immediately.
-  state_estimate.link_capacity_upper = kStartBitrate * 1.4;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  // If NetworkState increase dramatically, expect a probe to be sent.
-  state_estimate.link_capacity_upper = kStartBitrate * 1.4 * 2;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_EQ(probes.size(), 1u);
-}
-
-TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateDrop) {
-  ProbeControllerFixture fixture(
-      "WebRTC-Bwe-ProbingConfiguration/"
-      "network_state_interval:5s,network_state_drop_down_rate:0.5/");
-  std::unique_ptr<ProbeController> probe_controller =
-      fixture.CreateController();
-
-  auto probes = probe_controller->SetBitrates(
-      kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime());
-  probes = probe_controller->SetEstimatedBitrate(
-      kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false,
-      fixture.CurrentTime());
-  // Need to wait at least one second before process can trigger a new probe.
-  fixture.AdvanceTime(TimeDelta::Millis(1100));
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  NetworkStateEstimate state_estimate;
-  state_estimate.link_capacity_upper = kStartBitrate;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  // No probe since NetworkStateEstimate is not lower than the set
-  // estimated bitrate.
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  // If NetworkState decrease just a bit, dont expect the probe to be sent
-  // immediately.
-  state_estimate.link_capacity_upper = kStartBitrate * 0.9;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_TRUE(probes.empty());
-
-  // If NetworkState decrease dramatically, expect a probe to be sent.
-  state_estimate.link_capacity_upper = kStartBitrate * 0.9 * 0.5;
-  probe_controller->SetNetworkStateEstimate(state_estimate);
-  probes = probe_controller->Process(fixture.CurrentTime());
-  EXPECT_EQ(probes.size(), 1u);
-}
-
-TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateDropLossLimited) {
-  ProbeControllerFixture fixture(
-      "WebRTC-Bwe-ProbingConfiguration/"
-      "network_state_interval:5s,network_state_drop_down_rate:0.5,limit_probe_"
       "target_rate_to_loss_bwe:true/");
   std::unique_ptr<ProbeController> probe_controller =
       fixture.CreateController();
@@ -759,17 +683,25 @@ TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateDropLossLimited) {
   probes = probe_controller->Process(fixture.CurrentTime());
   EXPECT_TRUE(probes.empty());
 
-  // Loss limited.
+  state_estimate.link_capacity_upper = kStartBitrate * 3;
-  probes = probe_controller->SetEstimatedBitrate(
-      kStartBitrate / 3, /*bwe_limited_due_to_packet_loss=*/true,
-      fixture.CurrentTime());
-  // If NetworkState decrease dramatically, expect a probe to be sent.
-  // But limited to loss based estimate.
-  state_estimate.link_capacity_upper = kStartBitrate / 2;
   probe_controller->SetNetworkStateEstimate(state_estimate);
   probes = probe_controller->Process(fixture.CurrentTime());
   ASSERT_EQ(probes.size(), 1u);
-  EXPECT_EQ(probes[0].target_data_rate, kStartBitrate / 3);
+  EXPECT_GT(probes[0].target_data_rate, kStartBitrate);
+
+  // If network state not increased, send another probe.
+  fixture.AdvanceTime(TimeDelta::Millis(1100));
+  probes = probe_controller->Process(fixture.CurrentTime());
+  EXPECT_FALSE(probes.empty());
+
+  // Stop probing if estimate increase. We might probe further here though.
+  probes = probe_controller->SetEstimatedBitrate(
+      2 * kStartBitrate,
+      /*bwe_limited_due_to_packet_loss=*/false, fixture.CurrentTime());
+  // No more periodic probes.
+  fixture.AdvanceTime(TimeDelta::Millis(1100));
+  probes = probe_controller->Process(fixture.CurrentTime());
+  EXPECT_TRUE(probes.empty());
 }
 
 TEST(ProbeControllerTest, DontProbeFurtherWhenLossLimited) {
@@ -886,7 +818,7 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) {
   ProbeControllerFixture fixture(
       "WebRTC-Bwe-ProbingConfiguration/"
       "network_state_interval:2s,skip_if_est_larger_than_fraction_of_max:0.9,"
-      "network_state_drop_down_rate:0.5/");
+      "/");
   std::unique_ptr<ProbeController> probe_controller =
       fixture.CreateController();
   auto probes = probe_controller->SetBitrates(
@@ -899,12 +831,15 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) {
       fixture.CurrentTime());
   EXPECT_TRUE(probes.empty());
 
-  // Need to wait at least one second before process can trigger a new probe.
+  // Need to wait at least two seconds before process can trigger a new probe.
-  fixture.AdvanceTime(TimeDelta::Millis(1100));
+  fixture.AdvanceTime(TimeDelta::Millis(2100));
 
-  // Sends a probe immediately if NetworkState estimate decrease.
+  probes = probe_controller->SetEstimatedBitrate(
+      kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false,
+      fixture.CurrentTime());
+  EXPECT_TRUE(probes.empty());
   probe_controller->SetNetworkStateEstimate(
-      {.link_capacity_upper = kStartBitrate});
+      {.link_capacity_upper = 2 * kStartBitrate});
   probes = probe_controller->Process(fixture.CurrentTime());
   EXPECT_FALSE(probes.empty());
 }
@@ -912,7 +847,7 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) {
 TEST(ProbeControllerTest, DontSendProbeIfNetworkStateEstimateIsZero) {
   ProbeControllerFixture fixture(
       "WebRTC-Bwe-ProbingConfiguration/"
-      "network_state_interval:5s,network_state_drop_down_rate:0.5,limit_probe_"
+      "network_state_interval:5s,limit_probe_"
       "target_rate_to_loss_bwe:true/");
   std::unique_ptr<ProbeController> probe_controller =
       fixture.CreateController();