New Janus plugin, NoSIP, for legacy interop without touching signalling #799

lminiero · 2017-02-28T17:29:28Z

This is a plugin that has been cooking (at least in mind) for a while. Specifically, people interested in interoperability with the SIP world now have an alternative to the SIP plugin we already provided. More importantly, it's good ol' @saghul's BoringSDP idea brought to life (so kudos to him for suggesting the feature in the first place, at the time), but with a different name 😄

As the name suggests, this new plugin, called NoSIP, doesn't care about signalling at all, and only takes care of bridging the media between two peers as the SIP plugin already did. This means that signalling is entirely up to you: this plugin only deals with taking care of the media level interoperability for you. Of course, you still need to communicate with Janus and this plugin to establish media connectivity, but how you transport the SDPs around between peers is up to you: SIP, IAX, XMPP, pigeon, etc., we don't care.

Copying from the plugin doc, the typical flow is the following:

a WebRTC application handles signalling on its own (e.g., SIP), but needs to interact with a peer that doesn't support WebRTC (DTLS/ICE);
it creates a handle with the NoSIP plugin, creates a JSEP SDP offer, and passes it to the plugin;
the plugin creates a barebone SDP that can be used to communicate with the legacy peer, binds to the ports for RTP/RTCP, and sends this plain SDP back to the application;
the application uses this barebone SDP in its signalling, and expects an answer from the peer;
the SDP answer from the peer will be barebone as well, and so unfit for WebRTC usage; as such, the application passes it to the plugin as the answer to match the offer created before;
the plugin matches the answer to the offer, and starts exchanging RTP/RTCP with the legacy peer: media coming from the peer is relayed via WebRTC to the application, and WebRTC stuff coming from the application is relayed via plain RTP/RTCP to the legacy peer.

The same kind of flow also applies if the WebRTC peer using the NoSIP plugin is the callee: in that case, you first turn the barebone SDP from the peer in a WebRTC offer from Janus, and then you turn your WebRTC answer into something the legacy peer can digest.

I've tested this briefly in a newly crafted demo, and it seems to be doing its job, at least for the very basic call setup. The nosiptest.html demo doesn't involve any signalling, but simply creates two handles attached to the plugin, one to be the caller and another the callee: then signalling is "simulated", so that caller and callee get to exchange media through the plain RTP/RTCP relay implemented by the plugin. The final effect is obviously talking to yourself, but the principle is that if you see something, then it's working.

It's of course still not perfect and can be improved, but it's an important stepping stone. If this is something you care about, please test this and provide feedback.

saghul · 2017-03-01T07:37:47Z

plugins/janus_nosip.c

