Track pending ICE restarts independently for different media sections.

talk/app/webrtc/webrtcsessiondescriptionfactory.cc

Index: talk/app/webrtc/webrtcsessiondescriptionfactory.cc
diff --git a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
index f08b77eb40d4238e18f3532adb60f11e79a77e06..4c4c48b552affdae13c0b596b2e9a26c95bf4c9f 100644
--- a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
+++ b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
@@ -112,19 +112,27 @@ void WebRtcIdentityRequestObserver::OnSuccess(
 // static
 void WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
     const SessionDescriptionInterface* source_desc,
+    cricket::MediaType media_type,
     SessionDescriptionInterface* dest_desc) {
-  if (!source_desc)
+  if (!source_desc) {
+    return;
+  }
+  const cricket::ContentInfos& contents =
+      source_desc->description()->contents();
+  const cricket::ContentInfo* cinfo =
+      GetFirstMediaContent(contents, media_type);
+  if (!cinfo) {
     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));
-    }
+  }
+  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));
   }
 }
 
@@ -391,13 +399,21 @@ 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()) {
+    // Include all local ICE candidates in the SessionDescription unless
+    // the remote peer has requested an ICE restart.
+    if (!request.options.audio_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_AUDIO, offer);
+    }
+    if (!request.options.video_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_VIDEO, offer);
+    }
+    if (!request.options.data_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_DATA, offer);
+    }
   }
   PostCreateSessionDescriptionSucceeded(request.observer, offer);
 }
@@ -408,11 +424,11 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer(
   // 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();
+      session_->AudioIceRestartPending();
   request.options.video_transport_options.ice_restart =
-      session_->IceRestartPending();
+      session_->VideoIceRestartPending();
   request.options.data_transport_options.ice_restart =
-      session_->IceRestartPending();
+      session_->DataIceRestartPending();
   // We should pass current ssl role to the transport description factory, if
   // there is already an existing ongoing session.
   rtc::SSLRole ssl_role;
@@ -453,15 +469,22 @@ 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()) {

[honghaiz3, 2016/02/10 01:01:48]

Does it make sense to write a method for this part to remove the code
duplication? This and the above one (starting at line 402) are pretty much
identical. 

[Taylor Brandstetter, 2016/02/10 21:18:32]

After refactoring, this section of code is smaller, so I think there's less
incentive to make a helper method.

+    // Include all local ICE candidates in the SessionDescription unless
+    // the remote peer has requested an ICE restart.
+    if (!request.options.audio_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_AUDIO, answer);
+    }
+    if (!request.options.video_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_VIDEO, answer);
+    }
+    if (!request.options.data_transport_options.ice_restart) {
+      CopyCandidatesFromSessionDescription(session_->local_description(),
+                                           cricket::MEDIA_TYPE_DATA, answer);
+    }
   }
-  session_->ResetIceRestartLatch();

[honghaiz3, 2016/02/10 01:01:48]

Is there a good reason to move this from here to SetLocalDescription?

[Taylor Brandstetter, 2016/02/10 21:18:32]

Yes. If the remote peer starts an ICE restart, createAnswer should continue to
generate new credentials until a new local description is set.

By resetting the latch here, if you called createAnswer twice, the second answer
would have the old credentials, which is wrong. That reminds me, I should have a
test for this.

   PostCreateSessionDescriptionSucceeded(request.observer, answer);
 }