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.

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

minimizing signaling round trips required to set up connections #164

Open
saurik opened this issue Apr 23, 2021 · 5 comments
Open

minimizing signaling round trips required to set up connections #164

saurik opened this issue Apr 23, 2021 · 5 comments

Comments

@saurik
Copy link

saurik commented Apr 23, 2021

Hello! I've recently begun looking at mediasoup in some level of earnestness, and one of the things I've noticed is that setting up a connection--let's say a consumer--involves a number of round-trips to the server, which causes quite a bit of delay in receiving the video feed.

The steps in one random nicely-simple demo that I found (a weird one, though, which carries some "state" on the server) seems to be the following four requests (still for the consumer), which "adds up" and causes a noticeable delay when clicking the "subscribe" button.

  1. nothing to the server to create a transport and receive ice/dtls information
  2. rtp capabilities to the server to create the consumer and receive rtp parameters
  3. dtls parameters to the server in order to finalize and establish the connection
  4. nothing to the server to request that the remote consumer resume receiving

FWIW, I feel like the first two of these being separate steps might just be the random demo I found being "naive", but the API surface of mediasoup-client seems to really want that third step to be separate: it doesn't give you the dtls parameters until after you give it the rtp parameters.

Only, that's not a requirement of the WebRTC API: it is totally possible to get the fingerprints required for DTLS before having an offer in hand. (FWIW, Firefox doesn't support RTCCertificate.getFingerprints, but "worst case" you can create a dummy peer connection and parse its offer.)

I've thereby put together a "proof of concept" in the form of a fork of that random demo I found with a commit on a branch to show how to get the interaction with the server down to a single signaling round-trip for the consumer: saurik/mediasoup-sample-app@roundtrips

The one problem I ran into with this proof-of-concept in the existing mediasoup-client is that it takes the "additionalSettings" parameter and "sanitizes" it through utils.clone, which prevents me from smuggling an RTCCertificate object through the certificates array. The hack for that in Transport.ts:

// Clone and sanitize additionalSettings.
//additionalSettings = utils.clone(additionalSettings, {});
additionalSettings = additionalSettings || {};

Is better supporting this architecture something you'd consider for mediasoup{,-client}? For the consumer, arguably the minimum required would involve providing some way to pass through un-sanitized arguments to the RTCPeerConnection (so I could do it like this proof of concept).

But for the producer, I believe a different API is required, even though it is way more obvious why the producer should only need a single round trip :(... the client should be able to send the dtls fingerprint and rtp parameters at the same time, but it doesn't have a way to get the rtp parameters until after you have the remote ice candidates, which clearly isn't something the client actually needs to calculate those: it should only need the server's rtp capabilities. To fix this would seem to require a more different API of some kind (I haven't bothered to put any thought into what I'd personally do for this yet, as I'm kind of hoping that seeing that this is possible is "inspiring" enough ;P).

(If you need me to prove I can make this kind of flow work in Firefox without .getFingerprints()--potentially with deeper changes to the setup of the RTCPeerConnection, as I'd probably prefer to avoid creating a temporary RTCPeerConnection just to pull the fingerprints--I'd be happy to do that.)

@ibc
Copy link
Member

ibc commented Apr 23, 2021

FWIW, I feel like the first two of these being separate steps might just be the random demo I found being "naive", but the API surface of mediasoup-client seems to really want that third step to be separate: it doesn't give you the dtls parameters until after you give it the rtp parameters.

Only, that's not a requirement of the WebRTC API: it is totally possible to get the fingerprints required for DTLS before having an offer in hand. (FWIW, Firefox doesn't support RTCCertificate.getFingerprints, but "worst case" you can create a dummy peer connection and parse its offer.)

While having the DTLS parameters in advance may help reducing a round-trip, this is not implemented in all WebRTC clients, and AFAIK there is not way to get ICE parameters until you add a track or DataChannel into it.

As you say, it does not work in Firefox, which proves what I say. I'm open to consider this improvements but we need to provide a unified API that works in all modern and recent WebRTC clients.

@saurik
Copy link
Author

saurik commented Apr 23, 2021

As you say, it does not work in Firefox, which proves what I say.

Huh; but that isn't what I said? I said that Firefox doesn't implement .getFingerprints(), but that we definitely don't need that to make this work... I even offered to port it to Firefox. (I guess I should maybe take your response as a challenge? ;P)

AFAIK there is not way to get ICE parameters until you add a track or DataChannel into it.

The current architecture of mediasoup doesn't actually use ICE parameters from the client (and hence why I was able to not just claim this worked, but actually implement it quickly). Are you intending to change this (I'd probably find that exciting)?

FWIW, the minimal concrete request I'd thereby have could just be "can there be a way to get un-sanitized additionalSettings through createRecvTransport?" as that would potentially be useful for other purposes in addition to reducing round trips.

@ibc
Copy link
Member

ibc commented Apr 23, 2021

As you say, it does not work in Firefox, which proves what I say.

Huh; but that isn't what I said? I said that Firefox doesn't implement .getFingerprints(), but that we definitely don't need that to make this work... I even offered to port it to Firefox. (I guess I should maybe take your response as a challenge? ;P)

Do you mean implementing .getFingerprints() in Firefox itself? That would be nice, of course. BTW it's also missing in Safari iOS AFAIR. However, what to do with those browsers then?

AFAIK there is not way to get ICE parameters until you add a track or DataChannel into it.

The current architecture of mediasoup doesn't actually use ICE parameters from the client (and hence why I was able to not just claim this worked, but actually implement it quickly). Are you intending to change this (I'd probably find that exciting)?

