[Media Router] Support dual discovery

chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc

Index: chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
diff --git a/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc b/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
index 1ad648bc2b645d98e5b9983a68b5b54c95ebff3f..f8b84070fcd86b115c678a6f493a4477ffaae17b 100644
--- a/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
+++ b/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
@@ -28,8 +28,6 @@ enum ErrorType {
 };
 
 ErrorType CreateCastMediaSink(const media_router::DnsSdService& service,
-                              int channel_id,
-                              bool audio_only,
                               media_router::MediaSinkInternal* cast_sink) {
   DCHECK(cast_sink);
   if (service.service_name.find(
@@ -41,6 +39,10 @@ ErrorType CreateCastMediaSink(const media_router::DnsSdService& service,
   if (!ip_address.AssignFromIPLiteral(service.ip_address))
     return ErrorType::MISSING_OR_INVALID_IP_ADDRESS;
 
+  net::HostPortPair host_port_pair =

[mark a. foltz, 2017/07/11 23:52:51]

It looks like the local_discovery::ServiceDescription already passes this as a
net::HostPortPair.

https://cs.chromium.org/chromium/src/chrome/browser/local_discovery/service_d...

For some reason we convert it to a string along the way.  Would it be possible
to eliminate this parsing by just passing the original struct?

(Fine to do as a followup patch as that will touch several layers of unrelated
code.)

+      net::HostPortPair::FromString(service.service_host_port);
+  int port = host_port_pair.port();
+
   std::map<std::string, std::string> service_data;
   for (const auto& item : service.service_data) {
     // |item| format should be "id=xxxxxx", etc.
@@ -69,10 +71,8 @@ ErrorType CreateCastMediaSink(const media_router::DnsSdService& service,
   media_router::CastSinkExtraData extra_data;
   extra_data.ip_address = ip_address;
   extra_data.model_name = service_data["md"];
-  extra_data.capabilities = cast_channel::CastDeviceCapability::AUDIO_OUT;
-  if (!audio_only)
-    extra_data.capabilities |= cast_channel::CastDeviceCapability::VIDEO_OUT;
-  extra_data.cast_channel_id = channel_id;
+  extra_data.port = port;
+  extra_data.discovered_by_dial = false;
 
   cast_sink->set_sink(sink);
   cast_sink->set_cast_data(extra_data);
@@ -80,6 +80,22 @@ ErrorType CreateCastMediaSink(const media_router::DnsSdService& service,
   return ErrorType::NONE;
 }
 
+static media_router::MediaSinkInternal CreateCastSinkFromDialSink(
+    const media_router::MediaSinkInternal& dial_sink) {
+  std::string unique_id = dial_sink.sink().id();
+  std::string friendly_name = dial_sink.sink().name();
+  media_router::MediaSink sink(unique_id, friendly_name,
+                               media_router::MediaSink::IconType::CAST);
+
+  media_router::CastSinkExtraData extra_data;
+  extra_data.ip_address = dial_sink.dial_data().ip_address;
+  extra_data.port = media_router::CastMediaSinkService::kCastControlPort;
+  extra_data.model_name = dial_sink.dial_data().model_name;
+  extra_data.discovered_by_dial = true;
+
+  return media_router::MediaSinkInternal(sink, extra_data);
+}
+
 }  // namespace
 
 namespace media_router {
@@ -87,6 +103,9 @@ namespace media_router {
 // static
 const char CastMediaSinkService::kCastServiceType[] = "_googlecast._tcp.local";
 
+// static
+const int CastMediaSinkService::kCastControlPort = 8009;
+
 CastMediaSinkService::CastMediaSinkService(
     const OnSinksDiscoveredCallback& callback,
     content::BrowserContext* browser_context)
@@ -138,6 +157,42 @@ void CastMediaSinkService::SetDnsSdRegistryForTest(DnsSdRegistry* registry) {
   MediaSinkServiceBase::StartTimer();
 }
 
+void CastMediaSinkService::OnFetchCompleted() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  auto task_runner = content::BrowserThread::GetTaskRunnerForThread(
+      content::BrowserThread::IO);

[imcheng, 2017/07/12 00:51:36]

It might be easier to migrate this code to use TaskTraits if we kept a
scoped_refptr<SequencedTaskRunner> in this class, and use it throughout when we
need to post a task to it.

Optional to fix in this patch: replace calls of
DCHECK_CURRENTLY_ON(content::BrowserThread::IO) with a SequenceChecker. It might
be less confusing to change the current ThreadChecker calls to
DCHECK_CURRENTLY_ON(content::BrowserThread::UI).

+  base::PostTaskAndReplyWithResult(
+      task_runner.get(), FROM_HERE,
+      base::BindOnce(&CastMediaSinkService::GetCastSinksOnIOThread, this),

[imcheng, 2017/07/12 00:51:36]

Why do we need a thread hop now, but did not before in prior patchsets? This
seems a bit more complicated...

+      base::BindOnce(&CastMediaSinkService::OnFetchCompletedOnUIThread, this));
+}
+
+std::set<MediaSinkInternal> CastMediaSinkService::GetCastSinksOnIOThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  std::set<MediaSinkInternal> sinks;
+  // Copy cast sink from mDNS service to |current_sinks_|.
+  for (const auto& sink_it : current_sinks_by_mdns_map_) {
+    DVLOG(2) << "Discovered by mdns [name]: " << sink_it.second.sink().name();
+    sinks.insert(sink_it.second);
+  }
+
+  // Copy cast sink from DIAL discovery to |current_sinks_|.
+  for (const auto& sink_it : current_sinks_by_dial_map_) {
+    DVLOG(2) << "Discovered by dial [name]: " << sink_it.second.sink().name();
+    if (!base::ContainsKey(current_sinks_by_mdns_map_, sink_it.first))
+      sinks.insert(sink_it.second);
+  }
+  return sinks;
+}
+
+void CastMediaSinkService::OnFetchCompletedOnUIThread(
+    std::set<MediaSinkInternal> cast_sinks) {
+  MediaSinkServiceBase::current_sinks_ = cast_sinks;
+  MediaSinkServiceBase::OnFetchCompleted();
+}
+
 void CastMediaSinkService::OnDnsSdEvent(
     const std::string& service_type,
     const DnsSdRegistry::DnsSdServiceList& services) {
@@ -145,30 +200,68 @@ void CastMediaSinkService::OnDnsSdEvent(
   DVLOG(2) << "CastMediaSinkService::OnDnsSdEvent found " << services.size()
            << " services";
 
-  current_sinks_.clear();
-  current_services_ = services;
-
+  std::vector<MediaSinkInternal> cast_sinks;
   for (const auto& service : services) {
-    net::IPAddress ip_address;
-    if (!ip_address.AssignFromIPLiteral(service.ip_address)) {
-      DVLOG(2) << "Invalid ip_addresss: " << service.ip_address;
+    // Create Cast sink from mDNS service description.
+    MediaSinkInternal cast_sink;
+    ErrorType error = CreateCastMediaSink(service, &cast_sink);
+    if (error != ErrorType::NONE) {
+      DVLOG(2) << "Fail to create Cast device [error]: " << error;
       continue;
     }
-    net::HostPortPair host_port_pair =
-        net::HostPortPair::FromString(service.service_host_port);
 
-    content::BrowserThread::PostTask(
-        content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&CastMediaSinkService::OpenChannelOnIOThread, this, service,
-                   net::IPEndPoint(ip_address, host_port_pair.port())));
+    cast_sinks.push_back(cast_sink);

[imcheng, 2017/07/12 00:51:36]

std::move(cast_sink)

   }
 
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&CastMediaSinkService::OpenChannelsOnIOThread, this,
+                 std::move(cast_sinks)));
+
   MediaSinkServiceBase::RestartTimer();
 }
 
+void CastMediaSinkService::OpenChannelsOnIOThread(
+    std::vector<MediaSinkInternal> cast_sinks) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  current_sinks_by_mdns_map_.clear();
+  current_service_ip_endpoints_.clear();
+
+  for (const auto& cast_sink : cast_sinks) {

[imcheng, 2017/07/12 00:51:36]

No const, so you can std::move in L236.

+    net::IPEndPoint ip_endpoint(cast_sink.cast_data().ip_address,
+                                cast_sink.cast_data().port);
+    current_service_ip_endpoints_.insert(ip_endpoint);
+
+    OpenChannelOnIOThread(ip_endpoint, cast_sink);
+  }
+}
+
+void CastMediaSinkService::OnDialSinkAdded(const MediaSinkInternal& sink) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  auto ip_address = sink.dial_data().ip_address;
+  net::IPEndPoint ip_endpoint(ip_address, kCastControlPort);
+
+  if (base::ContainsKey(current_service_ip_endpoints_, ip_endpoint)) {
+    DVLOG(2) << "Sink discovered by mDNS, skip adding [name]: "
+             << sink.sink().name();
+    return;
+  }
+
+  auto cast_sink = CreateCastSinkFromDialSink(sink);
+  OpenChannelOnIOThread(ip_endpoint, cast_sink);

[imcheng, 2017/07/12 00:51:36]

Inline CreateCastSinkFromDialSink here.

+}
+
+void CastMediaSinkService::OnDialSinksRemoved() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  current_sinks_by_dial_map_.clear();
+}
+
 void CastMediaSinkService::OpenChannelOnIOThread(
-    const DnsSdService& service,
-    const net::IPEndPoint& ip_endpoint) {
+    const net::IPEndPoint& ip_endpoint,
+    const MediaSinkInternal& cast_sink) {

[imcheng, 2017/07/12 00:51:36]

pass by value here, since it will be moved.

   auto* observer = cast_socket_service_->GetObserver(kObserverId);
   if (!observer) {
     observer = new CastSocketObserver();
@@ -178,17 +271,18 @@ void CastMediaSinkService::OpenChannelOnIOThread(
   cast_socket_service_->OpenSocket(
       ip_endpoint, g_browser_process->net_log(),
       base::Bind(&CastMediaSinkService::OnChannelOpenedOnIOThread, this,

[imcheng, 2017/07/12 00:51:36]

BindOnce?

-                 service),
+                 std::move(cast_sink)),
       observer);
 }
 
 void CastMediaSinkService::OnChannelOpenedOnIOThread(
-    const DnsSdService& service,
+    MediaSinkInternal cast_sink,
     int channel_id,
     cast_channel::ChannelError channel_error) {
   if (channel_error != cast_channel::ChannelError::NONE) {
-    DVLOG(2) << "Fail to open channel " << service.ip_address << ": "
-             << service.service_host_port
+    DVLOG(2) << "Fail to open channel "
+             << cast_sink.cast_data().ip_address.ToString()
+             << " [name]: " << cast_sink.sink().name()
              << " [ChannelError]: " << (int)channel_error;
     return;
   }
@@ -199,32 +293,38 @@ void CastMediaSinkService::OnChannelOpenedOnIOThread(
     return;
   }
 
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&CastMediaSinkService::OnChannelOpenedOnUIThread, this,
-                 service, channel_id, socket->audio_only()));
-}
-
-void CastMediaSinkService::OnChannelOpenedOnUIThread(
-    const DnsSdService& service,
-    int channel_id,
-    bool audio_only) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  MediaSinkInternal sink;
-  ErrorType error = CreateCastMediaSink(service, channel_id, audio_only, &sink);
-  if (error != ErrorType::NONE) {
-    DVLOG(2) << "Fail to create Cast device [error]: " << error;
-    return;
+  // Skip adding if IP address not found in current round of mDNS discovery.
+  net::IPEndPoint ip_endpoint(cast_sink.cast_data().ip_address,
+                              cast_sink.cast_data().port);
+  if (!cast_sink.cast_data().discovered_by_dial) {
+    if (current_service_ip_endpoints_.find(ip_endpoint) ==
+        current_service_ip_endpoints_.end()) {
+      DVLOG(2) << "Service data not found in current service data list..."
+               << ip_endpoint.ToString();
+      return;
+    }
   }
 
-  if (!base::ContainsValue(current_services_, service)) {
-    DVLOG(2) << "Service data not found in current service data list...";
-    return;
+  media_router::CastSinkExtraData extra_data = cast_sink.cast_data();
+  extra_data.capabilities = cast_channel::CastDeviceCapability::AUDIO_OUT;
+  if (!socket->audio_only())
+    extra_data.capabilities |= cast_channel::CastDeviceCapability::VIDEO_OUT;
+  extra_data.cast_channel_id = channel_id;
+
+  MediaSinkInternal updated_sink(cast_sink.sink(), extra_data);
+  DVLOG(2) << "Ading sink to current_sinks_ [name]: "
+           << updated_sink.sink().name();
+
+  // Add or update existing cast sink.
+  if (updated_sink.cast_data().discovered_by_dial) {
+    current_sinks_by_dial_map_[ip_endpoint] = updated_sink;

[imcheng, 2017/07/12 00:51:36]

nit: I personally think what you had in PS3 (updating the current_sinks_* maps)
was a bit simpler, but I guess it comes down to whether it makes more sense to
do a thread hop here vs. in OnFetchCompleted. Your call.

+  } else {
+    current_sinks_by_mdns_map_[ip_endpoint] = updated_sink;
   }
 
-  DVLOG(2) << "Ading sink to current_sinks_ [id]: " << sink.sink().id();
-  current_sinks_.insert(sink);
-  MediaSinkServiceBase::RestartTimer();
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CastMediaSinkService::RestartTimer, this));
 }
 
 CastMediaSinkService::CastSocketObserver::CastSocketObserver() {}