Remove candidates when doing continual gathering

talk/app/webrtc/webrtcsession.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 553 matching lines @@
       metrics_observer_(NULL) {
   transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLED);
   transport_controller_->SignalConnectionState.connect(
       this, &WebRtcSession::OnTransportControllerConnectionState);
   transport_controller_->SignalReceiving.connect(
       this, &WebRtcSession::OnTransportControllerReceiving);
   transport_controller_->SignalGatheringState.connect(
       this, &WebRtcSession::OnTransportControllerGatheringState);
   transport_controller_->SignalCandidatesGathered.connect(
       this, &WebRtcSession::OnTransportControllerCandidatesGathered);
+  transport_controller_->SignalCandidatesRemoved.connect(
+      this, &WebRtcSession::OnTransportControllerCandidatesRemoved);
 }
 
 WebRtcSession::~WebRtcSession() {
   ASSERT(signaling_thread()->IsCurrent());
   // Destroy video_channel_ first since it may have a pointer to the
   // voice_channel_.
   if (video_channel_) {
     SignalVideoChannelDestroyed();
     channel_manager_->DestroyVideoChannel(video_channel_.release());
   }
@@ skipping 591 matching lines @@
       !maybe_set_transport(video_channel()) ||
       !maybe_set_transport(data_channel())) {
     return false;
   }
 
   return true;
 }
 
 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) {
     LOG(LS_ERROR) << "ProcessIceMessage: Candidate is NULL.";
     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) {
     return false;
   }
 
   // Add this candidate to the remote session description.
   if (!remote_desc_->AddCandidate(candidate)) {
     LOG(LS_ERROR) << "ProcessIceMessage: Candidate cannot be used.";
     return false;
@@ skipping 409 matching lines @@
     JsepIceCandidate candidate(transport_name, sdp_mline_index, *citer);
     if (ice_observer_) {
       ice_observer_->OnIceCandidate(&candidate);
     }
     if (local_desc_) {
       local_desc_->AddCandidate(&candidate);
     }
   }
 }
 
+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())
     voice_channel_->Enable(true);
 
   if (video_channel_ && !video_channel_->enabled())
     video_channel_->Enable(true);
 
   if (data_channel_ && !data_channel_->enabled())
     data_channel_->Enable(true);
@@ skipping 39 matching lines @@
       }
       ret = UseCandidate(candidate);
       if (!ret) {
         break;
       }
     }
   }
   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 nullptr;
+  }
+  return &(remote_desc_->description()->contents()[mediacontent_index]);
+}
+
+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;
   }
-
-  cricket::ContentInfo content =
-      remote_desc_->description()->contents()[mediacontent_index];
-  std::vector<cricket::Candidate> candidates;
-  candidates.push_back(candidate->candidate());
+  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 ||
         ice_connection_state_ ==
             PeerConnectionInterface::kIceConnectionDisconnected) {
       // If state is New, then the session has just gotten its first remote ICE
       // candidates, so go to Checking.
       // If state is Disconnected, the session is re-using old candidates or
       // receiving additional ones, so go to Checking.
       // If state is Connected, stay Connected.
       // TODO(bemasc): If state is Connected, and the new candidates are for a
       // newly added transport, then the state actually _should_ move to
       // checking.  Add a way to distinguish that case.
       SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking);
     }
     // TODO(bemasc): If state is Completed, go back to Connected.
   } else {
     if (!error.empty()) {
       LOG(LS_WARNING) << error;
     }
   }
   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_.
   const cricket::ContentInfo* video_info =
       cricket::GetFirstVideoContent(desc);
   if ((!video_info || video_info->rejected) && video_channel_) {
     SignalVideoChannelDestroyed();
     channel_manager_->DestroyVideoChannel(video_channel_.release());
   }
 
@@ skipping 441 matching lines @@
   }
 }
 
 void WebRtcSession::OnSentPacket_w(cricket::TransportChannel* channel,
                                    const rtc::SentPacket& sent_packet) {
   RTC_DCHECK(worker_thread()->IsCurrent());
   media_controller_->call_w()->OnSentPacket(sent_packet);
 }
 
 }  // namespace webrtc