+#define SRTP_MASTER_SALT_LENGTH	14
+#define SRTP_MASTER_LENGTH (SRTP_MASTER_KEY_LENGTH + SRTP_MASTER_SALT_LENGTH)
+static const char *janus_nosip_srtp_error[] =
+{


this is also present in the SIP plugin, right? Maybe some srtputils.c module is in order?

Yeah, that may be a good idea! I'll do that as part of this PR.

Did something in that spirit in #804

saghul · 2017-03-01T07:39:29Z

plugins/janus_nosip.c

+static void *janus_nosip_relay_thread(void *data);
+
+
+/* Random string helper (in case user doesn't provide opaque info) */


Is there a reason why this is exposed? Shouldn't Janus always generate some random stuff?

There is an info object users can provide in case they need some opaque identifier, but I'm questioning whether it's needed at all, considering you'll never bridge two sessions using this plugin directly... just in case it's needed, and the user doesn't provide any, then the random string kicks in.

saghul · 2017-03-01T07:40:10Z

plugins/janus_nosip.c

+}
+
+
+static void janus_nosip_detect_local_ip(char *buf, size_t buflen) {


IIRC this is also in some other parts of the code... how about moving it out so any plugin can use it?

Actually we have that already, we recently merged a contribution that provides a whole new set of IP- and interface-related utilities. I've been planning to replace all the parts where we do the same thing with those utils, will do that soon and here too.

Did that in #803, when it's merged I'll integrate in here too

saghul · 2017-03-01T07:42:52Z

plugins/janus_nosip.c

+				goto error;
+			}
+			gboolean offer = !strcasecmp(msg_sdp_type, "offer");
+			if(strstr(msg_sdp, "m=application")) {


janus could filter those streams out and generate the answer rejecting the datachannel

In case of an answer you're right, while I guess we'll still need tor reject it for offers. Will do that.

saghul · 2017-03-01T07:48:15Z

Hey Lorenzo, this is great to see! ❤️

There is one case I'm not sure this covers (just had a quick look): getting the first offer as a "boring" SDP. My impression was that this relies on getting the first offer as a WebRTC SDP, is that the case? If so, please consider the opposite case, so incoming calls from legacy devices can work. Maybe some sdp_type in the JSON would help, which could be one of webrtc or legacy.

Again, thanks for doing this, I'm sure many people will benefit from it. Hint: someone could write an OpenSIPS / Kamailio module which uses Janus to "modernize" the SDP when sending an INVITE to a device known to be a WebRTC endpoint... and the other way around :-)

lminiero · 2017-03-01T07:56:32Z

Thanks for the review!

We do handle the "boring" SDP offer, actually. The demo we presented tries to show both cases, in fact, as it creates two different handles, a caller and a callee. For the callee, you'll get a "process(boring offer)" as the first operation, which will trigger the WebRTC offer you can use for the setRemoteDescription. It's probably poorly documented or explained right now, I'll have to improve that. I'll add some examples of the API in action to the PR text so that it's clearer.

It would be cool to have that OpenSIPS/Kamailio integration, I'll keep that in mind! I'll also have to try implementing a test page using JsSIP to showcase the signalling layer as well, although I'm not sure JsSIP allows you to provide your own SDP instead of handling JSEP itself: maybe I should ask @jmillan or @ibc 😄

notedit · 2017-03-01T08:37:39Z

cool , thank you.

i want to see same idea on videoroom. any plan?

lminiero · 2017-03-01T09:12:49Z

What do you mean same idea in the VideoRoom? This PR is for 1-1 calls with legacy interop, not multiparty communication.

notedit · 2017-03-01T09:29:59Z

VideoRoom: the application handle the signaling, janus just handle the media layer.

lminiero · 2017-03-01T09:33:36Z

You can already do that wrapping the Janus API and exposing your own API to users, we do that all the time. Here the requirement was different, as the SIP plugin uses SIP internally and we wanted to provide an alternative that didn't.

smachlin · 2017-03-01T08:52:41Z

plugins/janus_nosip.c

+			if (!janus_is_ip_valid(item->value, &family)) {
+				JANUS_LOG(LOG_WARN, "Invalid local IP specified: %s, guessing the default...\n", item->value);
+			} else {
+				/* Verify that we can actually bind to that address */


this code looks like a general purpose code, it can probably be refactored out into a network utils library.

As I told Saul, this has already happened... #776 by @cmacq2 adds this and more. It's just that I'm lazy and haven't updated the code we have around that does similar things to use that instead 😉

Did that in #803, when it's merged I'll integrate in here too

smachlin · 2017-03-01T08:55:19Z

plugins/janus_nosip.c

+			}
+		}
+		if (!local_ip_set)
+			janus_nosip_detect_local_ip(local_ip, sizeof(local_ip));


in some places you surround single line ifs with {} and in some places you don't, better to be consistent (in my opinion it's better to surround even single lines)

Ack and agree! I always try to do that but there may be places where I still forget to do that.

smachlin · 2017-03-01T08:58:02Z

plugins/janus_nosip.c

+			}
+		}
+		janus_mutex_unlock(&sessions_mutex);
+		g_usleep(500000);


probably better to use cond wait/signal instead of sleep.

That's the way all plugins behave right now, but it has already been dealt with in the referece counters branch, #403, where we don't have that anymore.

smachlin · 2017-03-01T09:06:43Z

plugins/janus_nosip.c

+			return;
+		}
+		/* Forward to our NoSIP peer */
+		if(video) {


the audio and video branches are very similar, maybe there's a way to refactor into a helper method?

Good point, which probably applies to other plugins as well. I'll keep that in mind.

smachlin · 2017-03-01T09:33:52Z

plugins/janus_nosip.c

+		const char *request_text = json_string_value(request);
+		json_t *result = NULL, *localjsep = NULL;
+
+		if(!strcasecmp(request_text, "generate") || !strcasecmp(request_text, "process")) {


consider splinting this into separate "handle" functions - handle_generate, handle_recording, etc.
maybe even go deeper and split those as well - handle_recording_start, handle_recording_stop, handle_generate_answer, etc.

Not sure how that would help? Only semantics?

it'll make the code much more readable. personally, i find it difficult to follow a 500 line function or/and 5 level of nesting, especially one that handles several completely different aspects of the BL. i'm guessing i'm not the only one :)

Got it. In this case, my main point was trying to keep things compact and re-use as much code as possible, but I guess I could at least split the generate and process part in two branches, as that's what we do in almost all plugins anyway (it's rare we group-handle different methods).

smachlin · 2017-03-01T09:44:09Z

plugins/janus_nosip.c

+								if(video)
+									session->media.video_srtp_suite_in = suite;
+								else
+									sessi


will creating a thread for each session scale?
lets say you want to have 10000 audio calls on a server? will 10000 threads work?
i'm guessing the the context switches will be a problem.

That's how the SIP plugin already worked, and I just migrated (as in copied/pasted) it in here as well. I guess we might indeed have a single thread that polls on all the file descriptors for all sessions and handles all incoming traffic accordingly, but that would be a non trivial refactoring as it is. Besides, there's code (still unused) to handle session updates that could keep the thread busy, and I'm afraid this would impact other sessions.

Anyway, your point on threads in general is valid, and applies to the Janus core as well.

smachlin · 2017-03-01T09:52:56Z

why not taking this one step farther and eliminating SDP for signaling altogether? just provide and API to set ip/port and SRTP credentials.

lminiero · 2017-03-01T10:04:17Z

why not taking this one step farther and eliminating SDP for signaling altogether? just provide and API to set ip/port and SRTP credentials.

But that would require the application to then craft its own SDP if that's what it needs. I believe it's easier to just do it that way: if one only needs SRTP credentials and IP/ports it can extract them from the SDP, and preparing an SDP from raw info (e.g., from JavaScript) to pass to the plugin would be easy enough. Besides, it's not just IP/ports or SRTP credentials you have to worry about: there's codecs, payload types, formats, and a whole bunch of stuff you'd need to serialize someway... I really don't want to get into that rat hole 😉

Thanks for the review!

ibc · 2017-03-01T11:09:19Z

I'll also have to try implementing a test page using JsSIP to showcase the signalling layer as well, although I'm not sure JsSIP allows you to provide your own SDP instead of handling JSEP itself

Not sure if I understand the doubt. I mean, JsSIP generates its SDP offers/answers by calling the PeerConnection API, and JsSIP does accept remote SDPs as usual and then calls setRemoteDescription() and so on. It can also generate and consume re-offers at any time. Of course, JsSIP is governed by the SIP rules.

Said that... which is the question? :)

lminiero · 2017-03-01T11:12:27Z

@ibc I wanted to understand whether you can use JsSIP as a SIP stack only, and not let it handle JSEP/media as well. Normally, as you pointed out, JsSIP would generate its own SDPs, but with this plugin I'd like to check if I can provide, let's say, my own SDP (the barebone SDP stuff), use JsSIP as a stack to contact a WebSockets SIP server, and handle media negotiation with Janus as a consequence.

ibc · 2017-03-01T11:19:45Z

Ahhh ok ok, so you mean using JsSIP as a Node.js app in server side for pure SIP purposes, right?

lminiero · 2017-03-01T11:23:12Z

Sure, that too, but even in the browser if you want: think wanting to call (or be called by) somebody you know doesn't support WebRTC from the browser, you'll want an SDP they can digest, and something in the middle to fix the media gap. That's why I thought of JsSIP as a way to implement a simple proof of concept that involves signalling as well, instead of the ugly self-contained demo I have now 😄

ibc · 2017-03-01T11:33:01Z

May you please draw/describe a single flow/architecture with all the pieces (JsSIP web app, Janus, JsSIP-in-server, and legacy SIP phone) so I can completely understand the picture?

What I don't understand is: regarding JsSIP in client side (WebRTC web application), should it generate/consume non WebRTC valid SDPs?

lminiero · 2017-03-01T11:37:11Z

May you please draw/describe a single flow/architecture with all the pieces (JsSIP web app, Janus, JsSIP-in-server, and legacy SIP phone) so I can completely understand the picture?

Yeah, I plan to sketch this a little better, as that's indeed lacking in the PR at the moment. It is indeed a bit confusing to grasp!

What I don't understand is: regarding JsSIP in client side (WebRTC web application), should it generate/consume non WebRTC valid SDPs?

