Make setRemoteDescription, setLocalDescription et al async.

content/renderer/media/rtc_peer_connection_handler.cc

Index: content/renderer/media/rtc_peer_connection_handler.cc
diff --git a/content/renderer/media/rtc_peer_connection_handler.cc b/content/renderer/media/rtc_peer_connection_handler.cc
index 844e250113fe187285b26de5eeddfe1670e00b24..91b65339180b1501e656021e2dc0fd13306f6114 100644
--- a/content/renderer/media/rtc_peer_connection_handler.cc
+++ b/content/renderer/media/rtc_peer_connection_handler.cc
@@ -117,24 +117,111 @@ GetWebKitSignalingState(webrtc::PeerConnectionInterface::SignalingState state) {
   }
 }
 
+static blink::WebRTCSessionDescription CreateWebKitSessionDescription(
+    const std::string& sdp, const std::string& type) {
+  blink::WebRTCSessionDescription description;
+  description.initialize(base::UTF8ToUTF16(type), base::UTF8ToUTF16(sdp));
+  return description;
+}
+
 static blink::WebRTCSessionDescription
 CreateWebKitSessionDescription(
     const webrtc::SessionDescriptionInterface* native_desc) {
-  blink::WebRTCSessionDescription description;
   if (!native_desc) {
     LOG(ERROR) << "Native session description is null.";
-    return description;
+    return blink::WebRTCSessionDescription();
   }
 
   std::string sdp;
   if (!native_desc->ToString(&sdp)) {
     LOG(ERROR) << "Failed to get SDP string of native session description.";
-    return description;
+    return blink::WebRTCSessionDescription();
   }
 
-  description.initialize(base::UTF8ToUTF16(native_desc->type()),
-                         base::UTF8ToUTF16(sdp));
-  return description;
+  return CreateWebKitSessionDescription(sdp, native_desc->type());
+}
+
+void RunClosureWithTrace(const base::Closure& closure,

[perkj_chrome, 2014/11/05 10:31:02]

static to be consistent with above. Or move all to anonymouse ns. Here and
below.

[tommi (sloooow) - chröme, 2014/11/05 10:50:38]

Done.  Moved all into an anonymous namespace.  Had to move the implementation of
LocalRTCStatsRequest down a bit because of that.

+                         const char* trace_event_name) {
+  TRACE_EVENT0("webrtc", trace_event_name);
+  closure.Run();
+}
+
+void RunSynchronousClosure(const base::Closure& closure,
+                           const char* trace_event_name,
+                           base::WaitableEvent* event) {
+  {
+    TRACE_EVENT0("webrtc", trace_event_name);
+    closure.Run();
+  }
+  event->Signal();
+}
+
+template<typename ResultType>
+void RunSynchronousClosureWithResult(

[perkj_chrome, 2014/11/05 10:31:02]

unused

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+    const base::Callback<ResultType()>& closure,
+    const char* trace_event_name,
+    base::WaitableEvent* event,
+    ResultType* result) {
+  {
+    TRACE_EVENT0("webrtc", trace_event_name);
+    *result = closure.Run();
+  }
+  event->Signal();
+}
+
+void RunSynchronousClosureOnThread(

[perkj_chrome, 2014/11/05 10:31:01]

unused

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+    const scoped_refptr<base::SingleThreadTaskRunner>& thread,
+    const base::Closure& closure,
+    const char* trace_event_name) {
+  if (thread->BelongsToCurrentThread()) {
+    TRACE_EVENT0("webrtc", trace_event_name);
+    closure.Run();
+  } else {
+    base::WaitableEvent event(false, false);
+    thread->PostTask(FROM_HERE,
+        base::Bind(&RunSynchronousClosure, closure,
+                   base::Unretained(trace_event_name),
+                   base::Unretained(&event)));
+    event.Wait();
+  }
+}
+
+template<typename ResultType>
+ResultType RunSynchronousClosureOnThreadWithResultT(

[perkj_chrome, 2014/11/05 10:31:01]

unused?

[tommi (sloooow) - chröme, 2014/11/05 10:50:38]

Done.

+    const scoped_refptr<base::SingleThreadTaskRunner>& thread,
+    const base::Callback<ResultType()>& closure,
+    const char* trace_event_name) {
+  ResultType ret = ResultType();
+  if (!thread.get() || thread->BelongsToCurrentThread()) {
+    TRACE_EVENT0("webrtc", trace_event_name);
+    ret = closure.Run();
+  } else {
+    base::WaitableEvent event(false, false);
+    thread->PostTask(FROM_HERE,
+        base::Bind(&RunSynchronousClosureWithResult<ResultType>, closure,
+                   base::Unretained(trace_event_name), base::Unretained(&event),
+                   base::Unretained(&ret)));
+    event.Wait();
+  }
+
+  return ret;
+}
+
+static void GetSessionDescription(

[perkj_chrome, 2014/11/05 10:31:02]

suggest naming GetSdpAndTypeFromSessionDescription?

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+    const webrtc::SessionDescriptionInterface* session_description,
+    std::string* sdp, std::string* type) {
+  if (session_description) {
+    session_description->ToString(sdp);
+    *type = session_description->type();
+  }
+}
+
+void GetDescription(
+    const base::Callback<const webrtc::SessionDescriptionInterface*()>&
+        description,

[perkj_chrome, 2014/11/05 10:31:02]

Can we rename |description| to something that indicate that this is a
"function"?

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Renamed to description_callback

+    std::string* sdp, std::string* type) {
+  GetSessionDescription(description.Run(), sdp, type);

[perkj_chrome, 2014/11/05 10:31:02]

Can you please join GetSessionDescription and GetDescripion? 
How about 
void GetSdpAndType(const base::Callback<const
webrtc::SessionDescriptionInterface*()>& get_description_cb,
std::string* sdp, std::string* type) {
webrtc::SessionDescriptionInterface* desc ? get_description_cb.run();
if (desc)...

}

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

 }
 
 // Converter functions from WebKit types to libjingle types.
