From 2a6761236f06d8aa6f853e96538c70bb92ccacf0 Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Fri, 3 Sep 2021 14:17:38 -0400
Subject: [PATCH] ice: decouple instantiation and initialization of ICE
transport
Instantiation and initialization of ICE are done in a single
stage (in the class constructor).
To initialize the ICE instance, connection information must
first be gathered asynchronously.
However, when sharing ICE media instance between subcalls,
it's better to have a valid ICE instance as soon as possible
(even if not fully initialized) to proceed with the call
initialization process, then wait for ICE initialization to start
the call.
Thus, the ICE instantiation will be performed synchronously as
soon as the parent (main) call is created, then it will be
initialized asynchronously when the connection info are ready.
Gitlab: #619
Change-Id: I9c97516238f1a690603975ec968c8c6733155d4a
---
src/ice_transport.cpp | 73 +++++++++++++++----------------
src/ice_transport.h | 10 ++---
src/jamidht/connectionmanager.cpp | 14 +++---
src/jamidht/jamiaccount.cpp | 24 ++++++----
src/media/video/sinkclient.cpp | 2 +-
src/sip/sipaccount.cpp | 2 +
src/sip/sipcall.cpp | 48 +++++++++++---------
src/sip/sipcall.h | 7 +++
test/unitTest/ice/ice.cpp | 50 +++++++++------------
9 files changed, 120 insertions(+), 110 deletions(-)
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index e137291180..dd55873d8e 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 646767f4b5..06f77ed898 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 903990979d..ae92b4f6a3 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 4571f79062..97052b720f 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 8a9ad5c5df..cea0f780d6 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 38fd116e6a..ca2750ce78 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 8419220002..c269d5a96b 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 59647726ce..37cfed0f4e 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 61bd4cda7d..a85427ac1c 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), [&] {
--
GitLab