Propagate transport name together with candidates
This change further disconnects the use of mid and the transport_name()
Candidate property and facilitates removing that property altogether.
Bug: webrtc:406795492, webrtc:8395
Change-Id: Ia834b51e9d257127c0620bff489a93ca707bc528
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/394400
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45207}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index c5810b6..d584d45 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -1393,6 +1393,7 @@
":candidate",
":libjingle_peerconnection_api",
"../test:test_support",
+ "//third_party/abseil-cpp/absl/strings:string_view",
]
}
diff --git a/api/jsep.cc b/api/jsep.cc
index cf7c97b..337531e 100644
--- a/api/jsep.cc
+++ b/api/jsep.cc
@@ -15,11 +15,13 @@
#include <string>
#include <vector>
+#include "absl/strings/string_view.h"
#include "api/candidate.h"
namespace webrtc {
size_t SessionDescriptionInterface::RemoveCandidates(
+ absl::string_view mid,
const std::vector<Candidate>& /* candidates */) {
return 0;
}
diff --git a/api/jsep.h b/api/jsep.h
index 53a372a..2061845 100644
--- a/api/jsep.h
+++ b/api/jsep.h
@@ -273,7 +273,8 @@
// Returns the number of candidates removed.
// TODO: webrtc:42233526 - Deprecate and eventually remove this method in
// favor of the IceCandidate version.
- virtual size_t RemoveCandidates(const std::vector<Candidate>& candidates);
+ virtual size_t RemoveCandidates(absl::string_view mid,
+ const std::vector<Candidate>& candidates);
// Returns the number of m= sections in the session description.
virtual size_t number_of_mediasections() const = 0;
diff --git a/api/jsep_session_description.h b/api/jsep_session_description.h
index c32d827..acd82d1 100644
--- a/api/jsep_session_description.h
+++ b/api/jsep_session_description.h
@@ -62,7 +62,8 @@
virtual bool RemoveCandidate(const IceCandidate* candidate);
// TODO: https://issues.webrtc.org/42233526 - Remove this method in favor of
// the IceCandidate version.
- virtual size_t RemoveCandidates(const std::vector<Candidate>& candidates);
+ virtual size_t RemoveCandidates(absl::string_view mid,
+ const std::vector<Candidate>& candidates);
virtual size_t number_of_mediasections() const;
virtual const IceCandidateCollection* candidates(
size_t mediasection_index) const;
diff --git a/api/test/mock_session_description_interface.h b/api/test/mock_session_description_interface.h
index 655055a..a0b94df 100644
--- a/api/test/mock_session_description_interface.h
+++ b/api/test/mock_session_description_interface.h
@@ -17,6 +17,7 @@
#include <type_traits>
#include <vector>
+#include "absl/strings/string_view.h"
#include "api/candidate.h"
#include "api/jsep.h"
#include "test/gmock.h"
@@ -39,7 +40,7 @@
MOCK_METHOD(bool, RemoveCandidate, (const IceCandidate*), (override));
MOCK_METHOD(size_t,
RemoveCandidates,
- (const std::vector<Candidate>&),
+ (absl::string_view, const std::vector<Candidate>&),
(override));
MOCK_METHOD(size_t, number_of_mediasections, (), (const, override));
MOCK_METHOD(const IceCandidateCollection*,
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 41b61bd..3a04b25 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -2220,14 +2220,8 @@
if (!config_.gather_continually() || session != allocator_session()) {
return;
}
-
- std::vector<Candidate> candidates_to_remove;
- for (Candidate candidate : candidates) {
- candidate.set_transport_name(transport_name());
- candidates_to_remove.push_back(candidate);
- }
if (candidates_removed_callback_) {
- candidates_removed_callback_(this, candidates_to_remove);
+ candidates_removed_callback_(this, candidates);
}
}
diff --git a/pc/jsep_session_description.cc b/pc/jsep_session_description.cc
index 8613ada..96de928 100644
--- a/pc/jsep_session_description.cc
+++ b/pc/jsep_session_description.cc
@@ -280,14 +280,15 @@
}
size_t JsepSessionDescription::RemoveCandidates(
+ absl::string_view mid,
const std::vector<Candidate>& candidates) {
- size_t num_removed = 0;
+ int mediasection_index = GetMediasectionIndex(mid);
+ if (mediasection_index < 0) {
+ return 0u;
+ }
+
+ size_t num_removed = 0u;
for (auto& candidate : candidates) {
- int mediasection_index = GetMediasectionIndex(candidate.transport_name());
- if (mediasection_index < 0) {
- // Not found.
- continue;
- }
num_removed += candidate_collection_[mediasection_index].remove(candidate);
UpdateConnectionAddress(
candidate_collection_[mediasection_index],
diff --git a/pc/jsep_session_description_unittest.cc b/pc/jsep_session_description_unittest.cc
index 2f99dfd..fedd38c 100644
--- a/pc/jsep_session_description_unittest.cc
+++ b/pc/jsep_session_description_unittest.cc
@@ -498,20 +498,14 @@
SocketAddress("::1", 1234), kCandidatePriority, "", "",
IceCandidateType::kHost, kCandidateGeneration,
kCandidateFoundation);
- candidate1.set_transport_name("audio");
-
Candidate candidate2(ICE_CANDIDATE_COMPONENT_RTP, "tcp",
SocketAddress("::2", 1235), kCandidatePriority, "", "",
IceCandidateType::kHost, kCandidateGeneration,
kCandidateFoundation);
- candidate2.set_transport_name("audio");
-
Candidate candidate3(ICE_CANDIDATE_COMPONENT_RTP, "udp",
SocketAddress("192.168.1.1", 1236), kCandidatePriority,
"", "", IceCandidateType::kHost, kCandidateGeneration,
kCandidateFoundation);
- candidate3.set_transport_name("audio");
-
IceCandidate jice1("audio", 0, candidate1);
IceCandidate jice2("audio", 0, candidate2);
IceCandidate jice3("audio", 0, candidate3);
@@ -528,17 +522,17 @@
EXPECT_EQ("192.168.1.1:1236", media_desc->connection_address().ToString());
candidates.push_back(candidate3);
- ASSERT_TRUE(jsep_desc_->RemoveCandidates(candidates));
+ ASSERT_TRUE(jsep_desc_->RemoveCandidates("audio", candidates));
EXPECT_EQ("[::1]:1234", media_desc->connection_address().ToString());
candidates.clear();
candidates.push_back(candidate2);
- ASSERT_TRUE(jsep_desc_->RemoveCandidates(candidates));
+ ASSERT_TRUE(jsep_desc_->RemoveCandidates("audio", candidates));
EXPECT_EQ("[::1]:1234", media_desc->connection_address().ToString());
candidates.clear();
candidates.push_back(candidate1);
- ASSERT_TRUE(jsep_desc_->RemoveCandidates(candidates));
+ ASSERT_TRUE(jsep_desc_->RemoveCandidates("audio", candidates));
EXPECT_EQ("0.0.0.0:9", media_desc->connection_address().ToString());
}
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index a76bfa9..17d1124 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -1346,7 +1346,7 @@
void JsepTransportController::OnTransportCandidatesRemoved_n(
IceTransportInternal* transport,
const Candidates& candidates) {
- signal_ice_candidates_removed_.Send(candidates);
+ signal_ice_candidates_removed_.Send(transport, candidates);
}
void JsepTransportController::OnTransportCandidatePairChanged_n(
const CandidatePairChangeEvent& event) {
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 0ff1934..fdf3f1e 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -335,8 +335,8 @@
CallbackList<const IceCandidateErrorEvent&> signal_ice_candidate_error_
RTC_GUARDED_BY(network_thread_);
- CallbackList<const std::vector<Candidate>&> signal_ice_candidates_removed_
- RTC_GUARDED_BY(network_thread_);
+ CallbackList<IceTransportInternal*, const std::vector<Candidate>&>
+ signal_ice_candidates_removed_ RTC_GUARDED_BY(network_thread_);
CallbackList<const CandidatePairChangeEvent&>
signal_ice_candidate_pair_changed_ RTC_GUARDED_BY(network_thread_);
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index bf267df..08d0307 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -778,12 +778,13 @@
}));
});
transport_controller_->SubscribeIceCandidatesRemoved(
- [this](const std::vector<Candidate>& c) {
+ [this](IceTransportInternal* transport, const std::vector<Candidate>& c) {
RTC_DCHECK_RUN_ON(network_thread());
- signaling_thread()->PostTask(
- SafeTask(signaling_thread_safety_.flag(), [this, c = c]() {
+ std::string mid = transport->transport_name();
+ signaling_thread()->PostTask(SafeTask(
+ signaling_thread_safety_.flag(), [this, mid = mid, c = c]() {
RTC_DCHECK_RUN_ON(signaling_thread());
- OnTransportControllerCandidatesRemoved(c);
+ OnTransportControllerCandidatesRemoved(mid, c);
}));
});
transport_controller_->SubscribeIceCandidatePairChanged(
@@ -2076,12 +2077,24 @@
}
void PeerConnection::OnIceCandidatesRemoved(
+ absl::string_view mid,
const std::vector<Candidate>& candidates) {
if (IsClosed()) {
return;
}
- RunWithObserver(
- [&](auto observer) { observer->OnIceCandidatesRemoved(candidates); });
+ // Since this callback is based on the Candidate type, and not IceCandidate,
+ // all candidate instances should have the transport_name() property set to
+ // `mid`. See BasicPortAllocatorSession::PrunePortsAndRemoveCandidates for
+ // where the list of candidates is initially gathered.
+ std::vector<Candidate> candidates_for_notification;
+ candidates_for_notification.reserve(candidates.size());
+ for (Candidate candidate : candidates) { // Create a copy.
+ candidate.set_transport_name(mid);
+ candidates_for_notification.push_back(candidate);
+ }
+ RunWithObserver([&](auto observer) {
+ observer->OnIceCandidatesRemoved(candidates_for_notification);
+ });
}
void PeerConnection::OnSelectedCandidatePairChanged(
@@ -2455,18 +2468,11 @@
}
void PeerConnection::OnTransportControllerCandidatesRemoved(
+ absl::string_view mid,
const std::vector<Candidate>& candidates) {
- // Sanity check.
- for (const Candidate& candidate : candidates) {
- if (candidate.transport_name().empty()) {
- RTC_LOG(LS_ERROR) << "OnTransportControllerCandidatesRemoved: "
- "empty content name in candidate "
- << candidate.ToString();
- return;
- }
- }
- sdp_handler_->RemoveLocalIceCandidates(candidates);
- OnIceCandidatesRemoved(candidates);
+ RTC_DCHECK(!mid.empty());
+ sdp_handler_->RemoveLocalIceCandidates(mid, candidates);
+ OnIceCandidatesRemoved(mid, candidates);
}
void PeerConnection::OnTransportControllerCandidateChanged(
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 5274161..f9ef0b5 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -521,7 +521,8 @@
const std::string& error_text)
RTC_RUN_ON(signaling_thread());
// Some local ICE candidates have been removed.
- void OnIceCandidatesRemoved(const std::vector<Candidate>& candidates)
+ void OnIceCandidatesRemoved(absl::string_view mid,
+ const std::vector<Candidate>& candidates)
RTC_RUN_ON(signaling_thread());
void OnSelectedCandidatePairChanged(const CandidatePairChangeEvent& event)
@@ -582,6 +583,7 @@
void OnTransportControllerCandidateError(const IceCandidateErrorEvent& event)
RTC_RUN_ON(signaling_thread());
void OnTransportControllerCandidatesRemoved(
+ absl::string_view mid,
const std::vector<Candidate>& candidates) RTC_RUN_ON(signaling_thread());
void OnTransportControllerCandidateChanged(
const CandidatePairChangeEvent& event) RTC_RUN_ON(signaling_thread());
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 194fd7e..b75022f 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3024,8 +3024,11 @@
return false;
}
- const size_t number_removed =
- mutable_remote_description()->RemoveCandidates(candidates);
+ size_t number_removed = 0u;
+ for (const auto& c : candidates) {
+ number_removed +=
+ mutable_remote_description()->RemoveCandidates(c.transport_name(), {c});
+ }
if (number_removed != candidates.size()) {
RTC_LOG(LS_ERROR)
<< "RemoveIceCandidates: Failed to remove candidates. Requested "
@@ -3054,10 +3057,11 @@
}
void SdpOfferAnswerHandler::RemoveLocalIceCandidates(
+ absl::string_view mid,
const std::vector<Candidate>& candidates) {
RTC_DCHECK_RUN_ON(signaling_thread());
if (local_description()) {
- mutable_local_description()->RemoveCandidates(candidates);
+ mutable_local_description()->RemoveCandidates(mid, candidates);
}
}
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h
index 9b9fd04..f925f40 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -22,6 +22,7 @@
#include <string>
#include <vector>
+#include "absl/strings/string_view.h"
#include "api/audio_options.h"
#include "api/candidate.h"
#include "api/jsep.h"
@@ -153,7 +154,8 @@
bool RemoveIceCandidates(const std::vector<Candidate>& candidates);
// Adds a locally generated candidate to the local description.
void AddLocalIceCandidate(const IceCandidate* candidate);
- void RemoveLocalIceCandidates(const std::vector<Candidate>& candidates);
+ void RemoveLocalIceCandidates(absl::string_view mid,
+ const std::vector<Candidate>& candidates);
bool ShouldFireNegotiationNeededEvent(uint32_t event_id);
bool AddStream(MediaStreamInterface* local_stream);
diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc
index 8ee712d..b210030 100644
--- a/pc/webrtc_sdp_unittest.cc
+++ b/pc/webrtc_sdp_unittest.cc
@@ -1082,12 +1082,10 @@
jdesc_.candidates(1);
ASSERT_NE(nullptr, video_candidates_collection);
std::vector<Candidate> video_candidates;
- for (size_t i = 0; i < video_candidates_collection->count(); ++i) {
- Candidate c = video_candidates_collection->at(i)->candidate();
- c.set_transport_name("video_content_name");
- video_candidates.push_back(c);
+ for (const auto& c : video_candidates_collection->candidates()) {
+ video_candidates.push_back(c->candidate());
}
- jdesc_.RemoveCandidates(video_candidates);
+ jdesc_.RemoveCandidates("video_content_name", video_candidates);
}
// Turns the existing reference description into a description using
