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

Renegotiation support #1099

Merged
merged 31 commits into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@lminiero
Member

lminiero commented Dec 5, 2017

This patch tries to add renegotiation support to Janus and its plugins for existing media streams. It does also supports ICE restarts, which means that it obsoletes #753, which I'll close as soon as this is published.

For what concerns generic renegotiations and how to force a restart, you can check the examples in the above mentioned PR. Most importantly, though, this PR now allows you to renegotiate a session, e.g., as a consequence of a changed media device (think about changing a camera on the fly on the same peerconnection). The SSRC change is handled automatically, and the RTP headers are also updated on the fly, which means media-wise these changes should be completely transparent to plugins (and as such recorders).

Here's an example of how you can test this with the EchoTest demo:

echotest.webrtcStuff.myStream.removeTrack(echotest.webrtcStuff.myStream.getVideoTracks()[0]);
echotest.webrtcStuff.pc.removeTrack(echotest.webrtcStuff.pc.getSenders()[1])
navigator.mediaDevices.getUserMedia({audio:false, video:true}).then(function(stream) {
	echotest.webrtcStuff.pc.addTrack(stream.getVideoTracks()[0], stream);
	echotest.webrtcStuff.myStream.addTrack(stream.getVideoTracks()[0]);
	echotest.createOffer({
		media: { data: true },
		success: function(jsep) {
			echotest.send({message: {audio: true, video: true}, jsep: jsep});
		}
	});
});

This snippet basically removes the video track from both MediaStream and PeerConnection sender, does a new getUserMedia just for video, and then adds the new resulting video track to the two objects above. This requires a renegotiation to work, which is why we have a new createOffer call there. This createOffer will generate a new SDP with new SSRCs for video: as soon as the new answer from Janus comes back, the new camera will be used for video. Notice that, if you were using simulcast: true, you can use it for the new createOffer as well and it should work.

This is a snippet to test the same for a VideoRoom publisher:

sfutest.webrtcStuff.myStream.removeTrack(sfutest.webrtcStuff.myStream.getVideoTracks()[0]);
sfutest.webrtcStuff.pc.removeTrack(sfutest.webrtcStuff.pc.getSenders()[1])
navigator.mediaDevices.getUserMedia({audio:false, video:true}).then(function(stream) {
	sfutest.webrtcStuff.pc.addTrack(stream.getVideoTracks()[0], stream);
	sfutest.webrtcStuff.myStream.addTrack(stream.getVideoTracks()[0]);
	sfutest.createOffer({
		media: { audioRecv: false, audioSend: true, videoRecv: false, videoSend: true },
		success: function(jsep) {
			sfutest.send({message: {request: "configure", refresh: true}, jsep: jsep});
		}
	});
});

which does basically the same thing but in a VideoRoom. If you have subscribers, you should see viewers display the new camera (or the same one, if you replaced the video track with the same source) even after the renegotiation. A freeze there means something's not working.

As a further note, you can use these renegotiations to update media directions too, even though I don't think any of the plugins honor this at the moment, and neither does the core. We'll have to implement and fix this along the way: please provide feedback while using the features, and let us know about what's not working as expected when using this new feature.

Finally, you cannot currently use this to add new media tracks: let's say you started audio only, and want to renegotiate to add a video track, you currently can't do it. Again, this will need to be implemented later on, as soon as we know what we have now works reasonably well.

Feedback welcome!

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 5, 2017

Started playing with adding a new media stream in an existing PeerConnection: of course this means adding one if it wasn't present already, you're still bound to the "max 1 audio, 1 video, 1 data per PC" constraint.

Modified the EchoTest demo to start audio only:

media: { video: false, data: true },

and then tried this snippet:

navigator.mediaDevices.getUserMedia({audio:false, video:true}).then(function(stream) {
	echotest.webrtcStuff.pc.addTrack(stream.getVideoTracks()[0], stream);
	echotest.webrtcStuff.myStream.addTrack(stream.getVideoTracks()[0]);
	Janus.attachMediaStream($('#myvideo').get(0), echotest.webrtcStuff.myStream);
	$('.no-video-container').remove();
	$('#myvideo').removeClass('hide').show();
	echotest.createOffer({
		media: { data: true },
		success: function(jsep) {
			echotest.send({message: {audio: true, video: true}, jsep: jsep});
		}
	});
});

which is basically the same as the snippet presented before, but which also updates the local stream (which unlike the remote stream, is not automatically updated by janus.js). With the latest changes to the branch, this works for me, which is great news! Unfortunately, it also seem to result in desynced video, but that can be fixed along the way...

Edit: the desync was caused by a typo (damn copy/paste), works great now! Of course, this is something that plugins will need to provide support for: while for the EchoTest this is transparent, other plugins involving multiple users will need to be smarter than that. In the VideoRoom, for instance, a change in the available media for a publisher means sending a new offer to all its subscribers as well, or they won't be aware of the new media stream. I'll take note of this for the next patches.

lminiero added some commits Dec 5, 2017

Several negotiation related fixesordering of BUNDLE items, stream IDs…
… when ordering of m-lines is different, SRTP setup when audio and video only appears later on
@lminiero

This comment has been minimized.

Member

lminiero commented Dec 6, 2017

Played some more with a different scenario, today: starting from a datachannel only PeerConnection, and then adding audio and video in a new renegotiation later on. This brought up a ton of errors and assumptions we had in the code, mostly related to the ordering we assumed for m-lines (audio, video, data) that were of course invalidated with this renegotiation: as a result, nothing worked. I had to modify many parts of the core to get this working as expected, but I believe this now makes it much more flexible and resilient.

If you want to test this, start from datachannels only in the EchoTest demo:

media: { audio: false, video: false, data: true },

and then try this snippet to renegotiate with audio and video too:

navigator.mediaDevices.getUserMedia({audio:true, video:true}).then(function(stream) {
	echotest.webrtcStuff.pc.addTrack(stream.getAudioTracks()[0], stream);
	echotest.webrtcStuff.pc.addTrack(stream.getVideoTracks()[0], stream);
	echotest.webrtcStuff.myStream = stream;
	echotest.onlocalstream(stream);
	$("#videoleft").parent().unblock();
	echotest.createOffer({
		media: { data: true },
		success: function(jsep) {
			echotest.send({message: {audio: true, video: true}, jsep: jsep});
		}
	});
});

I'll have to work on the other way around, that is starting from audio and/or video, and only add datachannels in a later moment, since at the moment that won't work: in fact, the only place where we set up the SCTP association is in the "DTLS handshake done" code, which would not happen again after a renegotiation. This means we'll have to expose that as a separate function, to allow us to do that when needed, whether it's right after the DTLS handshake or later on.

Again, all these experiments have been only tested with the EchoTest so far, as I'm only interested in getting this to work from a functional perspective in the core. Any plugin-specific management of those scenarios will have to wait.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 12, 2017

Since the bulk of the changes has been added to both core and plugins (it's a matter of testing now), I've added some controls to the JavaScript API in janus.js to trigger renegotiations. The iceRestart: true flag when doing a createOffer to force an ICE restart was there already (see #753), so I'll skip that.

The important bits here are some additional flags you can set when doing a createOffer or createAnswer, to add, remove or replace one of the existing media you have in a call (audio, video and, partly, data). You know you can use the media object in createOffer and createAnswer to decide what should be captured, media direction and stuff like that (and if you didn't know, time to do your homework here). This patch now adds some new flags that allow you to drive what should happen in a renegotiation, e.g., whether you want to change the webcam you're using for video, if you want to stop sending audio, or start sending it if you weren't.

The new properties you can pass are the following:

  • addAudio: true: start capturing audio, if you weren't (will fail if you're sending audio already);
  • addVideo: true: start capturing video, if you weren't (will fail if you're sending video already);
  • addData: true: negotiate a datachannel, if it didn't exist (is a synonym for data: true);
  • removeAudio: true: stop capturing audio, and remove the local audio track;
  • removeVideo: true: stop capturing audio, and remove the local audio track;
  • replaceAudio: true: stop capturing the current audio (remove the local audio track), and capture a new audio source;
  • replaceVideo: true: stop capturing the current video (remove the local video track), and capture a new video source.

Notice that these properties are only processed when you're trying a renegotiation, and will be ignored when creating a new PeerConnection.

These properties don't replace the existing media properties, but go along with them. For instance, when adding a new video stream, or replacing an existing one, you can still use the video related properties as before, e.g., to pass a specific device ID or asking for a screenshare instead of a camera. Besides, notice that you'll currently have to pass info on the streams you want to keep as well, or they might be removed: this means that, if for instance you want to replace the video source, but want to keep the audio as it is, passing audio: false to the new createOffer will potentially disable audio.

Here are a few examples of how this works with the EchoTest demo (which I already updated to handle the additional onlocalstream and onremotestream calls that would follow renegotiations):

Remove video:

echotest.createOffer(
    {
        media: { removeVideo: true },
        success: function(jsep) {
            Janus.debug(jsep);
            echotest.send({message: {audio: true, video: true}, "jsep": jsep});
        },
        error: function(error) {
            bootbox.alert("WebRTC error... " + JSON.stringify(error));
        }
    });

Add video:

echotest.createOffer(
    {
        media: { addVideo: true },
        success: function(jsep) {
            Janus.debug(jsep);
            echotest.send({message: {audio: true, video: true}, "jsep": jsep});
        },
        error: function(error) {
            bootbox.alert("WebRTC error... " + JSON.stringify(error));
        }
    });

Replace video with a specific devideId:

echotest.createOffer(
    {
        media: {
            video: { deviceId: "44f4740bee234ce6ddcfea8e59e8ed7505054f75edf27e3a12294686b37ff6a7"    },
            replaceVideo: true
        },
        success: function(jsep) {
            Janus.debug(jsep);
            echotest.send({message: {audio: true, video: true}, "jsep": jsep});
        },
        error: function(error) {
            bootbox.alert("WebRTC error... " + JSON.stringify(error));
        }
    });

Hopefully this syntax is clean and simple enough as it is. If you start playing with it, please let me know if it's not working as expected and/or if it needs fine tuning.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 12, 2017

I updated all the relevant demos (some like screensharing and VP9 demos haven't been updated yet, but they're basically clones of the videoroom so I'll update them shortly) to support updates to local and remote streams resulting from renegotiations, so you should be able to test most if not all plugins for that. For instance, if you wanted to check how switching a camera in the videoroom demo works, you can use a command like this:

sfutest.createOffer(
    {
        media: {
            video: { deviceId: "44f4740bee234ce6ddcfea8e59e8ed7505054f75edf27e3a12294686b37ff6a7"    },
            replaceVideo: true
        },
        success: function(jsep) {
            Janus.debug(jsep);
            sfutest.send({message: {request: "configure"}, "jsep": jsep});
        },
        error: function(error) {
            bootbox.alert("WebRTC error... " + JSON.stringify(error));
        }
    });

Feedback welcome.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 13, 2017

Tested other demos/plugins as well, played with going from camera to screensharing and viceversa, etc., and everything seems to be working. That said, I'll wait for more comprehensive tests from other Janus users to merge, as I'm only doing some specific tests and cannot possibly cover all the common scenarios people play with.

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 19, 2017

Hi @lminiero, looks there is an issues with make command in this branch - I can't build Janus if disabled data channels support.

For example, I do the following:

git checkout -b renegotiation origin/renegotiation
chmod u+x autogen.sh
sh autogen.sh

then I do the following:

./configure --prefix=/opt/janus_ice_restart --enable-websockets --enable-rest --disable-data-channels --disable-rabbitmq --disable-mqtt --disable-plugin-audiobridge --disable-plugin-recordplay --disable-plugin-sip --disable-plugin-streaming --disable-plugin-videocall --disable-plugin-voicemail --disable-plugin-textroom --enable-post-processing PKG_CONFIG_PATH=/usr/local/opt/openssl/lib/pkgconfig

and receive this:

libsrtp version:           2.0.x
SSL/crypto library:        LibreSSL
DTLS set-timeout:          not available
DataChannels support:      no
Recordings post-processor: yes
TURN REST API client:      yes
Doxygen documentation:     no
Transports:
    REST (HTTP/HTTPS):     yes
    WebSockets:            yes
    RabbitMQ:              no
    MQTT:                  no
    Unix Sockets:          no
Plugins:
    Echo Test:             yes
    Streaming:             no
    Video Call:            no
    SIP Gateway (Sofia):   no
    SIP Gateway (libre):   no
    NoSIP (RTP Bridge):    yes
    Audio Bridge:          no
    Video Room:            yes
    Voice Mail:            no
    Record&Play:           no
    Text Room:             no
Event handlers:
    Sample event handler:  yes
    RabbitMQ event handler:no
JavaScript modules:        no

now I try to 'make' and receive 3 errors:

dtls.c:554:26: error: use of undeclared identifier 'janus_dtls_sctp_setup_thread'; did you mean 'janus_dtls_srtp_send_alert'?
g_thread_try_new(tname, janus_dtls_sctp_setup_thread, dtls, &error);

dtls.c:546:11: error: no member named 'sctp' in 'struct janus_dtls_srtp'
if(dtls->sctp == NULL) {

dtls.c:545:8: error: no member named 'sctp' in 'struct janus_dtls_srtp'
dtls->sctp = janus_sctp_association_create(dtls, handle->handle_id, 5000);

As I understand, the janus_dtls_sctp_setup_thread function is not defined if HAVE_SCTP is not defined

void *janus_dtls_sctp_setup_thread(void *data);

which actually will not be defined If I disabled the data-channels:

AC_DEFINE(HAVE_SCTP)

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 19, 2017

Thanks for the heads up! I'll have a look and fix shortly.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 19, 2017

@soulfly it should be fixed now, please let me know if that's not the case.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 19, 2017

Just tagged the latest version before renegotiations here. Merging this branch which becomes the new master 0.3.0. Hopefully this will encourage more people to experiment with its new features, and allows me to keep on working on what's coming next.

@lminiero lminiero merged commit 32e4478 into master Dec 19, 2017

@lminiero lminiero deleted the renegotiation branch Dec 19, 2017

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 19, 2017

@lminiero thanks, the issue is fixed now

now I receive 2 other errors:

janus.c:3233:74: error: no member named 'sctp' in 'struct janus_dtls_srtp'
stream->rtp_component->dtls != NULL && stream->rtp_component->dtls->sctp == NULL) {

and

janus.c:1379:80: error: no member named 'sctp' in 'struct janus_dtls_srtp'
&& stream->rtp_component->dtls != NULL && stream->rtp_component->dtls->sctp == NULL) {

from my understanding, this is related to your latest commit in this PR

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 19, 2017

Yep, probably one more #ifdef I have to add. I already merged this PR, so I'll apply the fix on master.

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 19, 2017

Fixed: 2fb2335

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 20, 2017

Thanks, works now!

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 20, 2017

@lminiero I'm a bit confused with VideoRoom plugin here

What is the final format of "configure" message to Janus server after create offer:

sfutest.send({message: {request: "configure", refresh: true}, jsep: jsep});

or

sfutest.send({message: {request: "configure", update: true}, jsep: jsep});

or

sfutest.send({message: {request: "configure"}, "jsep": jsep});

?

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 21, 2017

request: "configure" is enough, the core and plugin know it's a renegotiation, so you just need something to tell the plugin. You use restart: true (notice, restart, not refresh) only when you want to do an ICE restart, and only for viewers (for publishers the core knows it's a restart already).

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 21, 2017

Thanks @lminiero
I'm just curious, what is the purpose then for the update flag?
https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_videoroom.c#L4117

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 21, 2017

For viewers it's needed, as it's the only way to have the plugin send them a new offer. Publishers send a new offer themselves instead, so it's them who originate the renegotiation. You can find some more info on the rationale in #753 (which referred to ICE restarts but the principle is the same).

@soulfly

This comment has been minimized.

Contributor

soulfly commented Dec 22, 2017

I mean what is the difference between restart: true and update: true ?
I analized janus_videoroom.c and looks like it's all the same. Please confirm

@lminiero

This comment has been minimized.

Member

lminiero commented Dec 22, 2017

Yes, restart: true forces an ICE restart, update: true simply indicates a renegotiation.

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