@@ -310,7 +397,7 @@ class StatsResponse : public webrtc::StatsObserver {
   void OnComplete(const StatsReports& reports) override {
     DCHECK(signaling_thread_checker_.CalledOnValidThread());
     TRACE_EVENT0("webrtc", "StatsResponse::OnComplete");
-    // TODO(tommi): Get rid of these string copies some how.
+    // TODO(tommi): Get rid of these string copies somehow.
     // We can't use webkit objects directly since they use a single threaded
     // heap allocator.
     scoped_ptr<std::vector<StatsReportCopyable>> report_copies(
@@ -617,12 +704,10 @@ class RTCPeerConnectionHandler::Observer
 
 RTCPeerConnectionHandler::RTCPeerConnectionHandler(
     blink::WebRTCPeerConnectionHandlerClient* client,
-    PeerConnectionDependencyFactory* dependency_factory,
-    const scoped_refptr<base::SingleThreadTaskRunner>& signaling_thread)
+    PeerConnectionDependencyFactory* dependency_factory)
     : client_(client),
       dependency_factory_(dependency_factory),
       frame_(NULL),
-      signaling_thread_(signaling_thread),
       num_data_channels_created_(0),
       num_local_candidates_ipv4_(0),
       num_local_candidates_ipv6_(0),
@@ -750,6 +835,7 @@ void RTCPeerConnectionHandler::createOffer(
           weak_factory_.GetWeakPtr(), peer_connection_tracker_,
           PeerConnectionTracker::ACTION_CREATE_OFFER));
 
+  // TODO(tommi): Do this asynchronously via e.g. PostTaskAndReply.
   RTCMediaConstraints constraints(options);
   native_peer_connection_->CreateOffer(description_request.get(), &constraints);
 
@@ -769,6 +855,7 @@ void RTCPeerConnectionHandler::createOffer(
           weak_factory_.GetWeakPtr(), peer_connection_tracker_,
           PeerConnectionTracker::ACTION_CREATE_OFFER));
 
+  // TODO(tommi): Do this asynchronously via e.g. PostTaskAndReply.
   RTCMediaConstraints constraints;
   ConvertOfferOptionsToConstraints(options, &constraints);
   native_peer_connection_->CreateOffer(description_request.get(), &constraints);
@@ -787,6 +874,7 @@ void RTCPeerConnectionHandler::createAnswer(
           base::ThreadTaskRunnerHandle::Get(), request,
           weak_factory_.GetWeakPtr(), peer_connection_tracker_,
           PeerConnectionTracker::ACTION_CREATE_ANSWER));
