[remoting WebRTC] Use maxRateKbps from ICE config

This replaces the hard-coded relay bandwidth cap of 8000Kbps with
the value of maxRateKbps (if present) in the ICE config response from
the TURN server. If the value is not present, no cap is applied.

Bug: 880462
Change-Id: I0d8d2a48f78559e93cff94055a65da7ec32de494
Reviewed-on: https://chromium-review.googlesource.com/1211668Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589376}

remoting/protocol/ice_config.cc

@@ -4,6 +4,8 @@
 
 #include "remoting/protocol/ice_config.h"
 
+#include <algorithm>
+
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/strings/string_number_conversions.h"
@@ -86,6 +88,21 @@ bool AddServerToConfig(std::string url,
   return true;
 }
 
+// Returns the smallest specified value, or 0 if neither is specified.
+// A value is "specified" if it is greater than 0.
+int MinimumSpecified(int value1, int value2) {
+  if (value1 <= 0) {
+    // value1 is not specified, so return value2 (or 0).
+    return std::max(0, value2);
+  }
+  if (value2 <= 0) {
+    // value1 is specified, so return it directly.
+    return value1;
+  }
+  // Both values are specified, so return the minimum.
+  return std::min(value1, value2);
+}
+
 }  // namespace
 
 IceConfig::IceConfig() = default;
