Skip to content

Commit 063b85b

Browse files
MEDIA-4254: Add private-port flag for ICE endpoints (#407)
1 parent d65deb2 commit 063b85b

File tree

11 files changed

+158
-44
lines changed

11 files changed

+158
-44
lines changed

api/AllocateEndpoint.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,14 @@ struct AllocateEndpoint
1414
{
1515
struct Transport
1616
{
17-
Transport() : ice(false), dtls(false), sdes(false) {}
17+
Transport() : ice(false), dtls(false), sdes(false), privatePort(false) {}
1818

1919
bool ice;
2020
utils::Optional<bool> iceControlling;
2121

2222
bool dtls;
2323
bool sdes;
24+
bool privatePort;
2425
};
2526

2627
struct Audio

api/Parser.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ api::AllocateEndpoint::Transport parseAllocateEndpointTransport(const nlohmann::
144144

145145
setIfExists(transport.dtls, data, "dtls");
146146
setIfExists(transport.sdes, data, "sdes");
147+
setIfExists(transport.privatePort, data, "private-port");
147148

148149
return transport;
149150
}

bridge/LegacyApiRequestHandler.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -971,7 +971,7 @@ bool LegacyApiRequestHandler::allocateChannel(const std::string& contentName,
971971

972972
if (useBundling)
973973
{
974-
mixer.addBundleTransportIfNeeded(endpointId, iceRole.get(), true);
974+
mixer.addBundleTransportIfNeeded(endpointId, iceRole.get(), true, false);
975975
if (contentType == ContentType::Audio)
976976
{
977977
if (!mixer.addBundledAudioStream(channelId, endpointId, channel.getMediaMode()))
@@ -993,15 +993,15 @@ bool LegacyApiRequestHandler::allocateChannel(const std::string& contentName,
993993
{
994994
if (contentType == ContentType::Audio)
995995
{
996-
if (!mixer.addAudioStream(channelId, endpointId, iceRole, channel.getMediaMode()))
996+
if (!mixer.addAudioStream(channelId, endpointId, iceRole, channel.getMediaMode(), false))
997997
{
998998
outStatus = httpd::StatusCode::BAD_REQUEST;
999999
return false;
10001000
}
10011001
}
10021002
else if (contentType == ContentType::Video)
10031003
{
1004-
if (!mixer.addVideoStream(channelId, endpointId, iceRole, channel.isRelayTypeRewrite()))
1004+
if (!mixer.addVideoStream(channelId, endpointId, iceRole, channel.isRelayTypeRewrite(), false))
10051005
{
10061006
outStatus = httpd::StatusCode::BAD_REQUEST;
10071007
return false;
@@ -1075,7 +1075,7 @@ bool LegacyApiRequestHandler::allocateSctpConnection(const std::string& conferen
10751075
{
10761076
iceRole.set(ice::IceRole::CONTROLLING);
10771077
}
1078-
mixer.addBundleTransportIfNeeded(endpointId, iceRole.get(), true);
1078+
mixer.addBundleTransportIfNeeded(endpointId, iceRole.get(), true, false);
10791079
mixer.addBundledDataStream(streamId, endpointId);
10801080

10811081
uint32_t totalSleepTimeMs = 0;

bridge/Mixer.cpp

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -272,15 +272,17 @@ bool Mixer::hasPendingTransportJobs()
272272

273273
bool Mixer::addBundleTransportIfNeeded(const std::string& endpointId,
274274
const ice::IceRole iceRole,
275-
const bool hasVideoEnabled)
275+
const bool hasVideoEnabled,
276+
const bool usePrivatePort)
276277
{
277278
std::lock_guard<std::mutex> locker(_configurationLock);
278279
if (_bundleTransports.find(endpointId) != _bundleTransports.end())
279280
{
280281
return true;
281282
}
282283

283-
if (!_useGlobalPort && _rtpPorts.empty())
284+
const bool needToOpenConferencePorts = !usePrivatePort && !_useGlobalPort && _rtpPorts.empty();
285+
if (needToOpenConferencePorts)
284286
{
285287
if (!_transportFactory.openRtpMuxPorts(_rtpPorts, 1024))
286288
{
@@ -304,7 +306,8 @@ bool Mixer::addBundleTransportIfNeeded(const std::string& endpointId,
304306
expectedOutboundStreamCount,
305307
jobQueueSize,
306308
enableUplinkEstimation,
307-
true)
309+
true,
310+
usePrivatePort)
308311
: _transportFactory.createOnPorts(iceRole,
309312
endpointIdHash,
310313
_rtpPorts,
@@ -342,6 +345,7 @@ bool Mixer::addAudioStream(std::string& outId,
342345
const std::string& endpointId,
343346
const utils::Optional<ice::IceRole>& iceRole,
344347
const MediaMode mediaMode,
348+
const bool usePrivatePort,
345349
utils::Optional<uint32_t> idleTimeoutSeconds)
346350
{
347351
std::lock_guard<std::mutex> locker(_configurationLock);
@@ -352,8 +356,9 @@ bool Mixer::addAudioStream(std::string& outId,
352356
}
353357

354358
outId = std::to_string(_idGenerator.next());
355-
auto transport = iceRole.isSet() ? _transportFactory.create(iceRole.get(), utils::hash<std::string>{}(endpointId))
356-
: _transportFactory.create(utils::hash<std::string>{}(endpointId));
359+
auto transport = iceRole.isSet()
360+
? _transportFactory.create(iceRole.get(), utils::hash<std::string>{}(endpointId), usePrivatePort)
361+
: _transportFactory.create(utils::hash<std::string>{}(endpointId));
357362

358363
if (!transport)
359364
{
@@ -390,6 +395,7 @@ bool Mixer::addVideoStream(std::string& outId,
390395
const std::string& endpointId,
391396
const utils::Optional<ice::IceRole>& iceRole,
392397
bool rewriteSsrcs,
398+
const bool usePrivatePort,
393399
utils::Optional<uint32_t> idleTimeoutSeconds)
394400
{
395401
std::lock_guard<std::mutex> locker(_configurationLock);
@@ -409,8 +415,9 @@ bool Mixer::addVideoStream(std::string& outId,
409415
}
410416

411417
outId = std::to_string(_idGenerator.next());
412-
auto transport = iceRole.isSet() ? _transportFactory.create(iceRole.get(), utils::hash<std::string>{}(endpointId))
413-
: _transportFactory.create(utils::hash<std::string>{}(endpointId));
418+
auto transport = iceRole.isSet()
419+
? _transportFactory.create(iceRole.get(), utils::hash<std::string>{}(endpointId), usePrivatePort)
420+
: _transportFactory.create(utils::hash<std::string>{}(endpointId));
414421

415422
if (!transport)
416423
{

bridge/Mixer.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,17 +107,20 @@ class Mixer
107107

108108
bool addBundleTransportIfNeeded(const std::string& endpointId,
109109
const ice::IceRole iceRole,
110-
const bool hasVideoEnabled);
110+
const bool hasVideoEnabled,
111+
const bool usePrivatePort);
111112

112113
bool addAudioStream(std::string& outId,
113114
const std::string& endpointId,
114115
const utils::Optional<ice::IceRole>& iceRole,
115116
MediaMode mediaMode,
117+
const bool usePrivatePort,
116118
utils::Optional<uint32_t> idleTimeoutSeconds = utils::Optional<uint32_t>());
117119
bool addVideoStream(std::string& outId,
118120
const std::string& endpointId,
119121
const utils::Optional<ice::IceRole>& iceRole,
120122
bool rewriteSsrcs,
123+
const bool usePrivatePort,
121124
utils::Optional<uint32_t> idleTimeoutSeconds = utils::Optional<uint32_t>());
122125

123126
bool addBundledAudioStream(std::string& outId,

bridge/endpointActions/ConferenceActions.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ httpd::Response allocateEndpoint(ActionContext* context,
316316

317317
const bool hasVideoEnabled = allocateChannel.video.isSet();
318318

319-
mixer->addBundleTransportIfNeeded(endpointId, iceRole, hasVideoEnabled);
319+
mixer->addBundleTransportIfNeeded(endpointId, iceRole, hasVideoEnabled, bundleTransport.privatePort);
320320

321321
if (allocateChannel.audio.isSet())
322322
{
@@ -388,6 +388,7 @@ httpd::Response allocateEndpoint(ActionContext* context,
388388
endpointId,
389389
iceRole,
390390
audio.getMediaMode(),
391+
transport.privatePort,
391392
allocateChannel.idleTimeoutSeconds))
392393
{
393394
throw httpd::RequestErrorException(httpd::StatusCode::INTERNAL_SERVER_ERROR,
@@ -421,6 +422,7 @@ httpd::Response allocateEndpoint(ActionContext* context,
421422
endpointId,
422423
iceRole,
423424
ssrcRewrite,
425+
transport.privatePort,
424426
allocateChannel.idleTimeoutSeconds))
425427
{
426428
videoChannelId.set(outChannelId);

test/bridge/ApiRequestHandlerTest.cpp

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11

22
#include "bridge/ApiRequestHandler.h"
33
#include "bridge/Mixer.h"
4+
#include "bridge/TransportDescription.h"
45
#include "mocks/EngineMixerSpy.h"
56
#include "mocks/MixerManagerSpy.h"
67
#include "mocks/RtcTransportMock.h"
@@ -384,3 +385,104 @@ TEST_F(ApiRequestHandlerTest, allocateEndpointWithVideoFieldWhenVideoIsEnableSho
384385
EXPECT_NE(responseJson.end(), responseJson.find("video"));
385386
EXPECT_NE(responseJson.end(), responseJson.find("data"));
386387
}
388+
389+
TEST_F(ApiRequestHandlerTest, allocateEndpointWithPrivateShouldPassFlagToFactory)
390+
{
391+
auto requestHandler = createApiRequestHandler();
392+
393+
const auto conferenceId = createConference(requestHandler, R"({
394+
"last-n": 9,
395+
"enable-video": true
396+
})");
397+
398+
const char* body = R"({
399+
"action": "allocate",
400+
"bundle-transport": {
401+
"ice": true,
402+
"dtls": true,
403+
"private-port": true
404+
},
405+
"audio": {
406+
"relay-type": "ssrc-rewrite"
407+
}
408+
})";
409+
410+
const auto urlPath = std::string("/conferences/").append(conferenceId).append("/session0");
411+
412+
httpd::Request request("POST", urlPath.c_str());
413+
request.body.append(body, strlen(body));
414+
415+
EXPECT_CALL(*_mixerManagerSpyResources->transportFactoryMock, create(_, _, _, _, _, _, _, true)).Times(1);
416+
417+
const auto response = requestHandler.onRequest(request);
418+
419+
EXPECT_EQ(httpd::StatusCode::OK, response.statusCode);
420+
EXPECT_EQ(false, response.body.empty());
421+
422+
Mixer* mixer = nullptr;
423+
424+
auto lock = _mixerManagerSpy->getMixer(conferenceId, mixer);
425+
426+
ASSERT_NE(nullptr, mixer);
427+
428+
bridge::TransportDescription transportDescription;
429+
ASSERT_TRUE(mixer->getTransportBundleDescription("session0", transportDescription));
430+
}
431+
432+
TEST_F(ApiRequestHandlerTest, allocateEndpointWithoutPrivateShouldPassFlagToFactory)
433+
{
434+
auto requestHandler = createApiRequestHandler();
435+
436+
const auto conferenceId = createConference(requestHandler, R"({
437+
"last-n": 9,
438+
"enable-video": true
439+
})");
440+
441+
const char* body0 = R"({
442+
"action": "allocate",
443+
"bundle-transport": {
444+
"ice": true,
445+
"dtls": true,
446+
"private-port": false
447+
},
448+
"audio": {
449+
"relay-type": "ssrc-rewrite"
450+
}
451+
})";
452+
453+
const char* body1 = R"({
454+
"action": "allocate",
455+
"bundle-transport": {
456+
"ice": true,
457+
"dtls": true
458+
},
459+
"audio": {
460+
"relay-type": "ssrc-rewrite"
461+
}
462+
})";
463+
464+
EXPECT_CALL(*_mixerManagerSpyResources->transportFactoryMock,
465+
create(_, utils::hash<std::string>{}("session2"), _, _, _, _, _, false))
466+
.Times(1);
467+
468+
const auto urlPath0 = std::string("/conferences/").append(conferenceId).append("/session2");
469+
const auto urlPath1 = std::string("/conferences/").append(conferenceId).append("/session3");
470+
471+
httpd::Request request0("POST", urlPath0.c_str());
472+
request0.body.append(body0, strlen(body0));
473+
474+
const auto response0 = requestHandler.onRequest(request0);
475+
EXPECT_EQ(httpd::StatusCode::OK, response0.statusCode);
476+
EXPECT_EQ(false, response0.body.empty());
477+
478+
EXPECT_CALL(*_mixerManagerSpyResources->transportFactoryMock,
479+
create(_, utils::hash<std::string>{}("session3"), _, _, _, _, _, false))
480+
.Times(1);
481+
482+
httpd::Request request1("POST", urlPath1.c_str());
483+
request1.body.append(body1, strlen(body1));
484+
485+
const auto response1 = requestHandler.onRequest(request1);
486+
EXPECT_EQ(httpd::StatusCode::OK, response1.statusCode);
487+
EXPECT_EQ(false, response1.body.empty());
488+
}

test/bridge/MixerTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ class MixerTest : public ::testing::Test
236236
bool useData)
237237
{
238238
std::string outId;
239-
CHECK(mixer.addBundleTransportIfNeeded(endpointId, ice::IceRole::CONTROLLING, true));
239+
CHECK(mixer.addBundleTransportIfNeeded(endpointId, ice::IceRole::CONTROLLING, true, false));
240240
CHECK(mixer.addBundledAudioStream(outId, endpointId, MediaMode::SSRC_REWRITE));
241241
CHECK(mixer.addBundledVideoStream(outId, endpointId, true));
242242
CHECK(mixer.addBundledDataStream(outId, endpointId));

test/include/mocks/TransportFactoryMock.h

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,12 @@ class TransportFactoryMock : public transport::TransportFactory
1818
size_t expectedOutboundStreamCount,
1919
size_t jobQueueSize,
2020
bool enableUplinkEstimation,
21-
bool enableDownlinkEstimation),
21+
bool enableDownlinkEstimation,
22+
bool privatePort),
2223
(override));
2324
MOCK_METHOD(std::shared_ptr<transport::RtcTransport>,
2425
create,
25-
(const ice::IceRole iceRole, const size_t endpointId),
26+
(const ice::IceRole iceRole, const size_t endpointId, bool privatePort),
2627
(override));
2728

2829
MOCK_METHOD(std::shared_ptr<transport::RtcTransport>, create, (const size_t endpointIdHash), (override));
@@ -100,8 +101,8 @@ class TransportFactoryMock : public transport::TransportFactory
100101
using namespace ::testing;
101102

102103
ON_CALL(*this, create(_)).WillByDefault(Return(rtcTransport));
103-
ON_CALL(*this, create(_, _)).WillByDefault(Return(rtcTransport));
104-
ON_CALL(*this, create(_, _, _, _, _, _, _)).WillByDefault(Return(rtcTransport));
104+
ON_CALL(*this, create(_, _, _)).WillByDefault(Return(rtcTransport));
105+
ON_CALL(*this, create(_, _, _, _, _, _, _, _)).WillByDefault(Return(rtcTransport));
105106
ON_CALL(*this, createOnSharedPort(_, _)).WillByDefault(Return(rtcTransport));
106107
ON_CALL(*this, createOnSharedPort(_, _, _, _, _, _, _)).WillByDefault(Return(rtcTransport));
107108
ON_CALL(*this, createOnPrivatePort(_, _)).WillByDefault(Return(rtcTransport));
@@ -112,26 +113,17 @@ class TransportFactoryMock : public transport::TransportFactory
112113
{
113114
using namespace ::testing;
114115

115-
const auto callback1Args = [=](const auto&) {
116-
return rtcTransport.lock();
117-
};
118-
119-
const auto callback2Args = [=](const auto&, const auto&) {
116+
const auto callbackNArgs = [=](const auto&... args) {
120117
return rtcTransport.lock();
121118
};
122119

123-
const auto callback7Args =
124-
[=](const auto&, const auto&, const auto&, const auto&, const auto&, const auto&, const auto&) {
125-
return rtcTransport.lock();
126-
};
127-
128-
ON_CALL(*this, create(_)).WillByDefault(callback1Args);
129-
ON_CALL(*this, create(_, _)).WillByDefault(callback2Args);
130-
ON_CALL(*this, create(_, _, _, _, _, _, _)).WillByDefault(callback7Args);
131-
ON_CALL(*this, createOnSharedPort(_, _)).WillByDefault(callback2Args);
132-
ON_CALL(*this, createOnSharedPort(_, _, _, _, _, _, _)).WillByDefault(callback7Args);
133-
ON_CALL(*this, createOnPrivatePort(_, _)).WillByDefault(callback2Args);
134-
ON_CALL(*this, createOnPrivatePort(_, _, _, _, _, _, _)).WillByDefault(callback7Args);
120+
ON_CALL(*this, create(_)).WillByDefault(callbackNArgs);
121+
ON_CALL(*this, create(_, _, _)).WillByDefault(callbackNArgs);
122+
ON_CALL(*this, create(_, _, _, _, _, _, _, _)).WillByDefault(callbackNArgs);
123+
ON_CALL(*this, createOnSharedPort(_, _)).WillByDefault(callbackNArgs);
124+
ON_CALL(*this, createOnSharedPort(_, _, _, _, _, _, _)).WillByDefault(callbackNArgs);
125+
ON_CALL(*this, createOnPrivatePort(_, _)).WillByDefault(callbackNArgs);
126+
ON_CALL(*this, createOnPrivatePort(_, _, _, _, _, _, _)).WillByDefault(callbackNArgs);
135127
}
136128
};
137129

transport/TransportFactory.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -340,11 +340,12 @@ class TransportFactoryImpl final : public TransportFactory,
340340
size_t expectedOutboundStreamCount,
341341
size_t jobQueueSize,
342342
bool enableUplinkEstimation,
343-
bool enableDownlinkEstimation) override
343+
bool enableDownlinkEstimation,
344+
bool privatePort) override
344345
{
345-
if (!_sharedEndpoints.empty())
346+
if (privatePort || _sharedEndpoints.empty())
346347
{
347-
return createOnSharedPort(iceRole,
348+
return createOnPrivatePort(iceRole,
348349
endpointId,
349350
expectedInboundStreamCount,
350351
expectedOutboundStreamCount,
@@ -353,7 +354,7 @@ class TransportFactoryImpl final : public TransportFactory,
353354
enableDownlinkEstimation);
354355
}
355356

356-
return createOnPrivatePort(iceRole,
357+
return createOnSharedPort(iceRole,
357358
endpointId,
358359
expectedInboundStreamCount,
359360
expectedOutboundStreamCount,
@@ -362,9 +363,11 @@ class TransportFactoryImpl final : public TransportFactory,
362363
enableDownlinkEstimation);
363364
}
364365

365-
std::shared_ptr<RtcTransport> create(const ice::IceRole iceRole, const size_t endpointId) override
366+
std::shared_ptr<RtcTransport> create(const ice::IceRole iceRole,
367+
const size_t endpointId,
368+
const bool privatePort) override
366369
{
367-
return create(iceRole, endpointId, 16, 256, 4096, true, true);
370+
return create(iceRole, endpointId, 16, 256, 4096, true, true, privatePort);
368371
}
369372

370373
std::shared_ptr<RtcTransport> createOnPrivatePort(const ice::IceRole iceRole, const size_t endpointId) override

0 commit comments

Comments
 (0)