Revert "Make RemoveIceCandidates() return false if no candidates were removed"
This reverts commit f23a83481a780227f19639391d94143240f04e7c.
Reason for revert: Breaks downstream test
Bug: webrtc:8395
Original change's description:
> 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}
Bug: webrtc:8395
Change-Id: Ib15e38279e72a0507b5e5ea85873c88c14ac661c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/396101
Reviewed-by: Jeremy Leconte <jleconte@google.com>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Auto-Submit: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44922}
diff --git a/api/jsep_session_description.h b/api/jsep_session_description.h
index c32d827..d2683a0 100644
--- a/api/jsep_session_description.h
+++ b/api/jsep_session_description.h
@@ -75,9 +75,8 @@
SdpType type_;
std::vector<JsepCandidateCollection> candidate_collection_;
- bool IsValidMLineIndex(int index) const;
- bool GetMediasectionIndex(const IceCandidate* candidate, size_t* index) const;
- int GetMediasectionIndex(absl::string_view mid) const;
+ bool GetMediasectionIndex(const IceCandidate* candidate, size_t* index);
+ int GetMediasectionIndex(const Candidate& candidate);
};
} // namespace webrtc
diff --git a/pc/jsep_session_description.cc b/pc/jsep_session_description.cc
index 8613ada..bbeb4c3 100644
--- a/pc/jsep_session_description.cc
+++ b/pc/jsep_session_description.cc
@@ -10,9 +10,7 @@
#include "api/jsep_session_description.h"
-#include <algorithm>
#include <cstddef>
-#include <iterator>
#include <memory>
#include <optional>
#include <string>
@@ -103,6 +101,7 @@
}
media_desc->set_connection_address(connection_addr);
}
+
} // namespace
// TODO(steveanton): Remove this default implementation once Chromium has been
@@ -283,7 +282,7 @@
const std::vector<Candidate>& candidates) {
size_t num_removed = 0;
for (auto& candidate : candidates) {
- int mediasection_index = GetMediasectionIndex(candidate.transport_name());
+ int mediasection_index = GetMediasectionIndex(candidate);
if (mediasection_index < 0) {
// Not found.
continue;
@@ -317,33 +316,51 @@
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) const {
- if (!candidate || !index || !description_) {
+ size_t* index) {
+ if (!candidate || !index) {
return false;
}
- 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 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;
}
- return IsValidMLineIndex(*index);
+
+ if (candidate->sdp_mline_index() >= 0)
+ *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;
}
-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);
+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;
}
+
} // namespace webrtc
diff --git a/pc/jsep_session_description_unittest.cc b/pc/jsep_session_description_unittest.cc
index ee623cb..7619e5a 100644
--- a/pc/jsep_session_description_unittest.cc
+++ b/pc/jsep_session_description_unittest.cc
@@ -168,16 +168,6 @@
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 2125883..fbcdb5b 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -87,7 +87,6 @@
using ::testing::Combine;
using ::testing::ElementsAre;
-using ::testing::IsEmpty;
using ::testing::Pair;
using ::testing::Values;
@@ -596,8 +595,7 @@
std::unique_ptr<IceCandidate> ice_candidate =
CreateIceCandidate(audio_content->mid(), 0, candidate);
EXPECT_TRUE(caller->pc()->AddIceCandidate(ice_candidate.get()));
- // This will fail since `candidate.transport_name()` is empty.
- EXPECT_FALSE(caller->pc()->RemoveIceCandidates({candidate}));
+ EXPECT_TRUE(caller->pc()->RemoveIceCandidates({candidate}));
}
TEST_P(PeerConnectionIceTest, RemoveCandidateRemovesFromRemoteDescription) {
@@ -1600,14 +1598,12 @@
// `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()));
- // Removing the candidate will fail because of no transport_name().
- EXPECT_FALSE(caller->pc()->RemoveIceCandidates({candidate}));
+ EXPECT_TRUE(caller->pc()->RemoveIceCandidates({candidate}));
}
} // namespace webrtc
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 34102c8..df53177 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3001,7 +3001,7 @@
return false;
}
- const size_t number_removed =
+ 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 number_removed != 0u;
+ return true;
}
void SdpOfferAnswerHandler::AddLocalIceCandidate(
