Decouple signaling and WebRTC stuff

[ibc]

A perfect rationale is given in the Janus NoSIP plugin discussion.

This is: make JsSIP WebRTC agnostic and let the app provide JsSIP with externally produced SDP blobs.

[michelepra]

what you think about my solution?

[jmordica]

Is this to support interop with legacy sip servers?

If not, can someone point me to an open issue that contains such?

Thanks!

[ibc]

Define "interop with legacy SIP server". A browser cannot have a "call" with plain RTP audio.

[jmordica]

I’m referring to how Janus gateway (when using sip plugin) works with legacy sip servers. So from browser to Janus gateway it uses normal webrtc (ICE/DTLS), and from Janus gateway to sip server just regular SDP.

[ibc]

JsSIP is just a client side SIP UA library. Said that, once we get this feature done, how the application sends and receives SDP is up to it.

[jmordica]

Ah right on. I think this belongs in oversip. I'll delete my comments :)

[mgoodenUK]

This feature looks really interesting - Is it still on your roadmap?

[ibc]

Yes, but no ETA for now.

[mgoodenUK]

We'd like to implement this ourselves with the goal of ultimately raising a PR - Do you have any pointers on the best place to start? Obviously we'd prefer to implement in a way that you guys would find acceptable.

[ibc]

I'm afraid we have not yet thought on how to do this so cannot provide any pointers. Coming to my mind (but must re-check in the future):

• RTCSession needs new methods to provide it with a SDP offer or re-offer.
• RTCSession needs to emit new events when a remote SDP offer or response has been received.
• When a remote SDP offer has been received, such an event must contain (into its arguments) a callback to provide the RTCSession with a SDP answer.

[jmillan]

Agreed with @ibc, we haven't thought of it yet.

IMHO a good starting point would be, following what @ibc commented:

• Create a MediaConnection specification like we do with Socket
  • Such spec would be something like:
  • receiveSdpOffer(sdp) // SDP offer received from the wire, returns a promise resolving a SDP answer.
  • receiveSdpAnswer(sdp) // SDP answer received from the wire, returns a promise resolving void.
  • getLocalSdp() // Retrieves the local SDP. Ie: used if we want it before all ice candidates are gathered.
  • addTracks([tracks]) // Adds the given audio/video tracks, returns a promise resolving a SDP offer.
  • removeTracks([tracks]) // Removes the given audio/video tracks, returns a promise resolving a SDP offer.
  • Events:
    • 'newtrack' // new remote track
    • 'icecandidate' // new ice candidate.
    • 'close'
• Following the above specification, create a default implementation in its own file which would use RTCPeerConnection, as done now in RTCSession. It could be named MediaConnectionInterface.
• Usage:
  • In UA.call() and RTCSession.answer() the user now must provide a MediaConnection instance (we can later use factories) which is assigned to the RTCSession and following the above specification will be managed by RTCSession regardless its implementation.

[Haardt]

This is a really great feature. Can I use jsSIP with Mediasoup to send an RTP stream to a SIP server (for example to mixing audio)?

[jmillan]

No you can't. Maybe this effort would be a starting point that would let you to it, but you would have to do a module that communicates with mediasoup, deals with SDP etc. In short no, this will not allow you do that.

[Haardt]

Yes, not out of the box - Maybe the wrong place, but we have developed a SFU based on Mediasoup and wanted to use jsSIP to send the RTP streams to another SIP server. We wanted to use the SDP data from Medasoup and then pass it to jsSIP. We need to support "phone ports" :).

[ibc]

mediasoup does not generate SDPs. However there is an ongoing mediasoup-sdp-bridge project. Let's please not use this issue to discuss this.

[Haardt]

I have been working on this ticket. I removed the "RTCPeerConnection" and the "...getUserMedia" from the RTCSession file. My current API for Browser/Node-MediaConnectionInterface:

• signalingState() (ice state?, getter)
• setup(pcConfig, rtcConstraints) - create the RTCPeerConnection or something else
• receiveSdpOffer(options)
• receiveSdpAnswer(options)
• registerIceUpdates(constraints, type, callback) - used in '_createLocalDescription'
• prepareStreams(constraints) - calls 'getUserMedia' and adds the tracks
• onIceConnectionEvent(callback) - inform about ice failures ('global')
• getSenders() - used for dtmf... bad name?
• close() - Close the RtcPeerConnection and you can stop your streams
• setRemoteDescription(sdp) - rename to setRemoteSdp?
• getLocalSdp()

I can use the 'tryit-jssip' application with the modifications. However, the 'RTCPeerConnection' is also used outside (as 'peerconnection') - maybe 'beaking changes'.

@jmillan I can't find the events? How to handle 'newtrack, close' in the RTCSession?

[jmillan]

How to handle 'newtrack, close' in the RTCSession?

In the MediaConnection light spec done here it is said that such events are triggered by the MediaConnection instance.

RTCSession.connection will now be a MediaConnection instance instead of a RTCPeerConnection one.

Could you please detail the question having that in mind?

[Haardt]

Exactly, I have replaced the 'connection'. Now the 'Interface / MediaConnection' is used everywhere. I have created a feature branch: https://github.com/Haardt/JsSIP/tree/feature/427-Decouple_signaling_and_WebRTC_stuff Maybe you can look into it - i'm on the right way?

[jmillan]

Sure! Let us few days and we'll take a look.

[jmillan]

I've made some comments @Haardt, it's starting to look nice! I've more comments to do but I prefer that you handle these comments and then we'll go for a second round.

