Commit 2a676123 authored by Mohamed Chibani's avatar Mohamed Chibani Committed by Sébastien Blin
Browse files

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
parent b98db961
......@@ -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;
......
......@@ -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
......
......@@ -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.");
......
......@@ -424,10 +424,14 @@ JamiAccount::newOutgoingCall(std::string_view toUrl,
if (not call)
return {};
if (call->isIceEnabled()) {
call->createIceMediaTransport();
getIceOptions([=](auto&& opts) {
call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts));
newOutgoingCallHelper(call, toUrl);
});
}
newOutgoingCallHelper(call, toUrl);
return call;
}
......@@ -445,10 +449,14 @@ JamiAccount::newOutgoingCall(std::string_view toUrl, const std::vector<DRing::Me
if (not call)
return {};
if (call->isIceEnabled()) {
call->createIceMediaTransport();
getIceOptions([=](auto&& opts) {
call->initIceMediaTransport(true, std::forward<IceTransportOptions>(opts));
newOutgoingCallHelper(call, toUrl);
});
}
newOutgoingCallHelper(call, toUrl);
return call;
}
......
......@@ -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
......
......@@ -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);
}
......
......@@ -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);
// Init ICE.
mediaTransport_->initIceInstance(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());
}
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
......
......@@ -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);
......
......@@ -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), [&] {
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment