fix

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6720 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/webrtcsession.cc

@@ -892,36 +892,16 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) {
     return false;
   }
 
-  cricket::TransportProxy* transport_proxy = NULL;
+  bool valid = false;
-  if (remote_description()) {
+  if (!ReadyToUseRemoteCandidate(candidate, NULL, &valid)) {
-    size_t mediacontent_index =
+    if (valid) {
-        static_cast<size_t>(candidate->sdp_mline_index());
+      LOG(LS_INFO) << "ProcessIceMessage: Candidate saved";
-    size_t remote_content_size =
-        BaseSession::remote_description()->contents().size();
-    if (mediacontent_index >= remote_content_size) {
-      LOG(LS_ERROR)
-          << "ProcessIceMessage: Invalid candidate media index.";
-      return false;
-    }
-
-    cricket::ContentInfo content =
-        BaseSession::remote_description()->contents()[mediacontent_index];
-    transport_proxy = GetTransportProxy(content.name);
-  }
-
-  // We need to check the local/remote description for the Transport instead of
-  // the session, because a new Transport added during renegotiation may have
-  // them unset while the session has them set from the previou negotiation. Not
-  // doing so may trigger the auto generation of transport description and mess
-  // up DTLS identity information, ICE credential, etc.
-  if (!transport_proxy || !(transport_proxy->local_description_set() &&
-                            transport_proxy->remote_description_set())) {
-    LOG(LS_INFO) << "ProcessIceMessage: Local/Remote description not set "
-                 << "on the Transport, save the candidate for later use.";
       saved_candidates_.push_back(
-        new JsepIceCandidate(candidate->sdp_mid(), candidate->sdp_mline_index(),
+          new JsepIceCandidate(candidate->sdp_mid(),
+                               candidate->sdp_mline_index(),
                                candidate->candidate()));
-    return true;
+    }
+    return valid;
   }
 
   // Add this candidate to the remote session description.
@@ -1386,10 +1366,24 @@ bool WebRtcSession::UseCandidatesInSessionDescription(
   if (!remote_desc)
     return true;
   bool ret = true;
+
   for (size_t m = 0; m < remote_desc->number_of_mediasections(); ++m) {
     const IceCandidateCollection* candidates = remote_desc->candidates(m);
     for  (size_t n = 0; n < candidates->count(); ++n) {
-      ret = UseCandidate(candidates->at(n));
+      const IceCandidateInterface* candidate = candidates->at(n);
+      bool valid = false;
+      if (!ReadyToUseRemoteCandidate(candidate, remote_desc, &valid)) {
+        if (valid) {
+          LOG(LS_INFO) << "UseCandidatesInSessionDescription: Candidate saved.";
+          saved_candidates_.push_back(
+              new JsepIceCandidate(candidate->sdp_mid(),
+                                   candidate->sdp_mline_index(),
+                                   candidate->candidate()));
+        }
+        continue;
+      }
+
+      ret = UseCandidate(candidate);
       if (!ret)
         break;
     }
@@ -1696,4 +1690,42 @@ std::string WebRtcSession::GetSessionErrorMsg() {
   return desc.str();
 }
 
+// We need to check the local/remote description for the Transport instead of
+// the session, because a new Transport added during renegotiation may have
+// them unset while the session has them set from the previous negotiation.
+// Not doing so may trigger the auto generation of transport description and
+// mess up DTLS identity information, ICE credential, etc.
+bool WebRtcSession::ReadyToUseRemoteCandidate(
+    const IceCandidateInterface* candidate,
+    const SessionDescriptionInterface* remote_desc,
+    bool* valid) {
+  *valid = true;;
+  cricket::TransportProxy* transport_proxy = NULL;
+
+  const SessionDescriptionInterface* current_remote_desc =
+      remote_desc ? remote_desc : remote_description();
+
+  if (!current_remote_desc)
+    return false;
+
+  size_t mediacontent_index =
+      static_cast<size_t>(candidate->sdp_mline_index());
+  size_t remote_content_size =
+      current_remote_desc->description()->contents().size();
+  if (mediacontent_index >= remote_content_size) {
+    LOG(LS_ERROR)
+        << "ReadyToUseRemoteCandidate: Invalid candidate media index.";
+
+    *valid = false;
+    return false;
+  }
+
+  cricket::ContentInfo content =
+      current_remote_desc->description()->contents()[mediacontent_index];
+  transport_proxy = GetTransportProxy(content.name);
+
+  return transport_proxy && transport_proxy->local_description_set() &&
+      transport_proxy->remote_description_set();
+}
+
 }  // namespace webrtc

talk/app/webrtc/webrtcsession.h

@@ -309,6 +309,14 @@ class WebRtcSession : public cricket::BaseSession,
   bool ValidateDtlsSetupAttribute(const cricket::SessionDescription* desc,
                                   Action action);
 
+  // Returns true if we are ready to push down the remote candidate.
+  // |remote_desc| is the new remote description, or NULL if the current remote
+  // description should be used. Output |valid| is true if the candidate media
+  // index is valid.
+  bool ReadyToUseRemoteCandidate(const IceCandidateInterface* candidate,
+                                 const SessionDescriptionInterface* remote_desc,
+                                 bool* valid);
+
   std::string GetSessionErrorMsg();
 
   talk_base::scoped_ptr<cricket::VoiceChannel> voice_channel_;

talk/app/webrtc/webrtcsession_unittest.cc

@@ -3246,6 +3246,66 @@ TEST_F(WebRtcSessionTest, TestSuspendBelowMinBitrateConstraint) {
       video_options.suspend_below_min_bitrate.GetWithDefaultIfUnset(false));
 }
 
+// Tests that we can renegotiate new media content with ICE candidates in the
+// new remote SDP.
+TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesInSdp) {
+  MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp);
+  InitWithDtls();
+  SetFactoryDtlsSrtp();
+
+  mediastream_signaling_.UseOptionsAudioOnly();
+  SessionDescriptionInterface* offer = CreateOffer(NULL);
+  SetLocalDescriptionWithoutError(offer);
+
+  SessionDescriptionInterface* answer = CreateRemoteAnswer(offer);
+  SetRemoteDescriptionWithoutError(answer);
+
+  cricket::MediaSessionOptions options;
+  options.has_video = true;
+  offer = CreateRemoteOffer(options, cricket::SEC_DISABLED);
+
+  cricket::Candidate candidate1;
+  candidate1.set_address(talk_base::SocketAddress("1.1.1.1", 5000));
+  candidate1.set_component(1);
+  JsepIceCandidate ice_candidate(kMediaContentName1, kMediaContentIndex1,
+                                 candidate1);
+  EXPECT_TRUE(offer->AddCandidate(&ice_candidate));
+  SetRemoteDescriptionWithoutError(offer);
+
+  answer = CreateAnswer(NULL);
+  SetLocalDescriptionWithoutError(answer);
+}
+
+// Tests that we can renegotiate new media content with ICE candidates separated
+// from the remote SDP.
+TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesSeparated) {
+  MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp);
+  InitWithDtls();
+  SetFactoryDtlsSrtp();
+
+  mediastream_signaling_.UseOptionsAudioOnly();
+  SessionDescriptionInterface* offer = CreateOffer(NULL);
+  SetLocalDescriptionWithoutError(offer);
+
+  SessionDescriptionInterface* answer = CreateRemoteAnswer(offer);
+  SetRemoteDescriptionWithoutError(answer);
+
+  cricket::MediaSessionOptions options;
+  options.has_video = true;
+  offer = CreateRemoteOffer(options, cricket::SEC_DISABLED);
+  SetRemoteDescriptionWithoutError(offer);
+
+  cricket::Candidate candidate1;
+  candidate1.set_address(talk_base::SocketAddress("1.1.1.1", 5000));
+  candidate1.set_component(1);
+  JsepIceCandidate ice_candidate(kMediaContentName1, kMediaContentIndex1,
+                                 candidate1);
+  EXPECT_TRUE(session_->ProcessIceMessage(&ice_candidate));
+
+  answer = CreateAnswer(NULL);
+  SetLocalDescriptionWithoutError(answer);
+}
+
 // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled.  That test
 // currently fails because upon disconnection and reconnection OnIceComplete is
 // called more than once without returning to IceGatheringGathering.