The WebRTC application would keep on generating/consuming WebRTC SDPs: what changes here is that you rely on the help of the NoSIP plugin to get a stripped down SDP you use in your SIP/SDP offer/answer instead. When you get an SIP/SDP offer/answer from a legacy peer, you pass it to the plugin, and it will turn it into a WebRTC SDP you can consume. The end result is that you exchange WebRTC DTLS/ICE/SRTP with Janus, and Janus (via the NoSIP plugin) exchanges media with the legacy peer using plain RTP (or SDES-SRTP). Basically a media-only gateway.

ibc · 2017-03-01T11:44:50Z

When you get an SIP/SDP offer/answer from a legacy peer, you pass it to the plugin, and it will turn it into a WebRTC SDP you can consume.

I just need to know who you is in the above sentence :)
Is it the web app running JsSIP? or something in server side?

you pass it to the plugin

Probably I should read much more in order to understand how such a communication (between you and the plugin) is. Does the plugin provides a REST/HTTP API so web clients can post a WebRTC/legacy SDP and receive a legacy/WebRTC SDP? Am I getting crazy?

lminiero · 2017-03-01T11:57:40Z

Yes, it IS confusing! 😄
In this case, by "you" I mentioned the JsSIP user, whether it's a nodejs application or a browser tab. Yes, the idea is some kind of REST API like that (can be websockets, for instance), the Janus API takes care of that.

Let me try to clarify this a bit: let's assume Alice uses JsSIP in her browser, and Bob uses an old SIP softphone that only talks RTP. If Alice wants to call Bob (and somehow she knows Bob still lives in 2005), something like this can happen:

Alice uses getUserMedia to capture her devices;
Alice calls createOffer to get a WebRTC SDP offer;
Alice calls setLocalDescription with her own SDP;
Alice connects to the NoSIP plugin, and passes her SDP along to have the plugin generate a "boring" SDP; her SDP also acts as her WebRTC PeerConnection offer to communicate with Janus;
the NoSIP plugin parses the SDP, strips the WebRTC stuff (well, the Janus core does that, but that's the same), allocates the ports it needs to do RTP/RTCP, optionally prepares old SDES-SRTP stuff, and crafts an SDP that will make NoSIP the media peer of the communication;
the NoSIP plugin sends this "boring" SDP back to Alice;
Alice sends an INVITE to Bob, using this "boring" SDP in the offer instead of the one she got from createOffer;
Bob phone rings, and he answers; his old phone receives an SDP it can process, so an old fashioned SDP answer is generated and sent back in a 200 OK;
Alice receives this old SDP in the 200 OK, and it's not something the browser can use, so she passes it to the NoSIP plugin to get a WebRTC one;
the NoSIP plugin receives this old answer, and parses it to establish the communication channel with Bob (RTP/RTCP will be exchanged between the two on behalf of Alice);
then, the NoSIP plugin generates an answer towards Alice as a Janus user, which means Alice ends up receiving a WebRTC SDP answer from Janus to complete the setup of the PeerConnection between them;
Alice now has a WebRTC answer, which she uses to do a setRemoteDescription;
Alice and Bob talk to each other: Alice sends SRTP via WebRTC, Janus passes it to the NoSIP plugin, the NoSIP plugin relays it to Bob via RTP; Bob sends plain RTP to the NoSIP plugin, the NoSIP plugin relays it to Alice via Janus, Janus sends SRTP via WebRTC to Alice.

Hope this is clearer! For Alice as a callee the approach would be pretty much the same: she'd get an old SDP, which she can turn in a WebRTC offer from Janus via the NoSIP plugin; her WebRTC answer (which Janus processes to create a PeerConnection with Alice) is turned into an old SDP Alice can send in her 200 OK to Bob.

PS: edited to add a 12. point and make this even clearer.

ibc · 2017-03-01T12:17:47Z

Hyper clear now!

OK, so here is the problem: Currently JsSIP emits some events before sending an INVITE (the app can mangle the SDP in there) but such an event must be synchronous, this is, once the JS listener/callbacks returns the INVITE is sent, so there is no a chance for asynchronous stuff (i.e. making a REST API to NoSIP and getting the boring SDP offer). Same happens when receiving an INVITE or a SIP 200 response.

Indeed, the way to go must be removing the WebRTC/PeerConnection dependency from JsSIP and make it a pure SIP signaling library.

So, planning it: versatica/JsSIP#427 :)

lminiero · 2017-03-01T13:02:01Z

Nice, thanks! I'll definitely follow this to play with it when something's available 👍

…eduncancy

… stuff)

…he plugin

