From bcbdeedd432198c3d48effb2162af6344d885b14 Mon Sep 17 00:00:00 2001
From: Danil Chapovalov <danilchap@webrtc.org>
Date: Mon, 16 Dec 2019 15:59:02 +0100
Subject: [PATCH] In RtpBitrateConfigurator ignore new parameters when set to
 default values.

Bug: webrtc:11263
Change-Id: Ia7539c7c142b059d0295849b916439bb647f112d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/162207
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30191}
---
 call/rtp_bitrate_configurator.cc          |  6 +++++-
 call/rtp_bitrate_configurator_unittest.cc | 19 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/call/rtp_bitrate_configurator.cc b/call/rtp_bitrate_configurator.cc
index 99ccfc98f7..b90c1bff5d 100644
--- a/call/rtp_bitrate_configurator.cc
+++ b/call/rtp_bitrate_configurator.cc
@@ -67,8 +67,12 @@ RtpBitrateConfigurator::UpdateWithSdpParameters(
       bitrate_config.start_bitrate_bps !=
           base_bitrate_config_.start_bitrate_bps) {
     new_start.emplace(bitrate_config.start_bitrate_bps);
+    base_bitrate_config_.start_bitrate_bps = bitrate_config.start_bitrate_bps;
   }
-  base_bitrate_config_ = bitrate_config;
+  if (bitrate_config.min_bitrate_bps > 0)
+    base_bitrate_config_.min_bitrate_bps = bitrate_config.min_bitrate_bps;
+  if (bitrate_config.max_bitrate_bps > 0)
+    base_bitrate_config_.max_bitrate_bps = bitrate_config.max_bitrate_bps;
   return UpdateConstraints(new_start);
 }
 
diff --git a/call/rtp_bitrate_configurator_unittest.cc b/call/rtp_bitrate_configurator_unittest.cc
index 6449a1a0f5..7bbdd7c22a 100644
--- a/call/rtp_bitrate_configurator_unittest.cc
+++ b/call/rtp_bitrate_configurator_unittest.cc
@@ -27,7 +27,7 @@ class RtpBitrateConfiguratorTest : public ::testing::Test {
                            absl::optional<int> max_bitrate_bps) {
     absl::optional<BitrateConstraints> result =
         configurator_->UpdateWithSdpParameters(bitrate_config);
-    EXPECT_TRUE(result.has_value());
+    ASSERT_TRUE(result.has_value());
     if (start_bitrate_bps.has_value())
       EXPECT_EQ(result->start_bitrate_bps, start_bitrate_bps);
     if (min_bitrate_bps.has_value())
@@ -231,6 +231,23 @@ TEST_F(RtpBitrateConfiguratorTest, NewConfigWithNoChangesDoesNotCallNewConfig) {
   EXPECT_FALSE(configurator_->UpdateWithSdpParameters(config2).has_value());
 }
 
+TEST_F(RtpBitrateConfiguratorTest,
+       NewConfigWithUnsetMinAndMaxDoesNotCallNewConfig) {
+  BitrateConstraints config1;
+  config1.min_bitrate_bps = 100'000;
+  config1.start_bitrate_bps = 1'000;
+  config1.max_bitrate_bps = 1'000'000;
+
+  BitrateConstraints config2;
+  config2.min_bitrate_bps = 0;
+  config2.start_bitrate_bps = -1;
+  config2.max_bitrate_bps = -1;
+
+  configurator_->UpdateWithSdpParameters(config1);
+  // The second call should return nothing because it doesn't change any values.
+  EXPECT_EQ(configurator_->UpdateWithSdpParameters(config2), absl::nullopt);
+}
+
 // If config changes the max, but not the effective max,
 // new config shouldn't be returned, to avoid unnecessary encoder
 // reconfigurations.