Track pending ICE restarts independently for different media sections.

webrtc/api/webrtcsessiondescriptionfactory.cc

Index: webrtc/api/webrtcsessiondescriptionfactory.cc
diff --git a/webrtc/api/webrtcsessiondescriptionfactory.cc b/webrtc/api/webrtcsessiondescriptionfactory.cc
index 19ee1e6e03c0b920de0ba4fe9a81284c9d254a07..aa929d3347713fdd1deae1854af9aebbd5d7cfe3 100644
--- a/webrtc/api/webrtcsessiondescriptionfactory.cc
+++ b/webrtc/api/webrtcsessiondescriptionfactory.cc
@@ -95,18 +95,27 @@ void WebRtcIdentityRequestObserver::OnSuccess(
 // static
 void WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
     const SessionDescriptionInterface* source_desc,
+    const std::string& content_name,
     SessionDescriptionInterface* dest_desc) {
-  if (!source_desc)
+  if (!source_desc) {
     return;
-  for (size_t m = 0; m < source_desc->number_of_mediasections() &&
-                     m < dest_desc->number_of_mediasections(); ++m) {
-    const IceCandidateCollection* source_candidates =
-        source_desc->candidates(m);
-    const IceCandidateCollection* dest_candidates = dest_desc->candidates(m);
-    for  (size_t n = 0; n < source_candidates->count(); ++n) {
-      const IceCandidateInterface* new_candidate = source_candidates->at(n);
-      if (!dest_candidates->HasCandidate(new_candidate))
-        dest_desc->AddCandidate(source_candidates->at(n));
+  }
+  const cricket::ContentInfos& contents =
+      source_desc->description()->contents();
+  const cricket::ContentInfo* cinfo =
+      source_desc->description()->GetContentByName(content_name);
+  if (!cinfo) {
+    return;
+  }
+  size_t mediasection_index = static_cast<int>(cinfo - &contents[0]);
+  const IceCandidateCollection* source_candidates =
+      source_desc->candidates(mediasection_index);
+  const IceCandidateCollection* dest_candidates =
+      dest_desc->candidates(mediasection_index);
+  for (size_t n = 0; n < source_candidates->count(); ++n) {
+    const IceCandidateInterface* new_candidate = source_candidates->at(n);
+    if (!dest_candidates->HasCandidate(new_candidate)) {
+      dest_desc->AddCandidate(source_candidates->at(n));
     }
   }
 }
@@ -374,42 +383,38 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer(
                                        "Failed to initialize the offer.");
     return;
   }
-  if (session_->local_description() &&
-      !request.options.audio_transport_options.ice_restart &&
-      !request.options.video_transport_options.ice_restart &&
-      !request.options.data_transport_options.ice_restart) {
-    // Include all local ice candidates in the SessionDescription unless
-    // the an ice restart has been requested.
-    CopyCandidatesFromSessionDescription(session_->local_description(), offer);
+  if (session_->local_description()) {
+    for (const cricket::ContentInfo& content :

[pthatcher1, 2016/02/17 06:46:59]

Can you make a private helper method for this?

const std::vector<cricket::ContentInfo>& local_contents() {
  return session_->local_description()->description()->contents();
}

+         session_->local_description()->description()->contents()) {
+      // Include all local ICE candidates in the SessionDescription unless
+      // the remote peer has requested an ICE restart.
+      if (!request.options.transport_options[content.name].ice_restart) {
+        CopyCandidatesFromSessionDescription(session_->local_description(),
+                                             content.name, offer);
+      }
+    }
   }
   PostCreateSessionDescriptionSucceeded(request.observer, offer);
 }
 
 void WebRtcSessionDescriptionFactory::InternalCreateAnswer(
     CreateSessionDescriptionRequest request) {
-  // According to http://tools.ietf.org/html/rfc5245#section-9.2.1.1
-  // an answer should also contain new ice ufrag and password if an offer has
-  // been received with new ufrag and password.
-  request.options.audio_transport_options.ice_restart =
-      session_->IceRestartPending();
-  request.options.video_transport_options.ice_restart =
-      session_->IceRestartPending();
-  request.options.data_transport_options.ice_restart =
-      session_->IceRestartPending();
-  // We should pass current ssl role to the transport description factory, if
-  // there is already an existing ongoing session.
-  rtc::SSLRole ssl_role;
-  if (session_->GetSslRole(session_->voice_channel(), &ssl_role)) {
-    request.options.audio_transport_options.prefer_passive_role =
-        (rtc::SSL_SERVER == ssl_role);
-  }
-  if (session_->GetSslRole(session_->video_channel(), &ssl_role)) {
-    request.options.video_transport_options.prefer_passive_role =
-        (rtc::SSL_SERVER == ssl_role);
-  }
-  if (session_->GetSslRole(session_->data_channel(), &ssl_role)) {
-    request.options.data_transport_options.prefer_passive_role =
-        (rtc::SSL_SERVER == ssl_role);
+  if (session_->remote_description()) {
+    for (const cricket::ContentInfo& content :
+         session_->remote_description()->description()->contents()) {
+      // According to http://tools.ietf.org/html/rfc5245#section-9.2.1.1
+      // an answer should also contain new ICE ufrag and password if an offer
+      // has been received with new ufrag and password.
+      request.options.transport_options[content.name].ice_restart =
+          session_->IceRestartPending(content.name);

[pthatcher1, 2016/02/17 06:46:58]

I can't wait until we can stick all per-content-name/MID info in transceivers
instead of having so many methods that take a content name.

+      // We should pass the current SSL role to the transport description
+      // factory, if there is already an existing ongoing session.
+      rtc::SSLRole ssl_role;
+      if (session_->GetSslRole(session_->GetChannel(content.name), &ssl_role)) {
+        request.options.transport_options[content.name].prefer_passive_role =
+            (rtc::SSL_SERVER == ssl_role);
+      }
+    }
   }
 
   cricket::SessionDescription* desc(session_desc_factory_.CreateAnswer(
@@ -436,15 +441,17 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer(
                                        "Failed to initialize the answer.");
     return;
   }
-  if (session_->local_description() &&
-      !request.options.audio_transport_options.ice_restart &&
-      !request.options.video_transport_options.ice_restart &&
-      !request.options.data_transport_options.ice_restart) {
-    // Include all local ice candidates in the SessionDescription unless
-    // the remote peer has requested an ice restart.
-    CopyCandidatesFromSessionDescription(session_->local_description(), answer);
+  if (session_->local_description()) {
+    for (const cricket::ContentInfo& content :
+         session_->local_description()->description()->contents()) {
+      // Include all local ICE candidates in the SessionDescription unless
+      // the remote peer has requested an ICE restart.
+      if (!request.options.transport_options[content.name].ice_restart) {
+        CopyCandidatesFromSessionDescription(session_->local_description(),
+                                             content.name, answer);
+      }
+    }

[pthatcher1, 2016/02/17 06:46:58]

I don't like seeing this duplicated.  Can we make a
MaybeCopyCandidatesFromLocalDescription(JsepSessionDescription* desc) that has
this logic, and then use it in the two places?

   }
-  session_->ResetIceRestartLatch();

[pthatcher1, 2016/02/17 06:46:59]

Well, this was a terrible place to have this before.  You could only call
createAnswer once, and then the next time it wouldn't reset the ufrag/pwd? 

   PostCreateSessionDescriptionSucceeded(request.observer, answer);
 }