Removing webrtc::PortAllocatorFactoryInterface.

talk/app/webrtc/peerconnectionfactory.cc

 /*
  * libjingle
  * Copyright 2004--2011 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 17 matching lines @@
 #include "talk/app/webrtc/peerconnectionfactory.h"
 
 #include "talk/app/webrtc/audiotrack.h"
 #include "talk/app/webrtc/localaudiosource.h"
 #include "talk/app/webrtc/mediastream.h"
 #include "talk/app/webrtc/mediastreamproxy.h"
 #include "talk/app/webrtc/mediastreamtrackproxy.h"
 #include "talk/app/webrtc/peerconnection.h"
 #include "talk/app/webrtc/peerconnectionfactoryproxy.h"
 #include "talk/app/webrtc/peerconnectionproxy.h"
-#include "talk/app/webrtc/portallocatorfactory.h"
 #include "talk/app/webrtc/videosource.h"
 #include "talk/app/webrtc/videosourceproxy.h"
 #include "talk/app/webrtc/videotrack.h"
 #include "talk/media/webrtc/webrtcmediaengine.h"
 #include "talk/media/webrtc/webrtcvideodecoderfactory.h"
 #include "talk/media/webrtc/webrtcvideoencoderfactory.h"
 #include "webrtc/base/bind.h"
 #include "webrtc/modules/audio_device/include/audio_device.h"
 #include "webrtc/p2p/base/basicpacketsocketfactory.h"
 #include "webrtc/p2p/client/basicportallocator.h"
@@ skipping 99 matching lines @@
   ASSERT(worker_thread != NULL);
   ASSERT(signaling_thread != NULL);
   // TODO: Currently there is no way creating an external adm in
   // libjingle source tree. So we can 't currently assert if this is NULL.
   // ASSERT(default_adm != NULL);
 }
 
 PeerConnectionFactory::~PeerConnectionFactory() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   channel_manager_.reset(nullptr);
-  default_allocator_factory_ = nullptr;
 
   // Make sure |worker_thread_| and |signaling_thread_| outlive
   // |dtls_identity_store_|, |default_socket_factory_| and
   // |default_network_manager_|.
   dtls_identity_store_ = nullptr;
   default_socket_factory_ = nullptr;
   default_network_manager_ = nullptr;
 
   if (owns_ptrs_) {
     if (wraps_current_thread_)
       rtc::ThreadManager::Instance()->UnwrapCurrentThread();
     delete worker_thread_;
   }
 }
 
 bool PeerConnectionFactory::Initialize() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   rtc::InitRandom(rtc::Time());
 
-  default_allocator_factory_ = PortAllocatorFactory::Create(worker_thread_);
-  if (!default_allocator_factory_) {
-    return false;
-  }
-
   default_network_manager_.reset(new rtc::BasicNetworkManager());
   if (!default_network_manager_) {
     return false;
   }
 
   default_socket_factory_.reset(
       new rtc::BasicPacketSocketFactory(worker_thread_));
   if (!default_socket_factory_) {
     return false;
   }
@@ skipping 54 matching lines @@
 
 void PeerConnectionFactory::StopRtcEventLog() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   channel_manager_->StopRtcEventLog();
 }
 
 rtc::scoped_refptr<PeerConnectionInterface>
 PeerConnectionFactory::CreatePeerConnection(
     const PeerConnectionInterface::RTCConfiguration& configuration,
     const MediaConstraintsInterface* constraints,
-    PortAllocatorFactoryInterface* allocator_factory,
-    rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
-    PeerConnectionObserver* observer) {
-  RTC_DCHECK(signaling_thread_->IsCurrent());
-  RTC_DCHECK(allocator_factory || default_allocator_factory_);
-
-  if (!dtls_identity_store.get()) {
-    // Because |pc|->Initialize takes ownership of the store we need a new
-    // wrapper object that can be deleted without deleting the underlying
-    // |dtls_identity_store_|, protecting it from being deleted multiple times.
-    dtls_identity_store.reset(
-        new DtlsIdentityStoreWrapper(dtls_identity_store_));
-  }
-
-  PortAllocatorFactoryInterface* chosen_allocator_factory =
-      allocator_factory ? allocator_factory : default_allocator_factory_.get();
-  chosen_allocator_factory->SetNetworkIgnoreMask(options_.network_ignore_mask);
-
-  rtc::scoped_refptr<PeerConnection> pc(
-      new rtc::RefCountedObject<PeerConnection>(this));
-  if (!pc->Initialize(
-      configuration,
-      constraints,
-      chosen_allocator_factory,
-      dtls_identity_store.Pass(),
-      observer)) {
-    return NULL;
-  }
-  return PeerConnectionProxy::Create(signaling_thread(), pc);
-}
-
-rtc::scoped_refptr<PeerConnectionInterface>
-PeerConnectionFactory::CreatePeerConnection(
-    const PeerConnectionInterface::RTCConfiguration& configuration,
-    const MediaConstraintsInterface* constraints,
     rtc::scoped_ptr<cricket::PortAllocator> allocator,
     rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
     PeerConnectionObserver* observer) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
 
   if (!dtls_identity_store.get()) {
     // Because |pc|->Initialize takes ownership of the store we need a new
     // wrapper object that can be deleted without deleting the underlying
     // |dtls_identity_store_|, protecting it from being deleted multiple times.
     dtls_identity_store.reset(
         new DtlsIdentityStoreWrapper(dtls_identity_store_));
   }
 
   if (!allocator) {
     allocator.reset(new cricket::BasicPortAllocator(
         default_network_manager_.get(), default_socket_factory_.get()));
   }
-  default_network_manager_->set_network_ignore_mask(
-      options_.network_ignore_mask);
+  allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);

[pthatcher1, 2015/12/15 07:58:44]

So is this is really just a way to set the network ignore mask of the underlying
network manager, which will then be used to filter out networks when used by the
allocator in BasicPortAllocatorSession::DoAllocate amd
BasicPortAllocatorSession::OnNetworksChanged()?

If so, I have a few suggestions:

1.  Would it make sense to just skip setting the network mask in the
NetworkManager and instead apply filtering in the PortAllocator?  We could do
inside of DoAllocate and OnNetworksChanged quite easily.

2.  If that's infeasible, would it make sense to add a network_manager() getter
on the allocator, so the above line of code would be
allocator->newtork_manager()->SetNetworkIgnoreMask(...).  That would perhaps be
cleaner and make it more clear what the scope of the change is.

3.  If neither of those are feasible, then could we at least document what is
really going on here at this point (that the NetworkManager of the PortAllocator
is being modified, not really the PortAllocator)?

[phoglund, 2015/12/15 12:41:31]

Yeah. When I made the network mask change, I had to go through the port
allocator to get the information in options_ to the network manager. I don't
fully understand the role of the port allocator: does it make sense to tell it
to apply a ignore mask? If it does, 1) sounds great. The network manager already
classifies networks as loopback or not, so one could move all whole ignore logic
to the port allocator and make it do the filtering. Just watch out if there are
other users of the network manager, that doesn't go through the port allocator.

Also, please be careful so that the mask is set before the networks are
enumerated. Otherwise the list of networks will be incorrect.

 
   rtc::scoped_refptr<PeerConnection> pc(
       new rtc::RefCountedObject<PeerConnection>(this));
   if (!pc->Initialize(configuration, constraints, std::move(allocator),
                       std::move(dtls_identity_store), observer)) {
     return nullptr;
   }
   return PeerConnectionProxy::Create(signaling_thread(), pc);
 }
 
@@ skipping 42 matching lines @@
 }
 
 cricket::MediaEngineInterface* PeerConnectionFactory::CreateMediaEngine_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   return cricket::WebRtcMediaEngineFactory::Create(
       default_adm_.get(), video_encoder_factory_.get(),
       video_decoder_factory_.get());
 }
 
 }  // namespace webrtc