From 31e06cb63d4f7eb7f458e4a0e2ee4e3665674fd7 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 26 Feb 2021 09:23:53 +0100 Subject: [PATCH] addIceCandidate: prefer ice candidate sdpMid over sdpMLineIndex as described in JSEP https://tools.ietf.org/html/rfc8829#section-3.5.2.1 If the MID field is present in a received IceCandidate, it MUST be used for identification; otherwise, the "m=" section index is used instead. BUG=webrtc:12479 Change-Id: I688a5e59024fe8cc6a170c216c6f14536084cfb9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/208100 Reviewed-by: Harald Alvestrand Reviewed-by: Artem Titov Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/master@{#33357} --- pc/peer_connection_ice_unittest.cc | 20 ++++++++++++++++++++ pc/sdp_offer_answer.cc | 28 ++++++++++++++-------------- test/pc/e2e/sdp/sdp_changer.cc | 11 ++++++++--- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 6fabd51f3a..9a9e2e6225 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -1452,4 +1452,24 @@ TEST_P(PeerConnectionIceTest, CloseDoesNotTransitionGatheringStateToComplete) { pc->pc()->ice_gathering_state()); } +TEST_P(PeerConnectionIceTest, PrefersMidOverMLineIndex) { + const SocketAddress kCalleeAddress("1.1.1.1", 1111); + + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + // |candidate.transport_name()| is empty. + cricket::Candidate candidate = CreateLocalUdpCandidate(kCalleeAddress); + auto* audio_content = cricket::GetFirstAudioContent( + caller->pc()->local_description()->description()); + std::unique_ptr ice_candidate = + CreateIceCandidate(audio_content->name, 65535, candidate); + EXPECT_TRUE(caller->pc()->AddIceCandidate(ice_candidate.get())); + EXPECT_TRUE(caller->pc()->RemoveIceCandidates({candidate})); +} + } // namespace webrtc diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 4317bca132..d10f6fdf85 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -4532,20 +4532,7 @@ bool SdpOfferAnswerHandler::ReadyToUseRemoteCandidate( RTCErrorOr SdpOfferAnswerHandler::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()) { + if (!candidate->sdp_mid().empty()) { auto& contents = description->description()->contents(); auto it = absl::c_find_if( contents, [candidate](const cricket::ContentInfo& content_info) { @@ -4559,6 +4546,19 @@ RTCErrorOr SdpOfferAnswerHandler::FindContentInfo( } else { return &*it; } + } else 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) + ")."); + } } return RTCError(RTCErrorType::INVALID_PARAMETER, diff --git a/test/pc/e2e/sdp/sdp_changer.cc b/test/pc/e2e/sdp/sdp_changer.cc index f2aeb1b92d..ac0c46ae3c 100644 --- a/test/pc/e2e/sdp/sdp_changer.cc +++ b/test/pc/e2e/sdp/sdp_changer.cc @@ -524,9 +524,11 @@ SignalingInterceptor::PatchOffererIceCandidates( context_.simulcast_infos_by_mid.find(candidate->sdp_mid()); if (simulcast_info_it != context_.simulcast_infos_by_mid.end()) { // This is candidate for simulcast section, so it should be transformed - // into candidates for replicated sections - out.push_back(CreateIceCandidate(simulcast_info_it->second->rids[0], 0, - candidate->candidate())); + // into candidates for replicated sections. The sdpMLineIndex is set to + // -1 and ignored if the rid is present. + for (auto rid : simulcast_info_it->second->rids) { + out.push_back(CreateIceCandidate(rid, -1, candidate->candidate())); + } } else { out.push_back(CreateIceCandidate(candidate->sdp_mid(), candidate->sdp_mline_index(), @@ -550,6 +552,9 @@ SignalingInterceptor::PatchAnswererIceCandidates( // section. out.push_back(CreateIceCandidate(simulcast_info_it->second->mid, 0, candidate->candidate())); + } else if (context_.simulcast_infos_by_rid.size()) { + // When using simulcast and bundle, put everything on the first m-line. + out.push_back(CreateIceCandidate("", 0, candidate->candidate())); } else { out.push_back(CreateIceCandidate(candidate->sdp_mid(), candidate->sdp_mline_index(),