+  // TODO(tommi): Do this asynchronously via e.g. PostTaskAndReply.
   RTCMediaConstraints constraints(options);
   native_peer_connection_->CreateAnswer(description_request.get(),
                                         &constraints);
@@ -830,8 +918,12 @@ void RTCPeerConnectionHandler::setLocalDescription(
           weak_factory_.GetWeakPtr(), peer_connection_tracker_,
           PeerConnectionTracker::ACTION_SET_LOCAL_DESCRIPTION));
 
-  // TODO(tommi): Run this on the signaling thread.
-  native_peer_connection_->SetLocalDescription(set_request.get(), native_desc);
+  signaling_thread()->PostTask(FROM_HERE,
+      base::Bind(&RunClosureWithTrace,
+          base::Bind(&webrtc::PeerConnectionInterface::SetLocalDescription,
+              native_peer_connection_, set_request,
+              base::Unretained(native_desc)),
+          "SetLocalDescription"));
 }
 
 void RTCPeerConnectionHandler::setRemoteDescription(
@@ -867,30 +959,52 @@ void RTCPeerConnectionHandler::setRemoteDescription(
           base::ThreadTaskRunnerHandle::Get(), request,
           weak_factory_.GetWeakPtr(), peer_connection_tracker_,
           PeerConnectionTracker::ACTION_SET_REMOTE_DESCRIPTION));