[jmillan]

few points:

• Please keep the name _connection in RTCSession. It's way easier to see the real changes this way.
• Please keep the bracket style, new line before return, new line after the last const declaration in the block.
• We are not going to do getUserMedia anymore. Let the user provide the tracks or add them explicitly.
• UA does not need an instance of MediaConnection
• MediaConnection must be generic, and not mandate any WebRTC specific thing like getSenders.
• Lets provide a sendDTMFs() method and let the implementations do it their way.

[jmillan]

• No need for onIceConnectionEvent() or alike.
  • they are WebRTC specific.
  • the interface already defines [add|remove]EventListener. Let's provide a single way to do things.

[Haardt]

@jmillan i made some progress:

• [x] Please keep the name _connection
• [x] We are not going to do getUserMedia anymore
• [x] UA does not need an instance of MediaConnection
• [x] Lets provide a sendDTMFs()
• [ ] getSenders (Used in mute/unmute)
• [x] onIceConnectionEvent() removed
• [ ] Some problem with "this.rtcReady" flag (depends on Ice?)
• [ ] Please keep the bracket style (hope i get all)
• [x] Moved RTCSessionDescription to 'MediaConnection' Thank you for your comments! I need now the next suggestions :).

[jmillan]

Hi Haardt,

It's very difficult to comment on a branch if there is no PR. Could you make a PR from your new branch to a JsSIP master branch in your origin?

This way it'll be much more agile making comments and handling feedback.

[Haardt]

@jmillan i have create a draft pull request from my branch to my master.

[Haardt]

ping - some progress in the branch.

[jmillan]

We will comment on it.

[jmillan]

Another review done.

[Haardt]

I have implemented your comments. What do we do with 'getSender'? This is needed for mute.

I have also implemented two test projects:

• TrySip with the new version
• Mediasoup with FreeSwitch (Web->Mediasoup->jsSIP->FreeSwitch->SipPhone)

I think I can publish both projects at the end of next week.

[jmillan]

What do we do with 'getSender'? This is needed for mute?

I commented here:

https://github.com/Haardt/JsSIP/pull/1/files#r586408690

I think I can publish both projects at the end of next week.

We need to review it further. Please make sure you have implemented all the comments I made.

[jmillan]

Thanks @Haardt,

We'll have another review as soon as possible.

[jmillan]

Another review done.

Please:

• Go through all my comments.
• We need to keep accepting MediaStream in RTCSession.answer() and UA.call(), it must be restored.

It's looking good @Haardt :+1:

[Haardt]

Hi, next round :)? I have been working on you comments, but why keep the MediaStream in answer/call? The only thing that happens with the 'stream' is that it is added to the connection.

     stream.getTracks().forEach((track) =>
      {
        this._connection.addTrack(track, stream);
      });

[jmillan]

next round :)? I have been working on you comments, but why keep the MediaStream in answer/call? The only thing that happens with the 'stream' is that it is added to the connection.

Sounds legit. Let us comment on next review.

[jmillan]

New review done.

[jmillan]

Hi @Haardt,

Is there anything you may need from our side?

[Haardt]

@jmillan Hi, An OKR needs my attention at the end of Q1 :). Next week I take my time!

[nilsbandel]

Hi!

What would this mean for apps currently using JsSIP? Does it mean that JsSIP will no longer handle the WebRTC stuff, or that it still can but it's also possible to decouple if you only want to use the signalling part of JsSIP?

[ibc]

It means that JsSIP will no longer manage any RTCPeerConnection or getUserMedia() and the application should do that and pass and receive SDPs from JsSIP, which will just manage the SIP message exchange, SIP session status, SIP registration and things like that.

[nilsbandel]

I can see the use case described above where you'd want to decouple things, but for all current users of the library, wouldn't that mean quite a large refactor then?

From the PR in https://github.com/Haardt/JsSIP/pull/1/files#r586408690 it seems like there will now be a MediaConnection interface and an implementation of that in RTCPeerMediaConnection that you could use if you want to use WebRTC, is that correct? Is there going to be a transition guide of some sort to guide users in the breaking API changes?

[jmillan]

There will be a default MediaConnection implementation handling basic RTCPeerConnection as it's done now. The user will have to provide an instance of such built-in class if not implementing its own. The very same way we do with the Socket interface in UA.

https://jssip.net/documentation/3.7.x/api/ua_configuration_parameters/

[Haardt]

@jmillan Sorry...more delay. I can work next week on it.

[jmillan]

Sure @Haardt :+1:, let us know if you need some help.

[Haardt]

Some small progress pushed.

[Haardt]

I added an integration test to the 'test-mediaConnect.js' file.

[jmillan]

Thanks,

Seems that the the PR does not exist anymore, which makes it very difficult to review & comment.

Once you feel the branch is ready, after having reviewed my previous comments, can you please create a PR against master branch? You should probably merge master into your branch to make it mergeable.

[jmillan]

I see the PR is in your account, I'm going to comment there.

[jmillan]

@Haardt,

Nice work!. I've commented in the PR. I've created a delimiter (------- 29th April delimiter ------) and made a review after that.

Please, make sure you handle all the comments. Hopefully the next will be the last review.

Congrats for the nice work!

[Haardt]

Hi, another round is done! I guess there will be more - maybe :).

[jmillan]

Hi Andreas,

We will revisit this with time. We are having some internal thoughts that may affect this. Let us please some time to come back to this.

Thanks a lot.