Decouple signaling and WebRTC stuff #427

ibc · 2017-03-01T12:16:00Z

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.

Decouple signaling and WebRTC stuff versatica#427 rpid parser add async modifier for local and remote description add option to use reinvite instead update for session timer

michelepra · 2017-10-20T11:00:33Z

what you think about my solution?

jmordica · 2018-01-31T03:47:59Z

Is this to support interop with legacy sip servers?

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

Thanks!

ibc · 2018-01-31T14:01:30Z

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

jmordica · 2018-01-31T14:29:05Z

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 · 2018-01-31T15:22:17Z

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 · 2018-01-31T16:41:42Z

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

mgoodenUK · 2020-12-23T08:08:49Z

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

ibc · 2020-12-23T08:09:51Z

Yes, but no ETA for now.

mgoodenUK · 2021-01-04T17:10:29Z

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 · 2021-01-04T17:43:38Z

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 · 2021-01-07T13:42:37Z

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 · 2021-02-01T15:13:11Z

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 · 2021-02-02T06:58:31Z

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 · 2021-02-02T18:25:55Z

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 · 2021-02-02T18:28:51Z

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 · 2021-02-10T15:42:10Z

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 · 2021-02-10T16:44:47Z

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 · 2021-02-10T17:01:42Z

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 · 2021-02-10T17:31:28Z

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

jmillan · 2021-02-11T15:38:11Z

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 · 2021-02-11T15:41:56Z

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 · 2021-02-23T09:54:44Z

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 · 2021-02-23T10:17:25Z

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

Haardt · 2021-03-02T16:46:48Z

ping - some progress in the branch.

jmillan · 2021-03-02T17:00:29Z

We will comment on it.

jmillan · 2021-03-03T13:29:10Z

Another review done.

Haardt · 2021-03-03T16:06:13Z

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 · 2021-03-03T17:06:01Z

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 · 2021-03-03T17:11:00Z

Thanks @Haardt,

We'll have another review as soon as possible.

jmillan · 2021-03-04T10:00:47Z

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 👍

Haardt · 2021-03-04T12:31:44Z

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 · 2021-03-04T13:41:09Z

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 · 2021-03-05T11:49:03Z

New review done.

jmillan · 2021-03-17T13:42:17Z

Hi @Haardt,

Is there anything you may need from our side?

Haardt · 2021-03-18T08:16:19Z

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

nilsbandel · 2021-03-29T15:34:42Z

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 · 2021-03-29T17:24:48Z

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 · 2021-03-30T06:40:57Z

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 · 2021-03-30T07:41:29Z

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 · 2021-04-12T08:38:01Z

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

jmillan · 2021-04-14T07:19:08Z

Sure @Haardt 👍, let us know if you need some help.

Haardt · 2021-04-26T12:39:52Z

Some small progress pushed.

Haardt · 2021-04-27T11:31:26Z

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

jmillan · 2021-04-27T16:56:18Z

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 · 2021-04-27T17:01:33Z

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

jmillan · 2021-04-29T07:32:06Z

@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 · 2021-06-09T07:09:28Z

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

jmillan · 2021-06-10T11:42:32Z

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.

brnt · 2022-01-01T03:53:42Z

Any new thoughts here? I agree with the philosophy that the SIP signaling and media should be entirely independent. A more blunt way of putting it: Not every INVITE is for controlling audio/video.

Just enthusiastically piling on in support of this work!

ford-prefect · 2022-12-14T18:25:09Z

👋🏾 just checking in to see if this work is still active?

ibc added API enhancement labels Mar 1, 2017

ibc added this to the 4.0.X milestone Mar 1, 2017

ibc mentioned this issue Mar 1, 2017

New Janus plugin, NoSIP, for legacy interop without touching signalling meetecho/janus-gateway#799

Merged

This was referenced Apr 14, 2017

gatheringTimeout Missing v3.x #432

Closed

Add event to know when RTCPeerConnection has been created #421

Closed

michelepra added a commit to michelepra/JsSIP that referenced this issue Oct 20, 2017

enhanced jssip

dcaa09f

Decouple signaling and WebRTC stuff versatica#427 rpid parser add async modifier for local and remote description add option to use reinvite instead update for session timer

michelepra mentioned this issue Oct 31, 2017

choose UPDATE/RE-INVITE on sending session refresh request #384

Closed

versatica deleted a comment from nceliy Dec 27, 2018

hieyou1 mentioned this issue Jul 4, 2021

Possibility of WebRTC in Node with a new parameter passed to UA? #723

Closed

Decouple signaling and WebRTC stuff #427

Decouple signaling and WebRTC stuff #427

Comments

ibc commented Mar 1, 2017

michelepra commented Oct 20, 2017

jmordica commented Jan 31, 2018

ibc commented Jan 31, 2018

jmordica commented Jan 31, 2018

ibc commented Jan 31, 2018

jmordica commented Jan 31, 2018

mgoodenUK commented Dec 23, 2020

ibc commented Dec 23, 2020

mgoodenUK commented Jan 4, 2021

ibc commented Jan 4, 2021

jmillan commented Jan 7, 2021

Haardt commented Feb 1, 2021 • edited Loading

jmillan commented Feb 2, 2021

Haardt commented Feb 2, 2021

ibc commented Feb 2, 2021

Haardt commented Feb 10, 2021

jmillan commented Feb 10, 2021 • edited Loading

Haardt commented Feb 10, 2021

jmillan commented Feb 10, 2021

jmillan commented Feb 11, 2021

jmillan commented Feb 11, 2021

jmillan commented Feb 23, 2021

Haardt commented Feb 23, 2021

Haardt commented Mar 2, 2021

jmillan commented Mar 2, 2021

jmillan commented Mar 3, 2021

Haardt commented Mar 3, 2021 • edited Loading

jmillan commented Mar 3, 2021 • edited Loading

jmillan commented Mar 3, 2021

jmillan commented Mar 4, 2021

Haardt commented Mar 4, 2021 • edited Loading

jmillan commented Mar 4, 2021

jmillan commented Mar 5, 2021

jmillan commented Mar 17, 2021

Haardt commented Mar 18, 2021

nilsbandel commented Mar 29, 2021

ibc commented Mar 29, 2021

nilsbandel commented Mar 30, 2021

jmillan commented Mar 30, 2021

Haardt commented Apr 12, 2021

jmillan commented Apr 14, 2021

Haardt commented Apr 26, 2021

Haardt commented Apr 27, 2021

jmillan commented Apr 27, 2021

jmillan commented Apr 27, 2021

jmillan commented Apr 29, 2021

Haardt commented Jun 9, 2021

jmillan commented Jun 10, 2021

brnt commented Jan 1, 2022

ford-prefect commented Dec 14, 2022

Haardt commented Feb 1, 2021 •

edited

Loading

jmillan commented Feb 10, 2021 •

edited

Loading

Haardt commented Mar 3, 2021 •

edited

Loading

jmillan commented Mar 3, 2021 •

edited

Loading

Haardt commented Mar 4, 2021 •

edited

Loading