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

[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) {
     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;
   }
 
   if (ready) {
-    return UseCandidate(candidate);
+    return AddRemoteCandidate(candidate);
   } else {
     LOG(LS_INFO) << "ProcessIceMessage: Not ready to use candidate.";
     return true;
   }
 }
 
 bool WebRtcSession::SetIceTransports(
     PeerConnectionInterface::IceTransportsType type) {
   return port_allocator()->set_candidate_filter(
         ConvertIceTransportTypeToCandidateFilter(type));
@@ skipping 395 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) {
+    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())
     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 30 matching lines @@
     for (size_t n = 0; n < candidates->count(); ++n) {
       const IceCandidateInterface* candidate = candidates->at(n);
       bool valid = false;
       if (!ReadyToUseRemoteCandidate(candidate, remote_desc, &valid)) {
         if (valid) {
           LOG(LS_INFO) << "UseCandidatesInSessionDescription: Not ready to use "
                        << "candidate.";
         }
         continue;
       }
-      ret = UseCandidate(candidate);
+      ret = AddRemoteCandidate(candidate);
       if (!ret) {
         break;
       }
     }
   }
   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 nullptr;
+  }
+  return &(remote_desc_->description()->contents()[mediacontent_index]);
+}
+
+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;
   }
-
-  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::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_.
   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