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..e1bc7944b9f6e6e17ab1eaf5bb9206dba690a47c 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) {

[pthatcher1, 2016/02/10 00:01:41]

I'll need to think about this more to think if this is the right way to signal
candidate removal.  It is interesting, but I'm afraid that apps not expecting it
will be surprised in a bad way.  The alternative is to have new methods of
PeerConnection.onicecandidate and PeerConnection.removeIceCandidate.

[honghaiz3, 2016/02/10 19:22:43]

Looking at the code, I think there are two possible surprises:
1. when an old client receives a signal to remove a peer reflexive candidate. It
will fail to parse the candidate. But it does not cause real problems. It will
return a null ptr as the candidate which will be discarded later in the process.
2. The app may hold the candidates and push down as  a batch. This is not a
problem either as long as the ordering of the signaled candidates  are preserved
(if not, it will have issues too even if we use a new signaling like
"remove-candidate"). 
I believe the alternative way you mentioned will signal this as something like
"remove-candidate: ...."
I think we have discussed that approach with Justin before and he suggested
using a priority 0 to indicate a candidate being removed. 

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

I think having removals come out of the "added" event and into the "add" method
is the part that's surprising and risky.  

It's not just risky because of what browsers will do.  It's risky because of
what JS applications and servers and gateways and random remote endpoints will
do.  "Trickle remove" is just such a new thing that I'm sure it will cause
problems somewhere if it's implicitly done using the same events and methods and
signals, which is why I think we need new events, methods, and signals for
removal.

We can discuss it again with Justin, but I think he'll agree once he considers
how many things won't expect this and may break.

+    if (!remote_desc_->RemoveCandidate(candidate)) {
+      LOG(LS_ERROR) << "ProcessIceMessage: Candidate not found";
+    }
+    return StopUsingCandidate(candidate);
+  }
+
   bool valid = false;
   bool ready = ReadyToUseRemoteCandidate(candidate, NULL, &valid);
   if (!valid) {
@@ -1621,6 +1630,30 @@ 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) {
+    // 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);

[pthatcher1, 2016/02/10 00:01:40]

I think having an event of OnIceCandidateRemoved might make more sense, but we
need to think about the tradeoffs.

[honghaiz3, 2016/02/10 19:22:43]

See my reply above.

+    }
+    if (local_desc_) {
+      local_desc_->RemoveCandidate(&candidate);
+    }
+  }
+}
+
 // Enabling voice and video channel.
 void WebRtcSession::EnableChannels() {
   if (voice_channel_ && !voice_channel_->enabled())
@@ -1680,24 +1713,26 @@ bool WebRtcSession::UseCandidatesInSessionDescription(
   return ret;
 }
 
-bool WebRtcSession::UseCandidate(
-    const IceCandidateInterface* candidate) {
-
+const cricket::ContentInfo* WebRtcSession::GetRemoteMediaContent(
+    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::UseCandidate(const IceCandidateInterface* candidate) {
+  const cricket::ContentInfo* content = GetRemoteMediaContent(candidate);
+  if (content == nullptr) {
+    LOG(LS_ERROR) << "UseCandidate: 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 +1757,22 @@ bool WebRtcSession::UseCandidate(
   return true;
 }
 
+bool WebRtcSession::StopUsingCandidate(const IceCandidateInterface* candidate) {
+  const cricket::ContentInfo* content = GetRemoteMediaContent(candidate);
+  if (!content) {
+    LOG(LS_ERROR) << "StopUsingCandidate: 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_.