Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flatbuffers] Fix missing options in router.createWebRtcTransport() #1185

Merged
merged 8 commits into from
Oct 19, 2023
Merged

[flatbuffers] Fix missing options in router.createWebRtcTransport() #1185

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5c9291e
[flatbuffers] Fix missing options in router.createWebRtcTransport()
ibc Oct 19, 2023
51a6c62
Merge branch 'flatbuffers' into flatbuffer-fix-missing-create-webrtc-…
ibc Oct 19, 2023
93bef3d
WIP: Rust
ibc Oct 19, 2023
b10af01
WIP: Rust II
ibc Oct 19, 2023
1e46e35
TODO: This should make the test fail but it doesn't
ibc Oct 19, 2023
77dff05
Rust done
ibc Oct 19, 2023
8e9b313
cargo fmt
ibc Oct 19, 2023
cf0b033
Apply same change in the corresponding test in Node land
ibc Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions node/src/Router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@ export class Router<RouterAppData extends AppData = AppData>
listenInfos,
listenIps,
port,
enableUdp = true,
enableTcp = false,
enableUdp,
enableTcp,
preferUdp = false,
preferTcp = false,
initialAvailableOutgoingBitrate = 600000,
Expand Down Expand Up @@ -440,6 +440,19 @@ export class Router<RouterAppData extends AppData = AppData>
throw new TypeError('if given, appData must be an object');
}

// If webRtcServer is given, then do not force default values for enableUdp
// and enableTcp. Otherwise set them if unset.
if (webRtcServer)
{
enableUdp ??= true;
enableTcp ??= true;
}
else
{
enableUdp ??= true;
enableTcp ??= false;
}

// Convert deprecated TransportListenIps to TransportListenInfos.
if (listenIps)
{
Expand Down Expand Up @@ -545,7 +558,11 @@ export class Router<RouterAppData extends AppData = AppData>
webRtcServer ?
FbsWebRtcTransport.Listen.ListenServer :
FbsWebRtcTransport.Listen.ListenIndividual,
webRtcServer ? webRtcTransportListenServer : webRtcTransportListenIndividual
webRtcServer ? webRtcTransportListenServer : webRtcTransportListenIndividual,
enableUdp,
enableTcp,
preferUdp,
preferTcp
);