-  // TODO(tommi): Run this on the signaling thread.
-  native_peer_connection_->SetRemoteDescription(set_request.get(), native_desc);
+  signaling_thread()->PostTask(FROM_HERE,
+      base::Bind(&RunClosureWithTrace,
+          base::Bind(&webrtc::PeerConnectionInterface::SetRemoteDescription,
+              native_peer_connection_, set_request,
+              base::Unretained(native_desc)),
+          "SetRemoteDescription"));
 }
 
 blink::WebRTCSessionDescription
 RTCPeerConnectionHandler::localDescription() {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::localDescription");
-  const webrtc::SessionDescriptionInterface* native_desc =
-      native_peer_connection_->local_description();
-  blink::WebRTCSessionDescription description =
-      CreateWebKitSessionDescription(native_desc);
-  return description;
+
+  // Since local_description returns a pointer to a non-reference-counted object
+  // that lives on the signaling thread, we cannot fetch a pointer to it and use
+  // it directly here. Instead, we access the object completely on the signaling
+  // thread.
+  std::string sdp, type;
+  base::Callback<const webrtc::SessionDescriptionInterface*()> description =

[perkj_chrome, 2014/11/05 10:31:02]

name get_description_cb? 

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done. changed to description_cb (since the function name doesn't have a get_)

+      base::Bind(&webrtc::PeerConnectionInterface::local_description,
+                 native_peer_connection_);
+  RunSynchronousClosureOnThread(
+      base::Bind(&GetDescription, description, base::Unretained(&sdp),
+            base::Unretained(&type)),
+      "localDescription");
+
+  return CreateWebKitSessionDescription(sdp, type);
 }
 
 blink::WebRTCSessionDescription
 RTCPeerConnectionHandler::remoteDescription() {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::remoteDescription");
-  const webrtc::SessionDescriptionInterface* native_desc =
-      native_peer_connection_->remote_description();
-  blink::WebRTCSessionDescription description =
-      CreateWebKitSessionDescription(native_desc);
-  return description;
+  // Since local_description returns a pointer to a non-reference-counted object
+  // that lives on the signaling thread, we cannot fetch a pointer to it and use
+  // it directly here. Instead, we access the object completely on the signaling
+  // thread.
+  std::string sdp, type;
+  auto desc = base::Bind(&webrtc::PeerConnectionInterface::remote_description,

[perkj_chrome, 2014/11/05 10:31:02]

please use base::Callback<const webrtc::SessionDescriptionInterface*()> as in
localDescription.

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+                       native_peer_connection_);

[perkj_chrome, 2014/11/05 10:31:02]

nit: indentation

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+  RunSynchronousClosureOnThread(
+      base::Bind(&GetDescription, desc, base::Unretained(&sdp),
+          base::Unretained(&type)),
+      "remoteDescription");
+
+  return CreateWebKitSessionDescription(sdp, type);
 }
 
 bool RTCPeerConnectionHandler::updateICE(
@@ -915,6 +1029,9 @@ bool RTCPeerConnectionHandler::addICECandidate(
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::addICECandidate");
   // Libjingle currently does not accept callbacks for addICECandidate.
   // For that reason we are going to call callbacks from here.
+
+  // TODO(tommi): Instead of calling addICECandidate here, we can do a
+  // PostTaskAndReply kind of a thing.
   bool result = addICECandidate(candidate);
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
@@ -1014,6 +1131,7 @@ void RTCPeerConnectionHandler::removeStream(
     }
   }
   DCHECK(webrtc_stream.get());
+  // TODO(tommi): Make this async (PostTaskAndReply).
   native_peer_connection_->RemoveStream(webrtc_stream.get());
 
   if (peer_connection_tracker_) {
@@ -1061,7 +1179,7 @@ void RTCPeerConnectionHandler::GetStats(
     const std::string& track_id,
     blink::WebMediaStreamSource::Type track_type) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  signaling_thread_->PostTask(FROM_HERE,
+  signaling_thread()->PostTask(FROM_HERE,
       base::Bind(&GetStatsOnSignalingThread, native_peer_connection_, level,
                  make_scoped_refptr(observer), track_id, track_type));
 }
@@ -1120,9 +1238,10 @@ blink::WebRTCDTMFSenderHandler* RTCPeerConnectionHandler::createDTMFSender(
     return nullptr;
   }
 
-  webrtc::AudioTrackInterface* audio_track = native_track->GetAudioAdapter();
+  scoped_refptr<webrtc::AudioTrackInterface> audio_track =
+      native_track->GetAudioAdapter();
   rtc::scoped_refptr<webrtc::DtmfSenderInterface> sender(
-      native_peer_connection_->CreateDtmfSender(audio_track));
+      native_peer_connection_->CreateDtmfSender(audio_track.get()));
   if (!sender) {
     DLOG(ERROR) << "Could not create native DTMF sender.";
     return nullptr;
@@ -1137,9 +1256,14 @@ void RTCPeerConnectionHandler::stop() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "RTCPeerConnectionHandler::stop";
 
+  if (!client_)
+    return;  // Already stopped.
+
   if (peer_connection_tracker_)
     peer_connection_tracker_->TrackStop(this);
+
   native_peer_connection_->Close();
+
   // The client_ pointer is not considered valid after this point and no further
   // callbacks must be made.
   client_ = nullptr;
@@ -1339,4 +1463,36 @@ RTCPeerConnectionHandler::CreateNativeSessionDescription(
   return native_desc;
 }
 
+scoped_refptr<base::SingleThreadTaskRunner>
+RTCPeerConnectionHandler::signaling_thread() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return dependency_factory_->GetWebRtcSignalingThread();
+}
+
+void RTCPeerConnectionHandler::RunSynchronousClosureOnThread(

[perkj_chrome, 2014/11/05 10:31:01]

Name RunSynchronousClosureOnSignalingThread?

[tommi (sloooow) - chröme, 2014/11/05 10:50:39]

Done.

+    const base::Closure& closure,
+    const char* trace_event_name) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  scoped_refptr<base::SingleThreadTaskRunner> thread(signaling_thread());
+  if (!thread.get() || thread->BelongsToCurrentThread()) {
+    TRACE_EVENT0("webrtc", trace_event_name);
+    closure.Run();
+  } else {
+    base::WaitableEvent event(false, false);
+    thread->PostTask(FROM_HERE,
+        base::Bind(&RunSynchronousClosure, closure,
+                   base::Unretained(trace_event_name),
+                   base::Unretained(&event)));
+    event.Wait();
+  }
+}
+
+bool RTCPeerConnectionHandler::RunSynchronousClosureOnThreadWithResult(

[perkj_chrome, 2014/11/05 10:31:01]

unused?

[tommi (sloooow) - chröme, 2014/11/05 10:50:38]

Removed

+    const base::Callback<bool()>& closure,
+    const char* trace_event_name) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return RunSynchronousClosureOnThreadWithResultT<bool>(signaling_thread(),
+      closure, trace_event_name);
+}
+
 }  // namespace content