@@ -117,6 +134,7 @@ IceConfig IceConfig::Parse(const base::DictionaryValue& dictionary) {
 
   // Parse iceServers list and store them in |ice_config|.
   bool errors_found = false;
+  ice_config.max_bitrate_kbps = 0;
   for (const auto& server : *ice_servers_list) {
     const base::DictionaryValue* server_dict;
     if (!server.GetAsDictionary(&server_dict)) {
@@ -136,6 +154,16 @@ IceConfig IceConfig::Parse(const base::DictionaryValue& dictionary) {
     std::string password;
     server_dict->GetString("credential", &password);
 
+    // Compute the lowest specified bitrate of all the ICE servers.
+    // Ideally the bitrate would be stored per ICE server, but it is not
+    // possible (at the application level) to look up which particular
+    // ICE server was used for the P2P connection.
+    double new_bitrate_double;
+    if (server_dict->GetDouble("maxRateKbps", &new_bitrate_double)) {
+      ice_config.max_bitrate_kbps = MinimumSpecified(
+          ice_config.max_bitrate_kbps, static_cast<int>(new_bitrate_double));
+    }
+
     for (const auto& url : *urls_list) {
       std::string url_str;
       if (!url.GetAsString(&url_str)) {

remoting/protocol/ice_config.h

@@ -42,6 +42,10 @@ struct IceConfig {
 
   // Standard TURN servers
   std::vector<cricket::RelayServerConfig> turn_servers;
+
+  // If greater than 0, the max bandwidth used for relayed connections should
+  // be set to this value.
+  int max_bitrate_kbps = 0;
 };
 
 }  // namespace protocol

remoting/protocol/ice_config_unittest.cc

@@ -26,7 +26,8 @@ TEST(IceConfigTest, ParseValid) {
       "        \"turns:the_server.com?transport=udp\""
       "      ],"
       "      \"username\": \"123\","
-      "      \"credential\": \"abc\""
+      "      \"credential\": \"abc\","
+      "      \"maxRateKbps\": 8000.0"
       "    },"
       "    {"
       "      \"urls\": ["
@@ -71,6 +72,7 @@ TEST(IceConfigTest, ParseValid) {
   EXPECT_EQ(rtc::SocketAddress("stun_server.com", 18344),
             config.stun_servers[0]);
   EXPECT_EQ(rtc::SocketAddress("1.2.3.4", 3478), config.stun_servers[1]);
+  EXPECT_EQ(8000.0, config.max_bitrate_kbps);
 }
 
 TEST(IceConfigTest, ParseDataEnvelope) {
@@ -126,5 +128,63 @@ TEST(IceConfigTest, NotParseable) {
   EXPECT_TRUE(config.is_null());
 }
 
+TEST(IceConfigTest, UnspecifiedMaxRate_IsZero) {
+  const char kTestConfigJson[] =
+      "{"
+      "  \"iceServers\": ["
+      "    {"
+      "      \"urls\": ["
+      "        \"stun:1.2.3.4\""
+      "      ]"
+      "    }"
+      "  ]"
+      "}";
+
+  IceConfig config = IceConfig::Parse(kTestConfigJson);
+  EXPECT_EQ(0, config.max_bitrate_kbps);
+}
+
+TEST(IceConfigTest, OneSpecifiedMaxRate_IsUsed) {
+  const char kTestConfigJson1[] =
+      "{"
+      "  \"iceServers\": ["
+      "    {"
+      "      \"urls\": ["
+      "        \"stun:1.2.3.4\""
+      "      ],"
+      "      \"maxRateKbps\": 1000.0"
+      "    },"
+      "    {"
+      "      \"urls\": ["
+      "        \"stun:1.2.3.4\""
+      "      ]"
+      "    }"
+      "  ]"
+      "}";
+
+  IceConfig config1 = IceConfig::Parse(kTestConfigJson1);
+  EXPECT_EQ(1000, config1.max_bitrate_kbps);
+
+  const char kTestConfigJson2[] =
+      "{"
+      "  \"iceServers\": ["
+      "    {"
+      "      \"urls\": ["
+      "        \"stun:1.2.3.4\""
+      "      ]"
+      "    },"
+      "    {"
+      "      \"urls\": ["
+      "        \"stun:1.2.3.4\""
+      "      ],"
+      "      \"maxRateKbps\": 2000.0"
+      "    }"
+      "  ]"
+      "}";
+
+  IceConfig config2 = IceConfig::Parse(kTestConfigJson2);
+  EXPECT_EQ(2000, config2.max_bitrate_kbps);
+}
+
 }  // namespace protocol
 }  // namespace remoting

remoting/protocol/transport_context.cc

@@ -122,5 +122,10 @@ void TransportContext::OnIceConfig(RelayMode relay_mode,
   }
 }
 
+int TransportContext::GetTurnMaxRateKbps() const {
+  DCHECK_EQ(relay_mode_, RelayMode::TURN);
+  return ice_config_[RelayMode::TURN].max_bitrate_kbps;
+}
+
 }  // namespace protocol
 }  // namespace remoting

remoting/protocol/transport_context.h

@@ -101,6 +101,11 @@ class TransportContext : public base::RefCountedThreadSafe<TransportContext> {
   TransportRole role() const { return role_; }
   rtc::NetworkManager* network_manager() const { return network_manager_; }
 
+  // Returns the suggested bandwidth cap for TURN relay connections, or 0 if
+  // no rate-limit is set in the IceConfig. Currently this is not used for
+  // legacy GTURN connections.
+  int GetTurnMaxRateKbps() const;
+
  private:
   friend class base::RefCountedThreadSafe<TransportContext>;
 

remoting/protocol/webrtc_transport.cc

@@ -53,13 +53,6 @@ const int kTransportInfoSendDelayMs = 20;
 // XML namespace for the transport elements.
 const char kTransportNamespace[] = "google:remoting:webrtc";
 
-// Bitrate cap applied to relay connections. This is done to prevent
-// large amounts of packet loss, since the Google TURN/relay server drops
-// packets to limit the connection to ~10Mbps. The rate-limiting behavior works
-// badly with WebRTC's bandwidth-estimation, which results in the host process
-// trying to send frames too rapidly over the connection.
-constexpr int kMaxBitrateOnRelayKbps = 8000;
-
 #if !defined(NDEBUG)
 // Command line switch used to disable signature verification.
 // TODO(sergeyu): Remove this flag.
@@ -757,6 +750,16 @@ void WebrtcTransport::OnStatsDelivered(
   VLOG(0) << "Relay connection: " << (is_relay ? "true" : "false");
 
   if (is_relay) {
+    int max_bitrate_kbps = transport_context_->GetTurnMaxRateKbps();
+    if (max_bitrate_kbps <= 0) {
+      VLOG(0) << "No bitrate cap set.";
+      return;
+    }
+    VLOG(0) << "Capping bitrate to " << max_bitrate_kbps << "kbps.";
+
+    // Apply the suggested bitrate cap to prevent large amounts of packet loss.
+    // The Google TURN/relay server limits the connection speed by dropping
+    // packets, which may interact badly with WebRTC's bandwidth-estimation.
     auto senders = peer_connection()->GetSenders();
     for (rtc::scoped_refptr<webrtc::RtpSenderInterface> sender : senders) {
       // x-google-max-bitrate is only set for video codecs in the SDP exchange.
@@ -777,7 +780,7 @@ void WebrtcTransport::OnStatsDelivered(
                    << sender->id();
       }
 
-      parameters.encodings[0].max_bitrate_bps = kMaxBitrateOnRelayKbps * 1000;
+      parameters.encodings[0].max_bitrate_bps = max_bitrate_kbps * 1000;
       sender->SetParameters(parameters);
     }
   }