From 4163317283bea03c9eb1edfe0c1f5fe6029b8704 Mon Sep 17 00:00:00 2001 From: Guido Urdaneta Date: Thu, 23 May 2019 20:12:29 +0200 Subject: [PATCH] [PeerConnection::AddIceCandidate()] Use mid to look up contents in remote descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this CL, only the mline index of an ice candidate was used to look up contents. However, due to recent changes, it is possible that no mline index is specified, but that only a mid is specified. No mline index is indicated with a -1 value. This CL makes sure the mid is used if no mline index is given. Bug: chromium:965483 Change-Id: I8962e71acb386f7b50349802f27358ba24c11921 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138075 Commit-Queue: Guido Urdaneta Reviewed-by: Steve Anton Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#28045} --- pc/jsep_session_description.cc | 15 +++++- pc/peer_connection.cc | 75 ++++++++++++++++++++---------- pc/peer_connection.h | 3 ++ pc/peer_connection_ice_unittest.cc | 2 +- 4 files changed, 67 insertions(+), 28 deletions(-) diff --git a/pc/jsep_session_description.cc b/pc/jsep_session_description.cc index 90b12a4125..d9c1fad94d 100644 --- a/pc/jsep_session_description.cc +++ b/pc/jsep_session_description.cc @@ -207,7 +207,7 @@ bool JsepSessionDescription::Initialize( bool JsepSessionDescription::AddCandidate( const IceCandidateInterface* candidate) { - if (!candidate || candidate->sdp_mline_index() < 0) + if (!candidate) return false; size_t mediasection_index = 0; if (!GetMediasectionIndex(candidate, &mediasection_index)) { @@ -291,7 +291,18 @@ bool JsepSessionDescription::GetMediasectionIndex( if (!candidate || !index) { return false; } - *index = static_cast(candidate->sdp_mline_index()); + + // If the candidate has no valid mline index or sdp_mid, it is impossible + // to find a match. + if (candidate->sdp_mid().empty() && + (candidate->sdp_mline_index() < 0 || + static_cast(candidate->sdp_mline_index()) >= + description_->contents().size())) { + return false; + } + + if (candidate->sdp_mline_index() >= 0) + *index = static_cast(candidate->sdp_mline_index()); if (description_ && !candidate->sdp_mid().empty()) { bool found = false; // Try to match the sdp_mid with content name. diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index deaff5d613..ca66a095b0 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -6172,21 +6172,18 @@ bool PeerConnection::UseCandidatesInSessionDescription( } bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) { - size_t mediacontent_index = static_cast(candidate->sdp_mline_index()); - size_t remote_content_size = - remote_description()->description()->contents().size(); - if (mediacontent_index >= remote_content_size) { - RTC_LOG(LS_ERROR) << "UseCandidate: Invalid candidate media index."; + RTCErrorOr result = + FindContentInfo(remote_description(), candidate); + if (!result.ok()) { + RTC_LOG(LS_ERROR) << "UseCandidate: Invalid candidate. " + << result.error().message(); return false; } - - cricket::ContentInfo content = - remote_description()->description()->contents()[mediacontent_index]; std::vector candidates; candidates.push_back(candidate->candidate()); // Invoking BaseSession method to handle remote candidates. - RTCError error = - transport_controller_->AddRemoteCandidates(content.name, candidates); + RTCError error = transport_controller_->AddRemoteCandidates( + result.value()->name, candidates); if (error.ok()) { // Candidates successfully submitted for checking. if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew || @@ -6209,6 +6206,42 @@ bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) { return true; } +RTCErrorOr PeerConnection::FindContentInfo( + const SessionDescriptionInterface* description, + const IceCandidateInterface* candidate) { + if (candidate->sdp_mline_index() >= 0) { + size_t mediacontent_index = + static_cast(candidate->sdp_mline_index()); + size_t content_size = description->description()->contents().size(); + if (mediacontent_index < content_size) { + return &description->description()->contents()[mediacontent_index]; + } else { + return RTCError(RTCErrorType::INVALID_RANGE, + "Media line index (" + + rtc::ToString(candidate->sdp_mline_index()) + + ") out of range (number of mlines: " + + rtc::ToString(content_size) + ")."); + } + } else if (!candidate->sdp_mid().empty()) { + auto& contents = description->description()->contents(); + auto it = absl::c_find_if( + contents, [candidate](const cricket::ContentInfo& content_info) { + return content_info.mid() == candidate->sdp_mid(); + }); + if (it == contents.end()) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "Mid " + candidate->sdp_mid() + + " specified but no media section with that mid found."); + } else { + return &*it; + } + } + + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Neither sdp_mline_index nor sdp_mid specified."); +} + void PeerConnection::RemoveUnusedChannels(const SessionDescription* desc) { // Destroy video channel first since it may have a pointer to the // voice channel. @@ -6838,26 +6871,18 @@ bool PeerConnection::ReadyToUseRemoteCandidate( 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) { - RTC_LOG(LS_ERROR) - << "ReadyToUseRemoteCandidate: Invalid candidate media index " - << mediacontent_index; + RTCErrorOr result = + FindContentInfo(current_remote_desc, candidate); + if (!result.ok()) { + RTC_LOG(LS_ERROR) << "ReadyToUseRemoteCandidate: Invalid candidate. " + << result.error().message(); *valid = false; return false; } - cricket::ContentInfo content = - current_remote_desc->description()->contents()[mediacontent_index]; - - const std::string transport_name = GetTransportName(content.name); - if (transport_name.empty()) { - return false; - } - return true; + std::string transport_name = GetTransportName(result.value()->name); + return !transport_name.empty(); } bool PeerConnection::SrtpRequired() const { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 86fc1a8dd3..d9c625cea1 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -911,6 +911,9 @@ class PeerConnection : public PeerConnectionInternal, // Uses |candidate| in this session. bool UseCandidate(const IceCandidateInterface* candidate) RTC_RUN_ON(signaling_thread()); + RTCErrorOr FindContentInfo( + const SessionDescriptionInterface* description, + const IceCandidateInterface* candidate) RTC_RUN_ON(signaling_thread()); // Deletes the corresponding channel of contents that don't exist in |desc|. // |desc| can be null. This means that all channels are deleted. void RemoveUnusedChannels(const cricket::SessionDescription* desc) diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 03c11283bb..afc859434f 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -58,7 +58,7 @@ class PeerConnectionWrapperForIceTest : public PeerConnectionWrapper { const auto& first_content = desc->contents()[0]; candidate->set_transport_name(first_content.name); std::unique_ptr jsep_candidate = - CreateIceCandidate(first_content.name, 0, *candidate); + CreateIceCandidate(first_content.name, -1, *candidate); return pc()->AddIceCandidate(jsep_candidate.get()); }