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

janus.js - renegotiate with external stream #2604

Merged
merged 10 commits into from
May 6, 2021

Conversation

kmeyerhofer
Copy link
Contributor

This PR adds the ability for an existing PeerConnection to replaceVideo with an external stream and back again through the createOffer renegotiation.

The issue this PR solves is described in this Google Group post https://groups.google.com/g/meetecho-janus/c/Ucz-U3OORHM/m/mo6lXkdjAwAJ
To summarize the issue, a renegotiation createOffer sent with a canvas MediaStream (external stream) does not replace the track of the existing PeerConnection.

This PR allows the stream property sent in a createOffer parameter to be an object which can include the replaceVideo and externalStream properties, similar to the media property. The existing stream property behavior has not been modified.

Here is a working example:

// Assume Janus is correctly initialized
var canvas = document.getElementById('canvas');
var stream = canvas.captureStream();
janusPlugin.createOffer({
  stream: {
    externalStream: stream,
    replaceVideo: true,
  },
  simulcast: false,
  simulcast2: false,
  success: (jsep) => {
    janusPlugin.send({ message: { request: "configure", audio: true, video: true}, jsep });
  },
  error: (error) => { Janus.error('error', error); },
});

Using createOffer in this manner replaces a user media (video camera) stream with an external MediaStream and an external MediaStream with another external MediaStream.

To renegotiate the same peer connection from an external MediaStream back to user media (video camera) utilizes the same media property as before. Here is a working example:

// Assume Janus is correctly initialized
janusPlugin.createOffer({
  media: {
    video: userMediaVideoDevice,
    replaceVideo: true,
  },
  simulcast: false,
  simulcast2: false,
  success: (jsep) => {
    janusPlugin.send({ message: { request: "configure", audio: true, video: true}, jsep });
  },
  error: (error) => { Janus.error('error', error); },
});

In summary this change allows a PeerConnection renegotiation to replace the video in the following three conditions:

  1. User media -> external MediaStream
  2. External MediaStream -> external MediaStream
  3. External MediaStream -> User media

Implementation decision making

I was a bit unsure of design implications regarding this note in the createOffer/createAnswer documentation (emphasis added):

stream: optional, only to be passed in case you obtained a MediaStream object yourself with a getUserMedia request, and that you want the library to use instead of having it get one by itself (makes the media property useless, as it won't be read for accessing any device);

This is why I chose to modify the stream object instead of using the already available { media: { video: true, replaceVideo: true } } property.

@januscla
Copy link

Thanks for your contribution, @kmeyerhofer! Please make sure you sign our CLA, as it's a required step before we can merge this.

@kmeyerhofer
Copy link
Contributor Author

CLA signed.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! But I feel this needlessly makes things more complicated than they should. My feeling is that this could work just as easily by reusing the existing properties: if media contains replaceAudio and/or replaceVideo and stream contains a (new) external MediaStream, then replace the tracks (audio and/or video) with the ones from the new stream and renegotiate. Or am I missing something?

Don't worry too much about what the docs say: for the JS part, most was written at the very beginning, and they may not be exactly up-to-date. The fact media is ignored, for instance, is not true, since media direction is taken into account even when using external streams.

html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
@kmeyerhofer
Copy link
Contributor Author

Thanks for your review Lorenzo. I agree with your concern that these changes make things more complicated. Since you've clarified that using media replaceAudio and replaceVideo with a new stream is preferred, I can rewrite these changes to use them.

@kmeyerhofer
Copy link
Contributor Author

Based on your feedback @lminiero, I have simplified the previous changes to utilize the media replaceAudio and replaceVideo properties.

With the added changes, a createOffer or createAnswer can now be used like this:

// Assume Janus is correctly initialized
var canvas = document.getElementById('canvas');
var stream = canvas.captureStream();
janusPlugin.createOffer({
  stream: stream,
  media: {
    replaceAudio: true,
    replaceVideo: true,
  },
  simulcast: false,
  simulcast2: false,
  success: (jsep) => {
    janusPlugin.send({ message: { request: "configure", audio: true, video: true}, jsep });
  },
  error: (error) => { Janus.error('error', error); },
});

@lminiero
Copy link
Member

Thanks! I'll have a look at the new changes and make some tests when I get the chance.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few notes inline.

html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

lminiero commented May 4, 2021

@kmeyerhofer can this be merged? We never use external streams ourselves, so if you tested this and it works for you, it's good enough for me!

@kmeyerhofer
Copy link
Contributor Author

@lminiero Yes this is ready to be merged, thanks!

@lminiero
Copy link
Member

lminiero commented May 6, 2021

Merging then, thanks again 👍

@lminiero lminiero merged commit d167957 into meetecho:master May 6, 2021
@lminiero lminiero removed the WIP label Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants