Merge changes from topics "presubmit-am-025ffaa7069d4daca36c1227fbebabf9", "presubmit-am-090b26a61183498bb2baf851895231b2", "presubmit-am-32ae09e4874d428da9e567e760afd257", "presubmit-am-49414361cd82472bb3f7ee2ab5cf5032", "presubmit-am-7e2160445c1d426991f61c62212491ec", "presubmit-am-892dc6fe59b24fa29f054fd74e214240" into tm-qpr-dev

* changes:
  WebRTC client always sends ice servers to device
  Correctly handle ICE servers in webrtc client
  Delete unused code in webrtc client
  Accept ice servers with the 'request-offer' message
  Create the peer connection from the client handler
  Parse ice_servers property from client handler

4 files changed, 192 insertions, 115 deletions

diff --git a/host/frontend/webrtc/client/js/cf_webrtc.js b/host/frontend/webrtc/client/js/cf_webrtc.js
index 5c9138340..073ea36b1 100644
--- a/host/frontend/webrtc/client/js/cf_webrtc.js
+++ b/host/frontend/webrtc/client/js/cf_webrtc.js
@@ -422,7 +422,7 @@ class Controller {
     this.#pc.addIceCandidate(iceCandidate);
   }
 
-  ConnectDevice(pc) {
+  ConnectDevice(pc, infraConfig) {
     this.#pc = pc;
     console.debug('ConnectDevice');
     // ICE candidates will be generated when we add the offer. Adding it here
@@ -432,7 +432,8 @@ class Controller {
     this.#pc.addEventListener('icecandidate', evt => {
       if (evt.candidate) this.#sendIceCandidate(evt.candidate);
     });
-    this.#serverConnector.sendToDevice({type: 'request-offer'});
+    this.#serverConnector.sendToDevice(
+        {type: 'request-offer', ice_servers: infraConfig.ice_servers});
   }
 
   async renegotiateConnection() {
@@ -445,10 +446,7 @@ class Controller {
 }
 
 function createPeerConnection(infra_config) {
-  let pc_config = {iceServers: []};
-  for (const stun of infra_config.ice_servers) {
-    pc_config.iceServers.push({urls: 'stun:' + stun});
-  }
+  let pc_config = {iceServers: infra_config.ice_servers};
   let pc = new RTCPeerConnection(pc_config);
 
   pc.addEventListener('icecandidate', evt => {
@@ -470,12 +468,6 @@ export async function Connect(deviceId, serverConnector) {
   let infraConfig = requestRet.infraConfig;
   console.debug('Device available:');
   console.debug(deviceInfo);
-  let pc_config = {iceServers: []};
-  if (infraConfig.ice_servers && infraConfig.ice_servers.length > 0) {
-    for (const server of infraConfig.ice_servers) {
-      pc_config.iceServers.push(server);
-    }
-  }
   let pc = createPeerConnection(infraConfig);
 
   let control = new Controller(serverConnector);
@@ -491,6 +483,6 @@ export async function Connect(deviceId, serverConnector) {
         reject(evt);
       }
     });
-    control.ConnectDevice(pc);
+    control.ConnectDevice(pc, infraConfig);
   });
 }
diff --git a/host/frontend/webrtc/lib/client_handler.cpp b/host/frontend/webrtc/lib/client_handler.cpp
index 9be1ca095..94aa1758e 100644
--- a/host/frontend/webrtc/lib/client_handler.cpp
+++ b/host/frontend/webrtc/lib/client_handler.cpp
@@ -457,22 +457,65 @@ void CameraChannelHandler::OnMessage(const webrtc::DataBuffer &msg) {
                          msg_data + msg.size());
 }
 
+std::vector<webrtc::PeerConnectionInterface::IceServer>
+ClientHandler::ParseIceServersMessage(const Json::Value &message) {
+  std::vector<webrtc::PeerConnectionInterface::IceServer> ret;
+  if (!message.isMember("ice_servers") || !message["ice_servers"].isArray()) {
+    // Log as verbose since the ice_servers field is optional in some messages
+    LOG(VERBOSE) << "ice_servers field not present in json object or not an array";
+    return ret;
+  }
+  auto& servers = message["ice_servers"];
+  for (const auto& server: servers) {
+    webrtc::PeerConnectionInterface::IceServer ice_server;
+    if (!server.isMember("urls") || !server["urls"].isArray()) {
+      // The urls field is required
+      LOG(WARNING)
+          << "ICE server specification missing urls field or not an array: "
+          << server.toStyledString();
+      continue;
+    }
+    auto urls = server["urls"];
+    for (int url_idx = 0; url_idx < urls.size(); url_idx++) {
+      auto url = urls[url_idx];
+      if (!url.isString()) {
+        LOG(WARNING) << "Non string 'urls' field in ice server: "
+                     << url.toStyledString();
+        continue;
+      }
+      ice_server.urls.push_back(url.asString());
+    }
+    if (server.isMember("credential") && server["credential"].isString()) {
+      ice_server.password = server["credential"].asString();
+    }
+    if (server.isMember("username") && server["username"].isString()) {
+      ice_server.username = server["username"].asString();
+    }
+    ret.push_back(ice_server);
+  }
+  return ret;
+}
+
 std::shared_ptr<ClientHandler> ClientHandler::Create(
     int client_id, std::shared_ptr<ConnectionObserver> observer,
+    PeerConnectionBuilder &connection_builder,
     std::function<void(const Json::Value &)> send_to_client_cb,
     std::function<void(bool)> on_connection_changed_cb) {
-  return std::shared_ptr<ClientHandler>(new ClientHandler(
-      client_id, observer, send_to_client_cb, on_connection_changed_cb));
+  return std::shared_ptr<ClientHandler>(
+      new ClientHandler(client_id, observer, connection_builder,
+                        send_to_client_cb, on_connection_changed_cb));
 }
 
 ClientHandler::ClientHandler(
     int client_id, std::shared_ptr<ConnectionObserver> observer,
+    PeerConnectionBuilder &connection_builder,
     std::function<void(const Json::Value &)> send_to_client_cb,
     std::function<void(bool)> on_connection_changed_cb)
     : client_id_(client_id),
       observer_(observer),
       send_to_client_(send_to_client_cb),
       on_connection_changed_cb_(on_connection_changed_cb),
+      connection_builder_(connection_builder),
       camera_track_(new ClientVideoTrackImpl()) {}
 
 ClientHandler::~ClientHandler() {
@@ -481,54 +524,37 @@ ClientHandler::~ClientHandler() {
   }
 }
 
-bool ClientHandler::SetPeerConnection(
-    rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection) {
-  peer_connection_ = peer_connection;
-
-  // libwebrtc configures the video encoder with a start bitrate of just 300kbs
-  // which causes it to drop the first 4 frames it receives. Any value over 2Mbs
-  // will be capped at 2Mbs when passed to the encoder by the peer_connection
-  // object, so we pass the maximum possible value here.
-  webrtc::BitrateSettings bitrate_settings;
-  bitrate_settings.start_bitrate_bps = 2000000; // 2Mbs
-  peer_connection_->SetBitrate(bitrate_settings);
-  // At least one data channel needs to be created on the side that makes the
-  // SDP offer (the device) for data channels to be enabled at all.
-  // This channel is meant to carry control commands from the client.
-  auto control_channel = peer_connection_->CreateDataChannel(
-      "device-control", nullptr /* config */);
-  if (!control_channel) {
-    LOG(ERROR) << "Failed to create control data channel";
-    return false;
-  }
-  control_handler_.reset(new ControlChannelHandler(control_channel, observer_));
-  return true;
-}
-
 bool ClientHandler::AddDisplay(
     rtc::scoped_refptr<webrtc::VideoTrackInterface> video_track,
     const std::string &label) {
-  // Send each track as part of a different stream with the label as id
-  auto err_or_sender =
-      peer_connection_->AddTrack(video_track, {label} /* stream_id */);
-  if (!err_or_sender.ok()) {
-    LOG(ERROR) << "Failed to add video track to the peer connection";
-    return false;
+  displays_.emplace_back(video_track, label);
+  if (peer_connection_) {
+    // Send each track as part of a different stream with the label as id
+    auto err_or_sender =
+        peer_connection_->AddTrack(video_track, {label} /* stream_id */);
+    if (!err_or_sender.ok()) {
+      LOG(ERROR) << "Failed to add video track to the peer connection";
+      return false;
+    }
+    // TODO (b/154138394): use the returned sender (err_or_sender.value()) to
+    // remove the display from the connection.
   }
-  // TODO (b/154138394): use the returned sender (err_or_sender.value()) to
-  // remove the display from the connection.
   return true;
 }
 
 bool ClientHandler::AddAudio(
     rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track,
     const std::string &label) {
-  // Send each track as part of a different stream with the label as id
-  auto err_or_sender =
-      peer_connection_->AddTrack(audio_track, {label} /* stream_id */);
-  if (!err_or_sender.ok()) {
-    LOG(ERROR) << "Failed to add video track to the peer connection";
-    return false;
+  // Store the audio track for when the peer connection is created
+  audio_streams_.emplace_back(audio_track, label);
+  if (peer_connection_) {
+    // Send each track as part of a different stream with the label as id
+    auto err_or_sender =
+        peer_connection_->AddTrack(audio_track, {label} /* stream_id */);
+    if (!err_or_sender.ok()) {
+      LOG(ERROR) << "Failed to add video track to the peer connection";
+      return false;
+    }
   }
   return true;
 }
@@ -558,6 +584,56 @@ void ClientHandler::AddPendingIceCandidates() {
   pending_ice_candidates_.clear();
 }
 
+bool ClientHandler::BuildPeerConnection(const Json::Value &message) {
+  auto ice_servers = ParseIceServersMessage(message);
+  peer_connection_ = connection_builder_.Build(this, ice_servers);
+  if (!peer_connection_) {
+    return false;
+  }
+
+  // Re-add the video and audio tracks after the peer connection has been
+  // created
+  decltype(displays_) tmp_displays;
+  tmp_displays.swap(displays_);
+  for (auto &pair : tmp_displays) {
+    auto &video_track = pair.first;
+    auto &label = pair.second;
+    if (!AddDisplay(video_track, label)) {
+      return false;
+    }
+  }
+  decltype(audio_streams_) tmp_audio_streams;
+  tmp_audio_streams.swap(audio_streams_);
+  for (auto &pair : tmp_audio_streams) {
+    auto &audio_track = pair.first;
+    auto &label = pair.second;
+    if (!AddAudio(audio_track, label)) {
+      return false;
+    }
+  }
+
+  // libwebrtc configures the video encoder with a start bitrate of just 300kbs
+  // which causes it to drop the first 4 frames it receives. Any value over 2Mbs
+  // will be capped at 2Mbs when passed to the encoder by the peer_connection
+  // object, so we pass the maximum possible value here.
+  webrtc::BitrateSettings bitrate_settings;
+  bitrate_settings.start_bitrate_bps = 2000000;  // 2Mbs
+  peer_connection_->SetBitrate(bitrate_settings);
+
+  // At least one data channel needs to be created on the side that makes the
+  // SDP offer (the device) for data channels to be enabled at all.
+  // This channel is meant to carry control commands from the client.
+  auto control_channel = peer_connection_->CreateDataChannel(
+      "device-control", nullptr /* config */);
+  if (!control_channel) {
+    LOG(ERROR) << "Failed to create control data channel";
+    return false;
+  }
+  control_handler_.reset(new ControlChannelHandler(control_channel, observer_));
+
+  return true;
+}
+
 void ClientHandler::OnCreateSDPSuccess(
     webrtc::SessionDescriptionInterface *desc) {
   std::string offer_str;
@@ -607,9 +683,15 @@ void ClientHandler::HandleMessage(const Json::Value &message) {
   }
   auto type = message["type"].asString();
   if (type == "request-offer") {
-    // Can't check for state being different that kNew because renegotiation can
-    // start in any state after the answer is returned.
-    if (state_ == State::kCreatingOffer) {
+    if (state_ == State::kNew) {
+      // The peer connection must be created on the first request-offer
+      if (!BuildPeerConnection(message)) {
+        LogAndReplyError("Failed to create peer connection");
+        return;
+      }
+      // Renegotiation can start in any state after the answer is returned, not
+      // just kNew.
+    } else if (state_ == State::kCreatingOffer) {
       // An offer has been requested already
       LogAndReplyError("Multiple requests for offer received from single client");
       return;
diff --git a/host/frontend/webrtc/lib/client_handler.h b/host/frontend/webrtc/lib/client_handler.h
index ea58552c3..2617e6404 100644
--- a/host/frontend/webrtc/lib/client_handler.h
+++ b/host/frontend/webrtc/lib/client_handler.h
@@ -42,19 +42,24 @@ class CameraChannelHandler;
 
 class ClientVideoTrackInterface;
 class ClientVideoTrackImpl;
+class PeerConnectionBuilder;
 
 class ClientHandler : public webrtc::PeerConnectionObserver,
                       public std::enable_shared_from_this<ClientHandler> {
  public:
+  // Checks if the message contains an "ice_servers" array field and parses it
+  // into a vector of webrtc ICE servers. Returns an empty vector if the field
+  // isn't present.
+  static std::vector<webrtc::PeerConnectionInterface::IceServer>
+  ParseIceServersMessage(const Json::Value& message);
+
   static std::shared_ptr<ClientHandler> Create(
       int client_id, std::shared_ptr<ConnectionObserver> observer,
+      PeerConnectionBuilder& connection_builder,
       std::function<void(const Json::Value&)> send_client_cb,
       std::function<void(bool)> on_connection_changed_cb);
   ~ClientHandler() override;
 
-  bool SetPeerConnection(
-      rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection);
-
   bool AddDisplay(rtc::scoped_refptr<webrtc::VideoTrackInterface> track,
                   const std::string& label);
 
@@ -113,6 +118,7 @@ class ClientHandler : public webrtc::PeerConnectionObserver,
       kFailed,
   };
   ClientHandler(int client_id, std::shared_ptr<ConnectionObserver> observer,
+                PeerConnectionBuilder& connection_builder,
                 std::function<void(const Json::Value&)> send_client_cb,
                 std::function<void(bool)> on_connection_changed_cb);
 
@@ -121,12 +127,14 @@ class ClientHandler : public webrtc::PeerConnectionObserver,
 
   void LogAndReplyError(const std::string& error_msg) const;
   void AddPendingIceCandidates();
+  bool BuildPeerConnection(const Json::Value& message);
 
   int client_id_;
   State state_ = State::kNew;
   std::shared_ptr<ConnectionObserver> observer_;
   std::function<void(const Json::Value&)> send_to_client_;
   std::function<void(bool)> on_connection_changed_cb_;
+  PeerConnectionBuilder& connection_builder_;
   rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_;
   std::vector<rtc::scoped_refptr<webrtc::DataChannelInterface>> data_channels_;
   std::unique_ptr<InputChannelHandler> input_handler_;
@@ -138,6 +146,12 @@ class ClientHandler : public webrtc::PeerConnectionObserver,
   bool remote_description_added_ = false;
   std::vector<std::unique_ptr<webrtc::IceCandidateInterface>>
       pending_ice_candidates_;
+  std::vector<
+      std::pair<rtc::scoped_refptr<webrtc::VideoTrackInterface>, std::string>>
+      displays_;
+  std::vector<
+      std::pair<rtc::scoped_refptr<webrtc::AudioTrackInterface>, std::string>>
+      audio_streams_;
 };
 
 class ClientVideoTrackInterface {
@@ -148,5 +162,14 @@ class ClientVideoTrackInterface {
       const rtc::VideoSinkWants& wants) = 0;
 };
 
+class PeerConnectionBuilder {
+ public:
+  virtual ~PeerConnectionBuilder() = default;
+  virtual rtc::scoped_refptr<webrtc::PeerConnectionInterface> Build(
+      webrtc::PeerConnectionObserver* observer,
+      const std::vector<webrtc::PeerConnectionInterface::IceServer>&
+          per_connection_servers) = 0;
+};
+
 }  // namespace webrtc_streaming
 }  // namespace cuttlefish
diff --git a/host/frontend/webrtc/lib/streamer.cpp b/host/frontend/webrtc/lib/streamer.cpp
index 088b25b6f..9dd8feacb 100644
--- a/host/frontend/webrtc/lib/streamer.cpp
+++ b/host/frontend/webrtc/lib/streamer.cpp
@@ -136,6 +136,7 @@ class AudioDeviceModuleWrapper : public AudioSource {
 
 
 class Streamer::Impl : public ServerConnectionObserver,
+                       public PeerConnectionBuilder,
                        public std::enable_shared_from_this<ServerConnectionObserver> {
  public:
   std::shared_ptr<ClientHandler> CreateClientHandler(int client_id);
@@ -155,6 +156,12 @@ class Streamer::Impl : public ServerConnectionObserver,
   void HandleConfigMessage(const Json::Value& msg);
   void HandleClientMessage(const Json::Value& server_message);
 
+  // PeerConnectionBuilder
+  rtc::scoped_refptr<webrtc::PeerConnectionInterface> Build(
+      webrtc::PeerConnectionObserver* observer,
+      const std::vector<webrtc::PeerConnectionInterface::IceServer>&
+          per_connection_servers) override;
+
   // All accesses to these variables happen from the signal_thread, so there is
   // no need for extra synchronization mechanisms (mutex)
   StreamerConfig config_;
@@ -474,39 +481,8 @@ void Streamer::Impl::OnError(const std::string& error) {
 void Streamer::Impl::HandleConfigMessage(const Json::Value& server_message) {
   CHECK(signal_thread_->IsCurrent())
       << __FUNCTION__ << " called from the wrong thread";
-  if (server_message.isMember("ice_servers") &&
-      server_message["ice_servers"].isArray()) {
-    auto servers = server_message["ice_servers"];
-    operator_config_.servers.clear();
-    for (int server_idx = 0; server_idx < servers.size(); server_idx++) {
-      auto server = servers[server_idx];
-      webrtc::PeerConnectionInterface::IceServer ice_server;
-      if (!server.isMember("urls") || !server["urls"].isArray()) {
-        // The urls field is required
-        LOG(WARNING)
-            << "Invalid ICE server specification obtained from server: "
-            << server.toStyledString();
-        continue;
-      }
-      auto urls = server["urls"];
-      for (int url_idx = 0; url_idx < urls.size(); url_idx++) {
-        auto url = urls[url_idx];
-        if (!url.isString()) {
-          LOG(WARNING) << "Non string 'urls' field in ice server: "
-                       << url.toStyledString();
-          continue;
-        }
-        ice_server.urls.push_back(url.asString());
-        if (server.isMember("credential") && server["credential"].isString()) {
-          ice_server.password = server["credential"].asString();
-        }
-        if (server.isMember("username") && server["username"].isString()) {
-          ice_server.username = server["username"].asString();
-        }
-        operator_config_.servers.push_back(ice_server);
-      }
-    }
-  }
+  operator_config_.servers =
+      ClientHandler::ParseIceServersMessage(server_message);
 }
 
 void Streamer::Impl::HandleClientMessage(const Json::Value& server_message) {
@@ -598,7 +574,7 @@ std::shared_ptr<ClientHandler> Streamer::Impl::CreateClientHandler(
   auto observer = connection_observer_factory_->CreateObserver();
 
   auto client_handler = ClientHandler::Create(
-      client_id, observer,
+      client_id, observer, *this,
       [this, client_id](const Json::Value& msg) {
         SendMessageToClient(client_id, msg);
       },
@@ -610,28 +586,6 @@ std::shared_ptr<ClientHandler> Streamer::Impl::CreateClientHandler(
         }
       });
 
-  webrtc::PeerConnectionInterface::RTCConfiguration config;
-  config.sdp_semantics = webrtc::SdpSemantics::kUnifiedPlan;
-  config.enable_dtls_srtp = true;
-  config.servers.insert(config.servers.end(), operator_config_.servers.begin(),
-                        operator_config_.servers.end());
-  webrtc::PeerConnectionDependencies dependencies(client_handler.get());
-  // PortRangeSocketFactory's super class' constructor needs to be called on the
-  // network thread or have it as a parameter
-  dependencies.packet_socket_factory.reset(new PortRangeSocketFactory(
-      network_thread_.get(), config_.udp_port_range, config_.tcp_port_range));
-  auto peer_connection = peer_connection_factory_->CreatePeerConnection(
-      config, std::move(dependencies));
-
-  if (!peer_connection) {
-    LOG(ERROR) << "Failed to create peer connection";
-    return nullptr;
-  }
-
-  if (!client_handler->SetPeerConnection(std::move(peer_connection))) {
-    return nullptr;
-  }
-
   for (auto& entry : displays_) {
     auto& label = entry.first;
     auto& video_source = entry.second.source;
@@ -652,6 +606,32 @@ std::shared_ptr<ClientHandler> Streamer::Impl::CreateClientHandler(
   return client_handler;
 }
 
+rtc::scoped_refptr<webrtc::PeerConnectionInterface> Streamer::Impl::Build(
+    webrtc::PeerConnectionObserver* observer,
+    const std::vector<webrtc::PeerConnectionInterface::IceServer>&
+        per_connection_servers) {
+  webrtc::PeerConnectionInterface::RTCConfiguration config;
+  config.sdp_semantics = webrtc::SdpSemantics::kUnifiedPlan;
+  config.enable_dtls_srtp = true;
+  config.servers.insert(config.servers.end(), operator_config_.servers.begin(),
+                        operator_config_.servers.end());
+  config.servers.insert(config.servers.end(), per_connection_servers.begin(),
+                        per_connection_servers.end());
+  webrtc::PeerConnectionDependencies dependencies(observer);
+  // PortRangeSocketFactory's super class' constructor needs to be called on the
+  // network thread or have it as a parameter
+  dependencies.packet_socket_factory.reset(new PortRangeSocketFactory(
+      network_thread_.get(), config_.udp_port_range, config_.tcp_port_range));
+  auto peer_connection = peer_connection_factory_->CreatePeerConnection(
+      config, std::move(dependencies));
+
+  if (!peer_connection) {
+    LOG(ERROR) << "Failed to create peer connection";
+    return nullptr;
+  }
+  return peer_connection;
+}
+
 void Streamer::Impl::SendMessageToClient(int client_id,
                                          const Json::Value& msg) {
   LOG(VERBOSE) << "Sending to client: " << msg.toStyledString();