const requestOffset = new FbsRouter.CreateWebRtcTransportRequestT(
Expand Down
6 changes: 1 addition & 5 deletions node/src/WebRtcTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,25 +78,21 @@ export type WebRtcTransportOptionsBase<WebRtcTransportAppData> =
{
/**
* Listen in UDP. Default true.
* @deprecated
*/
enableUdp?: boolean;

/**
* Listen in TCP. Default false.
* @deprecated
* Listen in TCP. Default true if webrtcServer is given, false otherwise.
*/
enableTcp?: boolean;

/**
* Prefer UDP. Default false.
* @deprecated
*/
preferUdp?: boolean;

/**
* Prefer TCP. Default false.
* @deprecated
*/
preferTcp?: boolean;

Expand Down
304 changes: 262 additions & 42 deletions rust/src/fbs.rs
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions rust/src/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,10 @@ pub(crate) struct RouterCreateWebrtcTransportData {
#[serde(flatten)]
listen: RouterCreateWebrtcTransportListen,
initial_available_outgoing_bitrate: u32,
enable_udp: bool,
enable_tcp: bool,
prefer_udp: bool,
prefer_tcp: bool,
enable_sctp: bool,
num_sctp_streams: NumSctpStreams,
max_sctp_message_size: u32,
Expand Down Expand Up @@ -679,6 +683,10 @@ impl RouterCreateWebrtcTransportData {
},
initial_available_outgoing_bitrate: webrtc_transport_options
.initial_available_outgoing_bitrate,
enable_udp: webrtc_transport_options.enable_udp,
enable_tcp: webrtc_transport_options.enable_tcp,
prefer_udp: webrtc_transport_options.prefer_udp,
prefer_tcp: webrtc_transport_options.prefer_tcp,
enable_sctp: webrtc_transport_options.enable_sctp,
num_sctp_streams: webrtc_transport_options.num_sctp_streams,
max_sctp_message_size: webrtc_transport_options.max_sctp_message_size,
Expand All @@ -700,6 +708,10 @@ impl RouterCreateWebrtcTransportData {
is_data_channel: true,
}),
listen: self.listen.to_fbs(),
enable_udp: self.enable_udp,
ibc marked this conversation as resolved.
Show resolved Hide resolved
enable_tcp: self.enable_tcp,
prefer_udp: self.prefer_udp,
prefer_tcp: self.prefer_tcp,
}
}
}
Expand Down
22 changes: 21 additions & 1 deletion rust/src/router/webrtc_transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,18 @@ pub struct WebRtcTransportOptions {
/// Initial available outgoing bitrate (in bps).
/// Default 600000.
pub initial_available_outgoing_bitrate: u32,
/// Enable UDP.
/// Default true.
pub enable_udp: bool,
/// Enable TCP.
/// Default true if webrtc_server is given, false otherwise.
pub enable_tcp: bool,
/// Prefer UDP.
/// Default false.
pub prefer_udp: bool,
/// Prefer TCP.
/// Default false.
pub prefer_tcp: bool,
/// Create a SCTP association.
/// Default false.
pub enable_sctp: bool,
Expand All @@ -133,12 +145,16 @@ pub struct WebRtcTransportOptions {
}

impl WebRtcTransportOptions {
/// Create [`WebRtcTransport`] options with given listen IPs.
/// Create [`WebRtcTransport`] options with given listen infos.
#[must_use]
pub fn new(listen_infos: WebRtcTransportListenInfos) -> Self {
Self {
listen: WebRtcTransportListen::Individual { listen_infos },
initial_available_outgoing_bitrate: 600_000,
enable_udp: true,
enable_tcp: false,
prefer_udp: false,
prefer_tcp: false,
enable_sctp: false,
num_sctp_streams: NumSctpStreams::default(),
max_sctp_message_size: 262_144,
Expand All @@ -152,6 +168,10 @@ impl WebRtcTransportOptions {
Self {
listen: WebRtcTransportListen::Server { webrtc_server },
initial_available_outgoing_bitrate: 600_000,
enable_udp: true,
ibc marked this conversation as resolved.
Show resolved Hide resolved
enable_tcp: true,
prefer_udp: false,
prefer_tcp: false,
enable_sctp: false,
num_sctp_streams: NumSctpStreams::default(),
max_sctp_message_size: 262_144,
Expand Down
25 changes: 13 additions & 12 deletions rust/src/router/webrtc_transport/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,15 @@ fn create_with_webrtc_server_succeeds() {
});

let transport = router
.create_webrtc_transport(WebRtcTransportOptions::new_with_server(
webrtc_server.clone(),
))
.create_webrtc_transport({
let mut webrtc_transport_options = WebRtcTransportOptions::new_with_server(
webrtc_server.clone()
);
// Let's disable UDP so resulting ICE candidates should only contain TCP.
webrtc_transport_options.enable_udp = false;

webrtc_transport_options
})
.await
.expect("Failed to create WebRTC transport");

Expand All @@ -124,18 +130,13 @@ fn create_with_webrtc_server_succeeds() {

{
let ice_candidates = transport.ice_candidates();
assert_eq!(ice_candidates.len(), 2);
assert_eq!(ice_candidates.len(), 1);
assert_eq!(ice_candidates[0].ip, IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(ice_candidates[0].protocol, Protocol::Udp);
assert_eq!(ice_candidates[0].port, port1);
assert_eq!(ice_candidates[0].protocol, Protocol::Tcp);
assert_eq!(ice_candidates[0].port, port2);
assert_eq!(ice_candidates[0].r#type, IceCandidateType::Host);
Comment on lines +132 to 136
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had UDP and TCP before, why removing UDP?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I am testing enable_udp: false as I said. Just a few lines above:

let transport = router
            .create_webrtc_transport({
                let mut webrtc_transport_options = WebRtcTransportOptions::new_with_server(
                    webrtc_server.clone()
                );
                // Let's disable UDP so resulting ICE candidates should only contain TCP.
                webrtc_transport_options.enable_udp = false;

                webrtc_transport_options
            })
            .await
            .expect("Failed to create WebRTC transport");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is TCP testing no longer necessary here then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • That test creates a WebRtcServer that listens in UDP and TCP.
  • Then it creates a transport with webrtc_server and enable_udp: false, meaning that the resulting transport should only use the TCP ICE candidates of the given webrtc_server.
  • So the test asserts that there is only a ICE candidate (assert_eq!(ice_candidates.len(), 1);) and it's TCP.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what test is doing with these changes, but I don't understand what was wrong with the old test that you needed to change it.

Should you maybe add another test case instead? Also why no such change in TS, did TS test contain different conditions? (tests are usually symmetrical).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. I've done same change in Node test: cf0b033

Should you maybe add another test case instead?

There is little value in just asserting that a transport created with webRtcServer has the same ICE candidates than webRtcServer. So IMHO in those tests we can also test those enable_udp etc fields.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if it should do that, why not test it? 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because:

  • Jose and me have lot of work these days. I'm exhausted working on 3 different mediasoup PRs in parallel and doing things at work.
  • The current API sucks. It got way worse after adding webRtcServer since then we allow listenIps OR webRtcServer but not both, and some other options only make sense for one of them, etc etc. I know it and I'd love to refactor it in v4, but we have to keep backwards compatibility.

But all those are excuses and you are right.

assert_eq!(ice_candidates[0].tcp_type, None);
assert_eq!(ice_candidates[1].ip, IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(ice_candidates[1].protocol, Protocol::Tcp);
assert_eq!(ice_candidates[1].port, port2);
assert_eq!(ice_candidates[1].r#type, IceCandidateType::Host);
assert_eq!(
ice_candidates[1].tcp_type,
ice_candidates[0].tcp_type,
Some(IceCandidateTcpType::Passive)
);
}
Expand Down
1 change: 0 additions & 1 deletion rust/tests/integration/webrtc_transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ fn create_succeeds() {
.try_into()
.unwrap(),
);
// TODO: I wonder how this was not needed before...
webrtc_transport_options.enable_sctp = true;
webrtc_transport_options.num_sctp_streams = NumSctpStreams {
os: 2048,
Expand Down
1 change: 0 additions & 1 deletion worker/fbs/transport.fbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ table RecvRtpHeaderExtensions {

table Options {
direct: bool = false;

/// Only needed for DirectTransport. This value is handled by base Transport.
max_message_size: uint32 = null;
initial_available_outgoing_bitrate: uint32 = null;
Expand Down
4 changes: 4 additions & 0 deletions worker/fbs/webRtcTransport.fbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ union Listen {
table WebRtcTransportOptions {
base: FBS.Transport.Options (required);
listen: Listen (required);
enable_udp: bool = true;
enable_tcp: bool = true;
prefer_udp: bool = false;
prefer_tcp: bool = false;
}

enum FingerprintAlgorithm: uint8 {
Expand Down
5 changes: 2 additions & 3 deletions worker/include/RTC/WebRtcServer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ namespace RTC
public:
flatbuffers::Offset<FBS::WebRtcServer::DumpResponse> FillBuffer(
flatbuffers::FlatBufferBuilder& builder) const;
const std::vector<RTC::IceCandidate>& GetIceCandidates() const;
const std::vector<RTC::IceCandidate> GetIceCandidates(
bool enableUdp, bool enableTcp, bool preferUdp, bool preferTcp) const;

/* Methods inherited from Channel::ChannelSocket::RequestHandler. */
public:
Expand Down Expand Up @@ -102,8 +103,6 @@ namespace RTC
absl::flat_hash_map<std::string, RTC::WebRtcTransport*> mapLocalIceUsernameFragmentWebRtcTransport;
// Map of WebRtcTransports indexed by TransportTuple.hash.
absl::flat_hash_map<uint64_t, RTC::WebRtcTransport*> mapTupleWebRtcTransport;
// ICE candidates.
std::vector<RTC::IceCandidate> iceCandidates;
};
} // namespace RTC

Expand Down
3 changes: 2 additions & 1 deletion worker/src/RTC/Router.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ namespace RTC
if (!webRtcServer)
MS_THROW_ERROR("wrong webRtcServerId (no associated WebRtcServer found)");

auto& iceCandidates = webRtcServer->GetIceCandidates();
auto iceCandidates = webRtcServer->GetIceCandidates(
options->enableUdp(), options->enableTcp(), options->preferUdp(), options->preferTcp());

// This may throw.
auto* webRtcTransport = new RTC::WebRtcTransport(
Expand Down
86 changes: 55 additions & 31 deletions worker/src/RTC/WebRtcServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ namespace RTC

try
{
uint16_t iceLocalPreferenceDecrement{ 0u };

this->iceCandidates.reserve(listenInfos->size());

for (const auto* listenInfo : *listenInfos)
{
auto ip = listenInfo->ip()->str();
Expand All @@ -64,10 +60,6 @@ namespace RTC
announcedIp = listenInfo->announcedIp()->str();
}

const uint16_t iceLocalPreference =
IceCandidateDefaultLocalPriority - iceLocalPreferenceDecrement;
const uint32_t icePriority = generateIceCandidatePriority(iceLocalPreference);

if (listenInfo->protocol() == FBS::Transport::Protocol::UDP)
{
// This may throw.
Expand All @@ -84,15 +76,6 @@ namespace RTC

this->udpSocketOrTcpServers.emplace_back(udpSocket, nullptr, announcedIp);

if (announcedIp.size() == 0)
{
this->iceCandidates.emplace_back(udpSocket, icePriority);
}
else
{
this->iceCandidates.emplace_back(udpSocket, icePriority, announcedIp);
}

if (listenInfo->sendBufferSize() != 0)
{
udpSocket->SetSendBufferSize(listenInfo->sendBufferSize());
Expand Down Expand Up @@ -125,15 +108,6 @@ namespace RTC

this->udpSocketOrTcpServers.emplace_back(nullptr, tcpServer, announcedIp);

if (announcedIp.size() == 0)
{
this->iceCandidates.emplace_back(tcpServer, icePriority);
}
else
{
this->iceCandidates.emplace_back(tcpServer, icePriority, announcedIp);
}

if (listenInfo->sendBufferSize() != 0)
{
tcpServer->SetSendBufferSize(listenInfo->sendBufferSize());
Expand All @@ -150,9 +124,6 @@ namespace RTC
tcpServer->GetSendBufferSize(),
tcpServer->GetRecvBufferSize());
}

// Decrement initial ICE local preference for next IP.
iceLocalPreferenceDecrement += 100;
}

// NOTE: This may throw.
Expand Down Expand Up @@ -289,11 +260,64 @@ namespace RTC
}
}

const std::vector<RTC::IceCandidate>& WebRtcServer::GetIceCandidates() const
const std::vector<RTC::IceCandidate> WebRtcServer::GetIceCandidates(
bool enableUdp, bool enableTcp, bool preferUdp, bool preferTcp) const
{
MS_TRACE();

return this->iceCandidates;
std::vector<RTC::IceCandidate> iceCandidates;
uint16_t iceLocalPreferenceDecrement{ 0 };

for (auto& item : this->udpSocketOrTcpServers)
{
if (item.udpSocket && enableUdp)
{
uint16_t iceLocalPreference = IceCandidateDefaultLocalPriority - iceLocalPreferenceDecrement;

if (preferUdp)
{
iceLocalPreference += 1000;
}

uint32_t icePriority = generateIceCandidatePriority(iceLocalPreference);

if (item.announcedIp.empty())
{
iceCandidates.emplace_back(item.udpSocket, icePriority);
}
else
{
iceCandidates.emplace_back(
item.udpSocket, icePriority, const_cast<std::string&>(item.announcedIp));
}
}
else if (item.tcpServer && enableTcp)
{
uint16_t iceLocalPreference = IceCandidateDefaultLocalPriority - iceLocalPreferenceDecrement;

if (preferTcp)
{
iceLocalPreference += 1000;
}

uint32_t icePriority = generateIceCandidatePriority(iceLocalPreference);

if (item.announcedIp.empty())
{
iceCandidates.emplace_back(item.tcpServer, icePriority);
}
else
{
iceCandidates.emplace_back(
item.tcpServer, icePriority, const_cast<std::string&>(item.announcedIp));
}
}

// Decrement initial ICE local preference for next IP.
iceLocalPreferenceDecrement += 100;
}

return iceCandidates;
}

inline std::string WebRtcServer::GetLocalIceUsernameFragmentFromReceivedStunPacket(
Expand Down
3 changes: 3 additions & 0 deletions worker/src/RTC/WebRtcTransport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ namespace RTC

/* Instance methods. */

/**
* This constructor is used when the WebRtcTransport doesn't use a WebRtcServer.
*/
WebRtcTransport::WebRtcTransport(
RTC::Shared* shared,
const std::string& id,
Expand Down
Loading