Remove candidates when doing continual gathering

talk/app/webrtc/webrtcsession.cc

Index: talk/app/webrtc/webrtcsession.cc
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index b1a7c21c7e23dc60092cd6cb790011e055f98488..967a7c1d5f4e7f78b25c5be3323d0e80c537548e 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -571,6 +571,8 @@ WebRtcSession::WebRtcSession(webrtc::MediaControllerInterface* media_controller,
       this, &WebRtcSession::OnTransportControllerGatheringState);
   transport_controller_->SignalCandidatesGathered.connect(
       this, &WebRtcSession::OnTransportControllerCandidatesGathered);
+  transport_controller_->SignalCandidatesRemoved.connect(
+      this, &WebRtcSession::OnTransportControllerCandidatesRemoved);
 }
 
 WebRtcSession::~WebRtcSession() {
@@ -1182,9 +1184,9 @@ bool WebRtcSession::EnableBundle(const cricket::ContentGroup& bundle) {
 
 bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) {
   if (!remote_desc_) {
-    LOG(LS_ERROR) << "ProcessIceMessage: ICE candidates can't be added "
+    LOG(LS_ERROR) << "ProcessIceMessage: ICE candidates can't be processed "
                   << "without any remote session description.";
-     return false;
+    return false;
   }
 
   if (!candidate) {
@@ -1192,6 +1194,13 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) {
     return false;
   }
 
+  if (candidate->candidate().priority() == 0) {

[Taylor Brandstetter, 2016/02/10 21:58:08]

Using a priority of 0 doesn't seem like an ideal long term solution. I expected
that we'd add a "removeIceCandidate" method, and "oncandidateremoved" event.
Just my two cents.

[pthatcher1, 2016/02/11 20:25:51]

I agree.

+    if (!remote_desc_->RemoveCandidate(candidate)) {
+      LOG(LS_ERROR) << "ProcessIceMessage: Candidate not found";
+    }
+    return RemoveRemoteCandidate(candidate);
+  }
+
   bool valid = false;
   bool ready = ReadyToUseRemoteCandidate(candidate, NULL, &valid);
   if (!valid) {
@@ -1205,7 +1214,7 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) {
   }
 
   if (ready) {
-    return UseCandidate(candidate);
+    return AddRemoteCandidate(candidate);
   } else {
     LOG(LS_INFO) << "ProcessIceMessage: Not ready to use candidate.";
     return true;
@@ -1621,6 +1630,31 @@ void WebRtcSession::OnTransportControllerCandidatesGathered(
   }
 }
 
+void WebRtcSession::OnTransportControllerCandidatesRemoved(
+    const std::string& transport_name,
+    const cricket::Candidates& candidates) {
+  ASSERT(signaling_thread()->IsCurrent());
+  int sdp_mline_index;
+  if (!GetLocalCandidateMediaIndex(transport_name, &sdp_mline_index)) {
+    LOG(LS_ERROR) << "OnTransportControllerCandidatesRemoved: content name "
+                  << transport_name << " not found";
+    return;
+  }
+
+  for (const cricket::Candidate& cand : candidates) {
+    ASSERT(cand.priority() == 0);
+    // Use transport_name as the candidate media id.
+    JsepIceCandidate candidate(transport_name, sdp_mline_index, cand);
+    // It will be nicer if we can signal multiple candidates in one message.
+    if (ice_observer_) {
+      ice_observer_->OnIceCandidate(&candidate);
+    }
+    if (local_desc_) {
+      local_desc_->RemoveCandidate(&candidate);
+    }
+  }
+}
+
 // Enabling voice and video channel.
 void WebRtcSession::EnableChannels() {
   if (voice_channel_ && !voice_channel_->enabled())
@@ -1671,7 +1705,7 @@ bool WebRtcSession::UseCandidatesInSessionDescription(
         }
         continue;
       }
-      ret = UseCandidate(candidate);
+      ret = AddRemoteCandidate(candidate);
       if (!ret) {
         break;
       }
@@ -1680,24 +1714,26 @@ bool WebRtcSession::UseCandidatesInSessionDescription(
   return ret;
 }
 
-bool WebRtcSession::UseCandidate(
-    const IceCandidateInterface* candidate) {
-
+const cricket::ContentInfo* WebRtcSession::GetRemoteMediaContent(

[Taylor Brandstetter, 2016/02/10 21:58:08]

As long as you're making this a standalone method, could you make it work if the
candidate only has "sdp_mid" set, and not "sdp_mline_index"? This is allowed by
the spec, technically.

[honghaiz3, 2016/02/12 00:56:55]

How do you know if sdp_mline_index is set or not?

[Taylor Brandstetter, 2016/02/12 02:30:38]

Extremely unfortunately, the default value appears to be 0, which is
indistinguishable from a valid value:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'll make a note to myself to fix this. But for now, the algorithm here could be
"If sdp_mid is nonempty, use it. Otherwise, use sdp_mline_index."

+    const IceCandidateInterface* candidate) const {
   size_t mediacontent_index = static_cast<size_t>(candidate->sdp_mline_index());
   size_t remote_content_size = remote_desc_->description()->contents().size();
   if (mediacontent_index >= remote_content_size) {
-    LOG(LS_ERROR)
-        << "UseRemoteCandidateInSession: Invalid candidate media index.";
-    return false;
+    return nullptr;
   }
+  return &(remote_desc_->description()->contents()[mediacontent_index]);
+}
 
-  cricket::ContentInfo content =
-      remote_desc_->description()->contents()[mediacontent_index];
-  std::vector<cricket::Candidate> candidates;
-  candidates.push_back(candidate->candidate());
+bool WebRtcSession::AddRemoteCandidate(const IceCandidateInterface* candidate) {
+  const cricket::ContentInfo* content = GetRemoteMediaContent(candidate);
+  if (content == nullptr) {
+    LOG(LS_ERROR) << "AddRemoteCandidate: media content not found";
+    return false;
+  }
+  std::vector<cricket::Candidate> candidates(1, candidate->candidate());
   // Invoking BaseSession method to handle remote candidates.
   std::string error;
-  if (transport_controller_->AddRemoteCandidates(content.name, candidates,
+  if (transport_controller_->AddRemoteCandidates(content->name, candidates,
                                                  &error)) {
     // Candidates successfully submitted for checking.
     if (ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew ||
@@ -1722,6 +1758,23 @@ bool WebRtcSession::UseCandidate(
   return true;
 }
 
+bool WebRtcSession::RemoveRemoteCandidate(
+    const IceCandidateInterface* candidate) {
+  const cricket::ContentInfo* content = GetRemoteMediaContent(candidate);
+  if (!content) {
+    LOG(LS_ERROR) << "RemoveRemoteCandidate: media content not found";
+    return false;
+  }
+  cricket::Candidates candidates(1, candidate->candidate());
+  std::string error;
+  bool res = transport_controller_->RemoveRemoteCandidates(content->name,
+                                                           candidates, &error);
+  if (!res) {
+    LOG(LS_WARNING) << "Error when Removing remote candidate: " << error;
+  }
+  return res;
+}
+
 void WebRtcSession::RemoveUnusedChannels(const SessionDescription* desc) {
   // Destroy video_channel_ first since it may have a pointer to the
   // voice_channel_.