diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp index e137291180c2196592a305e77be638bb1ccf31ce..dd55873d8ea9e9aa5fa7abd23420f1ab265d8ca6 100644 --- a/src/ice_transport.cpp +++ b/src/ice_transport.cpp @@ -78,10 +78,10 @@ using namespace upnp; class IceTransport::Impl { public: - Impl(const char* name, const IceTransportOptions& options); + Impl(const char* name); ~Impl(); - void initIceInstance(); + void initIceInstance(const IceTransportOptions& options); void onComplete(pj_ice_strans* ice_st, pj_ice_strans_op op, pj_status_t status); @@ -304,34 +304,12 @@ add_turn_server(pj_pool_t& pool, pj_ice_strans_cfg& cfg, const TurnServerInfo& i //============================================================================== -IceTransport::Impl::Impl(const char* name, const IceTransportOptions& options) +IceTransport::Impl::Impl(const char* name) : sessionName_(name) - , pool_(nullptr, - [](pj_pool_t* pool) { - pj_pool_release(pool); - }) - , isTcp_(options.tcpEnable) - , upnpEnabled_(options.upnpEnable) - , on_initdone_cb_(options.onInitDone) - , on_negodone_cb_(options.onNegoDone) - , streamsCount_(options.streamsCount) - , compCountPerStream_(options.compCountPerStream) - , compCount_(streamsCount_ * compCountPerStream_) - , compIO_(compCount_) - , peerChannels_(compCount_) - , iceDefaultRemoteAddr_(compCount_) - , initiatorSession_(options.master) - , accountLocalAddr_(std::move(options.accountLocalAddr)) - , accountPublicAddr_(std::move(options.accountPublicAddr)) - , stunServers_(std::move(options.stunServers)) - , turnServers_(std::move(options.turnServers)) + , pool_(nullptr, [](pj_pool_t* pool) { pj_pool_release(pool); }) , thread_() { - JAMI_DBG("[ice:%p] Creating IceTransport session for \"%s\" - comp count %u - as a %s", - this, - name, - compCount_, - initiatorSession_ ? "master" : "slave"); + JAMI_DBG("[ice:%p] Creating IceTransport session for \"%s\"", this, name); } IceTransport::Impl::~Impl() @@ -385,8 +363,29 @@ IceTransport::Impl::~Impl() } void -IceTransport::Impl::initIceInstance() -{ +IceTransport::Impl::initIceInstance(const IceTransportOptions& options) +{ + isTcp_ = options.tcpEnable; + upnpEnabled_ = options.upnpEnable; + on_initdone_cb_ = options.onInitDone; + on_negodone_cb_ = options.onNegoDone; + streamsCount_ = options.streamsCount; + compCountPerStream_ = options.compCountPerStream; + compCount_ = streamsCount_ * compCountPerStream_; + compIO_ = std::move(std::vector<ComponentIO>(compCount_)); + peerChannels_ = std::move(std::vector<PeerChannel>(compCount_)); + iceDefaultRemoteAddr_.reserve(compCount_); + initiatorSession_ = options.master; + accountLocalAddr_ = std::move(options.accountLocalAddr); + accountPublicAddr_ = std::move(options.accountPublicAddr); + stunServers_ = std::move(options.stunServers); + turnServers_ = std::move(options.turnServers); + + JAMI_DBG("[ice:%p] Initializing the session - comp count %u - as a %s", + this, + compCount_, + initiatorSession_ ? "master" : "slave"); + if (upnpEnabled_) upnp_.reset(new upnp::Controller()); @@ -1122,8 +1121,8 @@ IceTransport::Impl::onReceiveData(unsigned comp_id, void* pkt, pj_size_t size) //============================================================================== -IceTransport::IceTransport(const char* name, const IceTransportOptions& options) - : pimpl_ {std::make_unique<Impl>(name, options)} +IceTransport::IceTransport(const char* name) + : pimpl_ {std::make_unique<Impl>(name)} {} IceTransport::~IceTransport() @@ -1133,9 +1132,9 @@ IceTransport::~IceTransport() } void -IceTransport::initIceInstance() +IceTransport::initIceInstance(const IceTransportOptions& options) { - pimpl_->initIceInstance(); + pimpl_->initIceInstance(options); } bool @@ -1869,10 +1868,10 @@ IceTransportFactory::IceTransportFactory() IceTransportFactory::~IceTransportFactory() {} std::shared_ptr<IceTransport> -IceTransportFactory::createTransport(const char* name, const IceTransportOptions& options) +IceTransportFactory::createTransport(const char* name) { try { - return std::make_shared<IceTransport>(name, options); + return std::make_shared<IceTransport>(name); } catch (const std::exception& e) { JAMI_ERR("%s", e.what()); return nullptr; @@ -1880,10 +1879,10 @@ IceTransportFactory::createTransport(const char* name, const IceTransportOptions } std::unique_ptr<IceTransport> -IceTransportFactory::createUTransport(const char* name, const IceTransportOptions& options) +IceTransportFactory::createUTransport(const char* name) { try { - return std::make_unique<IceTransport>(name, options); + return std::make_unique<IceTransport>(name); } catch (const std::exception& e) { JAMI_ERR("%s", e.what()); return nullptr; diff --git a/src/ice_transport.h b/src/ice_transport.h index 646767f4b512fd3fe2ea8fcd7763f9bc9274a7ad..06f77ed8981ba82c9e48a2f29da49587e1279c86 100644 --- a/src/ice_transport.h +++ b/src/ice_transport.h @@ -130,10 +130,10 @@ public: /** * Constructor */ - IceTransport(const char* name, const IceTransportOptions& options = {}); + IceTransport(const char* name); ~IceTransport(); - void initIceInstance(); + void initIceInstance(const IceTransportOptions& options); /** * Get current state @@ -269,11 +269,9 @@ public: IceTransportFactory(); ~IceTransportFactory(); - std::shared_ptr<IceTransport> createTransport(const char* name, - const IceTransportOptions& options = {}); + std::shared_ptr<IceTransport> createTransport(const char* name); - std::unique_ptr<IceTransport> createUTransport(const char* name, - const IceTransportOptions& options = {}); + std::unique_ptr<IceTransport> createUTransport(const char* name); /** * PJSIP specifics diff --git a/src/jamidht/connectionmanager.cpp b/src/jamidht/connectionmanager.cpp index 903990979d008f3df7982c51827a4a37e8deec3d..ae92b4f6a314335ce2980ef1177d2157666338e6 100644 --- a/src/jamidht/connectionmanager.cpp +++ b/src/jamidht/connectionmanager.cpp @@ -571,10 +571,9 @@ ConnectionManager::Impl::connectDevice(const std::shared_ptr<dht::crypto::Certif ice_config.master = false; ice_config.streamsCount = JamiAccount::ICE_STREAMS_COUNT; ice_config.compCountPerStream = JamiAccount::ICE_COMP_COUNT_PER_STREAM; - info->ice_ = Manager::instance() - .getIceTransportFactory() - .createUTransport(sthis->account.getAccountID().c_str(), ice_config); - info->ice_->initIceInstance(); + info->ice_ = Manager::instance().getIceTransportFactory().createUTransport( + sthis->account.getAccountID().c_str()); + info->ice_->initIceInstance(ice_config); if (!info->ice_) { JAMI_ERR("Cannot initialize ICE session."); @@ -931,10 +930,9 @@ ConnectionManager::Impl::onDhtPeerRequest(const PeerConnectionRequest& req, ice_config.streamsCount = JamiAccount::ICE_STREAMS_COUNT; ice_config.compCountPerStream = JamiAccount::ICE_COMP_COUNT_PER_STREAM; ice_config.master = true; - info->ice_ = Manager::instance() - .getIceTransportFactory() - .createUTransport(shared->account.getAccountID().c_str(), ice_config); - info->ice_->initIceInstance(); + info->ice_ = Manager::instance().getIceTransportFactory().createUTransport( + shared->account.getAccountID().c_str()); + info->ice_->initIceInstance(ice_config); if (not info->ice_) { JAMI_ERR("Cannot initialize ICE session."); diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp index 4571f79062900e7b76b93b66da0fd95249995326..97052b720f428eea96544896b26f7efd42713daf 100644 --- a/src/jamidht/jamiaccount.cpp +++ b/src/jamidht/jamiaccount.cpp @@ -424,10 +424,14 @@ JamiAccount::newOutgoingCall(std::string_view toUrl, if (not call) return {}; - getIceOptions([=](auto&& opts) { - call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts)); - newOutgoingCallHelper(call, toUrl); - }); + if (call->isIceEnabled()) { + call->createIceMediaTransport(); + getIceOptions([=](auto&& opts) { + call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts)); + }); + } + + newOutgoingCallHelper(call, toUrl); return call; } @@ -445,10 +449,14 @@ JamiAccount::newOutgoingCall(std::string_view toUrl, const std::vector<DRing::Me if (not call) return {}; - getIceOptions([=](auto&& opts) { - call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts)); - newOutgoingCallHelper(call, toUrl); - }); + if (call->isIceEnabled()) { + call->createIceMediaTransport(); + getIceOptions([=](auto&& opts) { + call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts)); + }); + } + + newOutgoingCallHelper(call, toUrl); return call; } diff --git a/src/media/video/sinkclient.cpp b/src/media/video/sinkclient.cpp index 8a9ad5c5dfecff1fd66bbcf99d086574700666b1..cea0f780d65825a94069ec6cfd5258a09b0bfdf4 100644 --- a/src/media/video/sinkclient.cpp +++ b/src/media/video/sinkclient.cpp @@ -330,7 +330,7 @@ SinkClient::SinkClient(const std::string& id, bool mixer) , lastFrameDebug_(std::chrono::system_clock::now()) #endif { - JAMI_DBG("[Sink:%p] Sink [%.*s] created", this, getId().size(), getId().data()); + JAMI_DBG("[Sink:%p] Sink [%s] created", this, getId().c_str()); } void diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp index 38fd116e6a8470dc5e9539283b87a68f78bb5d4b..ca2750ce78ec7fe4a5cefea20038baaa6f2c2397 100644 --- a/src/sip/sipaccount.cpp +++ b/src/sip/sipaccount.cpp @@ -217,6 +217,7 @@ SIPAccount::newOutgoingCall(std::string_view toUrl, auto toUri = getToUri(to); if (call->isIceEnabled()) { + call->createIceMediaTransport(); call->initIceMediaTransport(true); } call->setPeerNumber(toUri); @@ -309,6 +310,7 @@ SIPAccount::newOutgoingCall(std::string_view toUrl, const std::vector<DRing::Med // Do not init ICE yet if the the media list is empty. This may occur // if we are sending an invite with no SDP offer. if (call->isIceEnabled() and not mediaList.empty()) { + call->createIceMediaTransport(); call->initIceMediaTransport(true); } diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp index 8419220002148a0e5b1cd72f3b7ab96ac4ce4174..c269d5a96b393fde6f0e39589379a2be9174d073 100644 --- a/src/sip/sipcall.cpp +++ b/src/sip/sipcall.cpp @@ -525,6 +525,7 @@ SIPCall::SIPSessionReinvite(const std::vector<MediaAttribute>& mediaAttrList) return !PJ_SUCCESS; if (isIceEnabled()) { + createIceMediaTransport(); if (initIceMediaTransport(true)) addLocalIceAttributes(); } @@ -864,6 +865,7 @@ SIPCall::answer(const std::vector<DRing::MediaMap>& mediaList) if (auto interfaceAddr = ip_utils::getInterfaceAddr(account->getLocalInterface(), publicAddr.getFamily())) { opts.accountLocalAddr = interfaceAddr; + createIceMediaTransport(); if (initIceMediaTransport(true, std::move(opts))) addLocalIceAttributes(); } else { @@ -2881,6 +2883,22 @@ SIPCall::InvSessionDeleter::operator()(pjsip_inv_session* inv) const noexcept pjsip_inv_dec_ref(inv); } +void +SIPCall::createIceMediaTransport() +{ + auto& iceTransportFactory = Manager::instance().getIceTransportFactory(); + std::lock_guard<std::mutex> lk(transportMtx_); + resetTransport(std::move(mediaTransport_)); + mediaTransport_ = iceTransportFactory.createTransport(getCallId().c_str()); + if (mediaTransport_) { + JAMI_DBG("[call:%s] Successfully created media ICE transport [ice:%p]", + getCallId().c_str(), + mediaTransport_.get()); + } else { + JAMI_ERR("[call:%s] Failed to create media ICE transport", getCallId().c_str()); + } +} + bool SIPCall::initIceMediaTransport(bool master, std::optional<IceTransportOptions> options) { @@ -2890,7 +2908,12 @@ SIPCall::initIceMediaTransport(bool master, std::optional<IceTransportOptions> o return false; } - JAMI_DBG("[call:%s] create media ICE transport", getCallId().c_str()); + JAMI_DBG("[call:%s] Init media ICE transport", getCallId().c_str()); + if (not mediaTransport_) { + JAMI_ERR("[call:%s] Failed to create media ICE transport", getCallId().c_str()); + return false; + } + auto iceOptions = options == std::nullopt ? acc->getIceOptions() : *options; auto optOnInitDone = std::move(iceOptions.onInitDone); @@ -2935,25 +2958,10 @@ SIPCall::initIceMediaTransport(bool master, std::optional<IceTransportOptions> o iceOptions.compCountPerStream = ICE_COMP_COUNT_PER_STREAM; auto& iceTransportFactory = Manager::instance().getIceTransportFactory(); - auto transport = iceTransportFactory.createUTransport(getCallId().c_str(), iceOptions); - - // Destroy old ice on a separate io pool - { - std::lock_guard<std::mutex> lk(transportMtx_); - resetTransport(std::move(mediaTransport_)); - mediaTransport_ = iceTransportFactory.createTransport(getCallId().c_str(), iceOptions); - } - - if (mediaTransport_) { - JAMI_DBG("[call:%s] Successfully created media ICE transport [ice:%p]", - getCallId().c_str(), - mediaTransport_.get()); - mediaTransport_->initIceInstance(); - } else { - JAMI_ERR("[call:%s] Failed to create media ICE transport", getCallId().c_str()); - } + // Init ICE. + mediaTransport_->initIceInstance(iceOptions); - return static_cast<bool>(mediaTransport_); + return true; } std::vector<std::string> @@ -3076,7 +3084,7 @@ SIPCall::setupIceResponse() onFailure(EIO); return; } - + createIceMediaTransport(); if (not initIceMediaTransport(false, opt)) { JAMI_ERR("[call:%s] ICE initialization failed", getCallId().c_str()); // Fatal condition diff --git a/src/sip/sipcall.h b/src/sip/sipcall.h index 59647726ce342a5023868ff18b7fa378dcceb91d..37cfed0f4e4a816f4da6a3388a0ecd5f09941867 100644 --- a/src/sip/sipcall.h +++ b/src/sip/sipcall.h @@ -278,6 +278,13 @@ public: void setPeerUri(const std::string& peerUri) { peerUri_ = peerUri; } + // Create a new ICE media session. If we already have an instance, + // it will be destroyed first. + void createIceMediaTransport(); + + // Initialize the ICE session. + // The initialization is performed asynchronously, i.e, the instance + // may not be ready to use when this method returns. bool initIceMediaTransport(bool master, std::optional<IceTransportOptions> options = std::nullopt); diff --git a/test/unitTest/ice/ice.cpp b/test/unitTest/ice/ice.cpp index 61bd4cda7d106c862bfdc6db7484a13af2162847..a85427ac1c1f83057832cc63728cfbab84d21af8 100644 --- a/test/unitTest/ice/ice.cpp +++ b/test/unitTest/ice/ice.cpp @@ -138,9 +138,8 @@ IceTest::testRawIceConnection() ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE", - ice_config); - ice_master->initIceInstance(); + ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE"); + ice_master->initIceInstance(ice_config); cv_create.notify_all(); ice_config.onInitDone = [&](bool ok) { CPPUNIT_ASSERT(ok); @@ -173,9 +172,8 @@ IceTest::testRawIceConnection() ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE", - ice_config); - ice_slave->initIceInstance(); + ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE"); + ice_slave->initIceInstance(ice_config); cv_create.notify_all(); CPPUNIT_ASSERT( @@ -247,9 +245,8 @@ IceTest::testTurnMasterIceConnection() ice_config.master = true; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE", - ice_config); - ice_master->initIceInstance(); + ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE"); + ice_master->initIceInstance(ice_config); cv_create.notify_all(); ice_config.turnServers = {}; ice_config.onInitDone = [&](bool ok) { @@ -291,9 +288,8 @@ IceTest::testTurnMasterIceConnection() ice_config.master = false; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE", - ice_config); - ice_slave->initIceInstance(); + ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE"); + ice_slave->initIceInstance(ice_config); cv_create.notify_all(); CPPUNIT_ASSERT( @@ -360,9 +356,8 @@ IceTest::testTurnSlaveIceConnection() ice_config.master = true; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE", - ice_config); - ice_master->initIceInstance(); + ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE"); + ice_master->initIceInstance(ice_config); cv_create.notify_all(); ice_config.onInitDone = [&](bool ok) { CPPUNIT_ASSERT(ok); @@ -408,9 +403,8 @@ IceTest::testTurnSlaveIceConnection() ice_config.master = false; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE", - ice_config); - ice_slave->initIceInstance(); + ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE"); + ice_slave->initIceInstance(ice_config); cv_create.notify_all(); CPPUNIT_ASSERT( cv.wait_for(lk, std::chrono::seconds(10), [&] { return iceMasterReady && iceSlaveReady; })); @@ -473,9 +467,8 @@ IceTest::testReceiveTooManyCandidates() ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE", - ice_config); - ice_master->initIceInstance(); + ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE"); + ice_master->initIceInstance(ice_config); cv_create.notify_all(); ice_config.onInitDone = [&](bool ok) { CPPUNIT_ASSERT(ok); @@ -516,9 +509,8 @@ IceTest::testReceiveTooManyCandidates() ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE", - ice_config); - ice_slave->initIceInstance(); + ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE"); + ice_slave->initIceInstance(ice_config); cv_create.notify_all(); CPPUNIT_ASSERT( cv.wait_for(lk, std::chrono::seconds(10), [&] { return iceMasterReady && iceSlaveReady; })); @@ -583,9 +575,8 @@ IceTest::testCompleteOnFailure() ice_config.master = true; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE", - ice_config); - ice_master->initIceInstance(); + ice_master = Manager::instance().getIceTransportFactory().createTransport("master ICE"); + ice_master->initIceInstance(ice_config); cv_create.notify_all(); ice_config.onInitDone = [&](bool ok) { CPPUNIT_ASSERT(ok); @@ -631,9 +622,8 @@ IceTest::testCompleteOnFailure() ice_config.master = false; ice_config.streamsCount = 1; ice_config.compCountPerStream = 1; - ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE", - ice_config); - ice_slave->initIceInstance(); + ice_slave = Manager::instance().getIceTransportFactory().createTransport("slave ICE"); + ice_slave->initIceInstance(ice_config); cv_create.notify_all(); // Check that nego failed and callback called CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(120), [&] {