diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 9c27be52a9..ddc8876291 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -892,36 +892,16 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) { return false; } - cricket::TransportProxy* transport_proxy = NULL; - if (remote_description()) { - size_t mediacontent_index = - static_cast(candidate->sdp_mline_index()); - 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; + bool valid = false; + if (!ReadyToUseRemoteCandidate(candidate, NULL, &valid)) { + if (valid) { + LOG(LS_INFO) << "ProcessIceMessage: Candidate saved"; + saved_candidates_.push_back( + new JsepIceCandidate(candidate->sdp_mid(), + candidate->sdp_mline_index(), + candidate->candidate())); } - - 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(), - 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(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 diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h index 3ffea8ffac..63e0cc4b99 100644 --- a/talk/app/webrtc/webrtcsession.h +++ b/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 voice_channel_; diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 956f8a66fb..a1d48cf5e0 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/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.