[flatbuffers] Fix missing options in router.createWebRtcTransport() #1185
Conversation
Fixes #1184 ### Details - No idea how, but flatbuffers branch misses `enableUdp`, `enableTcp`, and `preferUdp` and `preferTcp` options when creating a `WebRtcTransport` with a `WebRtcServer`. This was implemented in v3. ### TODO Rust side, but I prefer to have the Node + C++ sides approved first. ### NOTE I'm extremely worried that something else may have been lost, specially in `WebRtcServer` cpp and hpp files.
There was a problem hiding this comment.
I'm extremely worried that something else may have been lost, specially in WebRtcServer cpp and hpp files.
It was you in this commit. One of those in which most of changes are unrelated to the initial intent :-).
|
Can you please recheck that commit link? It doesn't work for me. |
Edited the link. |
So for whatever reason, setting `webrtc_transport_options.enable_udp = false` is ignored. Such a `enable_udp: false` value is not sent through the channel but instead the worker receives `true`. Why?
|
We have to/from_fbs methods, have you updated them all? |
|
Rust done and test modified to use the new stuff. PR ready. @nazar-pc can you please press the "Approve" button? |
| 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); |
There was a problem hiding this comment.
We had UDP and TCP before, why removing UDP?
There was a problem hiding this comment.
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");There was a problem hiding this comment.
Why is TCP testing no longer necessary here then?
There was a problem hiding this comment.
- That test creates a
WebRtcServerthat listens in UDP and TCP. - Then it creates a transport with
webrtc_serverandenable_udp: false, meaning that the resulting transport should only use the TCP ICE candidates of the givenwebrtc_server. - So the test asserts that there is only a ICE candidate (
assert_eq!(ice_candidates.len(), 1);) and it's TCP.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I mean if it should do that, why not test it? 🙂
There was a problem hiding this comment.
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
webRtcServersince then we allowlistenIpsORwebRtcServerbut 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.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); |
There was a problem hiding this comment.
I mean if it should do that, why not test it? 🙂
ibc
deleted the
flatbuffer-fix-missing-create-webrtc-transport-options
branch
Fixes #1184
Details
enableUdp,enableTcp, andpreferUdpandpreferTcpoptions when creating aWebRtcTransportwith aWebRtcServer. This was implemented in v3.