lminiero · 2017-10-23T17:06:22Z

This has been around long enough, and I don't plan to add new features in the short term, so we might just as well merge it in case it's of use to anybody as it is. We can always improve it along the way.

lminiero added 3 commits February 28, 2017 18:18

New Janus plugin, NoSIP, for legacy interop without touching signalling

04c5c9e

Several new helper methods for SDP utilities

b310383

New Janus plugin, NoSIP, for legacy interop without touching signalling

84c9371

lminiero changed the base branch from master to sdputils-pt2 February 28, 2017 17:47

saghul reviewed Mar 1, 2017

View reviewed changes

smachlin reviewed Mar 1, 2017

View reviewed changes

ibc mentioned this pull request Mar 1, 2017

Decouple signaling and WebRTC stuff versatica/JsSIP#427

Open

lminiero mentioned this pull request Mar 2, 2017

Moved most of SRTP-related stuff to rtp.h/.c (cleans dtls and janus_sip) #804

Merged

lminiero added 4 commits March 2, 2017 13:12

Removed info string, cleaned up a bit, and refactored code to limit r…

d072629

…eduncancy

Merge branch 'nosip' of github.com:meetecho/janus-gateway into nosip

4550d89

Merge branch 'sdputils-pt2' into nosip

785564c

Modified NoSIP plugin code to use #796 and #804 (RTP context and SRTP…

576f6db

… stuff)

lminiero added 27 commits June 8, 2017 12:43

Merge branch 'master' into nosip

91aa740

Merge branch 'master' into nosip

6797864

Merge branch 'master' into nosip

42c2db1

Merge branch 'master' into nosip

fcea801

Merge branch 'master' into nosip

e9ba7d7

Merge branch 'master' into nosip

bd71be7

Better management of POLLERR errors in NoSIP plugin

636790a

Merge branch 'master' into nosip

bdc3ad0

Merge branch 'master' into nosip

a0b05c0

Added some checks/fixes already made in master

8a32bb7

Merge branch 'master' into nosip

47eb485

Merge branch 'master' into nosip

ed622c2

Merge branch 'master' into nosip

3b162a2

Merge branch 'master' into nosip

d318caf

Merge branch 'master' into nosip

84df3a6

Aligned NoSIP branch to latest changes

def7be2

Merge branch 'master' into nosip

00c852d

Merge branch 'master' into nosip

81a9707

Merge branch 'master' into nosip

079cc48

Merge branch 'master' into nosip

932dd62

Updated NoSIP plugin to use new rtpsrtp.h header

6e15362

Merge branch 'master' into nosip

b6b2aa3

Merge branch 'master' into nosip

e836799

Added configurable port range for RTP/RTCP ports in the NoSIP plugin

21737e7

Moved NoSIP links in the demos to highlight the different nature of t…

b8205d9

…he plugin

Fixed broken enforcement of new RTP/RTCP range in NoSIP plugin

3b77505

Merge branch 'master' into nosip

cdb96cc

lminiero merged commit 5cbe556 into master Oct 23, 2017

lminiero deleted the nosip branch October 23, 2017 17:06

		static void janus_nosip_relay_thread(void data);


		/* Random string helper (in case user doesn't provide opaque info) */

		}


		static void janus_nosip_detect_local_ip(char *buf, size_t buflen) {

New Janus plugin, NoSIP, for legacy interop without touching signalling #799

New Janus plugin, NoSIP, for legacy interop without touching signalling #799

Conversation

lminiero commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Mar 1, 2017

lminiero commented Mar 1, 2017

notedit commented Mar 1, 2017

lminiero commented Mar 1, 2017

notedit commented Mar 1, 2017

lminiero commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smachlin commented Mar 1, 2017

lminiero commented Mar 1, 2017

ibc commented Mar 1, 2017

lminiero commented Mar 1, 2017

ibc commented Mar 1, 2017

lminiero commented Mar 1, 2017

ibc commented Mar 1, 2017

lminiero commented Mar 1, 2017 • edited Loading

ibc commented Mar 1, 2017 • edited Loading

lminiero commented Mar 1, 2017 • edited Loading

ibc commented Mar 1, 2017

lminiero commented Mar 1, 2017

lminiero commented Oct 23, 2017

lminiero commented Mar 1, 2017 •

edited

Loading

ibc commented Mar 1, 2017 •

edited

Loading

lminiero commented Mar 1, 2017 •

edited

Loading