That's right, but in case we eventually move mediasoup to full ICE (instead of ICE-Lite) then we are gonna need client's ICE parameters, and those are just generated after adding the first track or DataChannel into the PeerConnerction.

FWIW, the minimal concrete request I'd thereby have could just be "can there be a way to get un-sanitized additionalSettings through createRecvTransport?" as that would potentially be useful for other purposes in addition to reducing round trips.

"Un-sanitized" means that, for instance, TypeScript won't catch type errors. Not sure why we need un-sanitized (so TypeScript any types) if we can just add more typed properties into additionalSettings. Do I miss something?

Regarding the main proposal here (if I'right), saving a round-trip for signaling DTLS parameters to the server:

  • Transport instance in mediasoup-client, eventually, emits "connect" event local DTLS parameters.
  • Right now this event is emitted after the first Producer or DataProducer or Consumer or DataConsumer is created.
  • Once "connect" is emitted once, it's not emitted anymore (obviously).
  • So if getFingerprints() is supported, then Transport could immediately emit "connect" once created. Well, not immediately since nobody could be listening to "connect" event yet, but perhaps it could emit "connect" when a "connect" listener is added by the application.
  • This way, no changes in the API are required and "old" clients would work without having to change anything.

@saurik
Copy link
Author

saurik commented Apr 23, 2021

Do you mean implementing .getFingerprints() in Firefox itself? That would be nice, of course.

No... so, I'm saying I can make the code work on Firefox, in JavaScript, quite easily using the way I described: creating a dummy peer connection. This is a "weird" way to do it, but I'd bet I can do even better--just screwing around with the one peer connection--if I have a chance to mess with the original RTCPeerConnection object (which I can't do sitting outside the mediasoup-client API). (That said, I'm pretty sure Firefox has said they intend to implement .getFingerprints().)

BTW it's also missing in Safari iOS AFAIR.

According to MDN, both RTCCertificate and .getFingerprints() has been supported on iOS Safari since iOS 12.2.

https://developer.mozilla.org/en-US/docs/Web/API/RTCCertificate

However, what to do with those browsers then?

Here: I've updated the proof of concept at saurik/mediasoup-sample-app@roundtrips to use a way of doing this which avoids .getFingerprints(); the core code is just the following (with the if statement to show the old version using .getFingerprints() and the new version using SDP parsing). Again, I will repeat: if I were "inside" mediasoup-client's handler, I'd bet (but am not 100% sure) I could avoid the extra peer connection.

var dtlsParameters;
if (true) {
  const peer = new RTCPeerConnection({ certificates: [certificate] });
  await peer.createDataChannel({});
  const fingerprint = (await peer.createOffer()).sdp.match(/a=fingerprint:([^ ]*) ([:0-9A-Fa-f]*)/);
  dtlsParameters = { role: 'client', fingerprints: [{ algorithm: fingerprint[1], value: fingerprint[2] }] };
} else {
  dtlsParameters = { role: 'client', fingerprints: certificate.getFingerprints() };
  dtlsParameters.fingerprints[0].value = dtlsParameters.fingerprints[0].value.toUpperCase();
}

That's right, but in case we eventually move mediasoup to full ICE (instead of ICE-Lite) then we are gonna need client's ICE parameters, and those are just generated after adding the first track or DataChannel into the PeerConnerction.

Is it not possible to add a data channel to the peer connectionto get the ICE parameters to be built, and then add the media tracks later? (FWIW, I'd totally believe that that isn't possible, but I'd like to think that that is possible; I honestly wish very deeply that every mediasoup transport supported simultaneous data channel production/consumption--I appreciate the issues for media tracks, but would hope they don't apply to data channels--but that's a different sadness.)

"Un-sanitized" means that, for instance, TypeScript won't catch type errors. Not sure why we need un-sanitized (so TypeScript any types) if we can just add more typed properties into additionalSettings. Do I miss something?

Yeah, so the issue is that I need additionalSettings.certificates to be an array of RTCCertificate; but, because it is being passed through utils.clone, all of the "behavior" (prototypes, constructors, etc.) of the objects are being stripped (aka, "sanitized"). This is a runtime data munging issue, not a compile-type type issue. Usually people "sanitize" objects like this (by passing them through util.clone) because they might come from some uncontrolled script and they want to ensure that they are "clean" JSON with standard prototypes (a strict tree of Array and Object, with string, bool, and number values)... essentially, for "security". It isn't clear to me what the security need here is, as even if this comes from the server it would have to be JSON in such a case, so this is simply making it impossible to pass the certificates array through to RTCPeerConnection's constructor.

@ibc
Copy link
Member

ibc commented Apr 24, 2021

Again, I will repeat: if I were "inside" mediasoup-client's handler, I'd bet (but am not 100% sure) I could avoid the extra peer connection.

You can fork mediasoup-client for development purposes.

Is it not possible to add a data channel to the peer connectionto get the ICE parameters to be built

As you noticed, we don't need local ICE parameters if mediasoup remains being ICE Lite, so we are safe here. Otherwise, if we made mediasoup full ICE, we are not gonna create a fake DataChannel in mediasoup-client to just have immediate access to local ICE parameters.

Yeah, so the issue is that I need additionalSettings.certificates to be an array of RTCCertificate; but, because it is being passed through utils.clone

We don't need to pass certificates into additionalSettings. It could be a new field in settings main object.

Coming back to the origin of the topic, I agree there is room for improvement and I'll consider passing certificates to createXxxTransport() to save round trips. However, before that we are gonna work in the ability to use a single PeerConnection for sending and receiving media at the same time. Once we have that done we'll come back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants