Make RemoveIceCandidates() return false if no candidates were removed
Simplifying index checks in JsepSessionDescription and adding test.
Bug: webrtc:8395
Change-Id: I7e0a11628b884394d6dcf26b367267a1bde30905
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/395281
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44916}
diff --git a/api/jsep_session_description.h b/api/jsep_session_description.h
index 19dbf99..978a472 100644
--- a/api/jsep_session_description.h
+++ b/api/jsep_session_description.h
@@ -72,8 +72,9 @@
SdpType type_;
std::vector<JsepCandidateCollection> candidate_collection_;
- bool GetMediasectionIndex(const IceCandidate* candidate, size_t* index);
- int GetMediasectionIndex(const Candidate& candidate);
+ bool IsValidMLineIndex(int index) const;
+ bool GetMediasectionIndex(const IceCandidate* candidate, size_t* index) const;
+ int GetMediasectionIndex(absl::string_view mid) const;
};
} // namespace webrtc
diff --git a/pc/jsep_session_description.cc b/pc/jsep_session_description.cc
index bded5cd..cf5dfa3 100644
--- a/pc/jsep_session_description.cc
+++ b/pc/jsep_session_description.cc
@@ -10,7 +10,9 @@
#include "api/jsep_session_description.h"
+#include <algorithm>
#include <cstddef>
+#include <iterator>
#include <memory>
#include <optional>
#include <string>
@@ -101,7 +103,6 @@
}
media_desc->set_connection_address(connection_addr);
}
-
} // namespace
// TODO(steveanton): Remove this default implementation once Chromium has been
@@ -268,7 +269,7 @@
const std::vector<Candidate>& candidates) {
size_t num_removed = 0;
for (auto& candidate : candidates) {
- int mediasection_index = GetMediasectionIndex(candidate);
+ int mediasection_index = GetMediasectionIndex(candidate.transport_name());
if (mediasection_index < 0) {
// Not found.
continue;
@@ -302,51 +303,33 @@
return !out->empty();
}
+bool JsepSessionDescription::IsValidMLineIndex(int index) const {
+ RTC_DCHECK(description_);
+ return index >= 0 &&
+ index < static_cast<int>(description_->contents().size());
+}
+
bool JsepSessionDescription::GetMediasectionIndex(const IceCandidate* candidate,
- size_t* index) {
- if (!candidate || !index) {
+ size_t* index) const {
+ if (!candidate || !index || !description_) {
return false;
}
- // 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<size_t>(candidate->sdp_mline_index()) >=
- description_->contents().size())) {
- return false;
- }
-
- if (candidate->sdp_mline_index() >= 0)
+ auto mid = candidate->sdp_mid();
+ if (!mid.empty()) {
+ *index = GetMediasectionIndex(mid);
+ } else {
+ // An sdp_mline_index of -1 will be treated as invalid.
*index = static_cast<size_t>(candidate->sdp_mline_index());
- if (description_ && !candidate->sdp_mid().empty()) {
- bool found = false;
- // Try to match the sdp_mid with content name.
- for (size_t i = 0; i < description_->contents().size(); ++i) {
- if (candidate->sdp_mid() == description_->contents().at(i).mid()) {
- *index = i;
- found = true;
- break;
- }
- }
- if (!found) {
- // If the sdp_mid is presented but we can't find a match, we consider
- // this as an error.
- return false;
- }
}
- return true;
+ return IsValidMLineIndex(*index);
}
-int JsepSessionDescription::GetMediasectionIndex(const Candidate& candidate) {
- // Find the description with a matching transport name of the candidate.
- const std::string& transport_name = candidate.transport_name();
- for (size_t i = 0; i < description_->contents().size(); ++i) {
- if (transport_name == description_->contents().at(i).mid()) {
- return static_cast<int>(i);
- }
- }
- return -1;
+int JsepSessionDescription::GetMediasectionIndex(absl::string_view mid) const {
+ const auto& contents = description_->contents();
+ auto it =
+ std::find_if(contents.begin(), contents.end(),
+ [&](const auto& content) { return mid == content.mid(); });
+ return it == contents.end() ? -1 : std::distance(contents.begin(), it);
}
-
} // namespace webrtc
diff --git a/pc/jsep_session_description_unittest.cc b/pc/jsep_session_description_unittest.cc
index 1de2a1a..c00e48d 100644
--- a/pc/jsep_session_description_unittest.cc
+++ b/pc/jsep_session_description_unittest.cc
@@ -166,6 +166,16 @@
EXPECT_EQ(2u, jsep_desc_->number_of_mediasections());
}
+TEST_F(JsepSessionDescriptionTest, IsValidMLineIndex) {
+ ASSERT_GT(jsep_desc_->number_of_mediasections(), 0u);
+ // Create a candidate with no mid and an illegal index.
+ IceCandidate ice_candidate("", -1, candidate_);
+ EXPECT_FALSE(jsep_desc_->AddCandidate(&ice_candidate));
+ IceCandidate ice_candidate2("", jsep_desc_->number_of_mediasections(),
+ candidate_);
+ EXPECT_FALSE(jsep_desc_->AddCandidate(&ice_candidate2));
+}
+
// Test that we can add a candidate to a session description without MID.
TEST_F(JsepSessionDescriptionTest, AddCandidateWithoutMid) {
IceCandidate jsep_candidate("", 0, candidate_);
diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc
index fbcdb5b..2125883 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -87,6 +87,7 @@
using ::testing::Combine;
using ::testing::ElementsAre;
+using ::testing::IsEmpty;
using ::testing::Pair;
using ::testing::Values;
@@ -595,7 +596,8 @@
std::unique_ptr<IceCandidate> ice_candidate =
CreateIceCandidate(audio_content->mid(), 0, candidate);
EXPECT_TRUE(caller->pc()->AddIceCandidate(ice_candidate.get()));
- EXPECT_TRUE(caller->pc()->RemoveIceCandidates({candidate}));
+ // This will fail since `candidate.transport_name()` is empty.
+ EXPECT_FALSE(caller->pc()->RemoveIceCandidates({candidate}));
}
TEST_P(PeerConnectionIceTest, RemoveCandidateRemovesFromRemoteDescription) {
@@ -1598,12 +1600,14 @@
// `candidate.transport_name()` is empty.
Candidate candidate = CreateLocalUdpCandidate(kCalleeAddress);
+ ASSERT_THAT(candidate.transport_name(), IsEmpty());
auto* audio_content =
GetFirstAudioContent(caller->pc()->local_description()->description());
std::unique_ptr<IceCandidate> ice_candidate =
CreateIceCandidate(audio_content->mid(), 65535, candidate);
EXPECT_TRUE(caller->pc()->AddIceCandidate(ice_candidate.get()));
- EXPECT_TRUE(caller->pc()->RemoveIceCandidates({candidate}));
+ // Removing the candidate will fail because of no transport_name().
+ EXPECT_FALSE(caller->pc()->RemoveIceCandidates({candidate}));
}
} // namespace webrtc
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index df53177..34102c8 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3001,7 +3001,7 @@
return false;
}
- size_t number_removed =
+ const size_t number_removed =
mutable_remote_description()->RemoveCandidates(candidates);
if (number_removed != candidates.size()) {
RTC_LOG(LS_ERROR)
@@ -3017,7 +3017,7 @@
<< "RemoveIceCandidates: Error when removing remote candidates: "
<< error.message();
}
- return true;
+ return number_removed != 0u;
}
void SdpOfferAnswerHandler::AddLocalIceCandidate(
