-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
pc1: use replaceTrack to stop and replace video on the fly #996
Conversation
wondering if this needs a check for replaceTrack in RTCRtpSender.prototype as just (literally minutes ago) got unflagged in Chrome |
b28dc77
to
d590c70
Compare
(not sure if this really belongs to pc1 but adding it here was the easiest way) |
d590c70
to
06099c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment about replaceTrack() in the info section?
demonstrating one use of replaceTrack and also how to determine support.
06099c5
to
589bd85
Compare
updated -- also cc'ing @henbos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You code looks good but I wonder if we might want to demo the seamless nature of replaceTrack() instead?
Though if it really is seamless the user might not realize anything happened :)
.catch(function(err) { | ||
console.error(err); | ||
}); | ||
}, 5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stops the video, waits for 5 seconds, then requests new media (which may add more delay) and replaces the track.
This makes sense to allow the camera to stop and restart and to make it apparent (thought 5 seconds seems a bit much?) that a restart happened.
However, if we want the sample to illustrate seamlessly replacing video, I think it makes more sense to do this:
navigator.mediaDevices.getUserMedia({video: true})
.then(stream => {
let sender = pc1.getSenders().find(s => { return s.track && s.track.kind === 'video'; });
return sender.replaceTrack(stream.getVideoTracks()[0]);
}).then(() => {
localStream.getVideoTracks()[0].stop();
localStream.removeTrack(localStream.getVideoTracks()[0]);
let sender = pc1.getSenders().find(s => { return s.track && s.track.kind === 'video'; });
localStream.addTrack(sender.track);
});
This...
- Only replaces the local video if it was able to send the new track to the other end.
- Performs replaceTrack() in a seamless way: because we don't ever send a stopped track, this better illustrates the seamless nature of replaceTrack(). The remote view should not look disrupted. If the same camera is used before and after, I would expect you not to be able to tell that replaceTrack() happened. If a different camera is used, I would expect the video to change without any delay or black frames in-between.
However...
- If getUserMedia() or replaceTrack() failed you might want to stop the local track anyway and log an error, this would require adding a catch to the above code. Or you can keep using the old track to demo that the web page can recover from not getting a new track, it depends on what you think the user expects to happen from a restart button.
- To demo seamlessness, because we don't stop the first track until it is no longer in-use (we always have 1 or 2 tracks), we don't give the camera a chance to reboot. (But if you want that you can do stop and then do start and don't need this new button.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stop, wait and then send new, while it uses the new replaceTrack() API, illustrates a use-case that was already achievable through addTrack() and removeTrack().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the best demo for this is actually with two devices -- and things like a front and a back camera where you can use facingmode. But not many devices do that.
Also this is more for QA.
removeTrack is different, you can't reuse the sender.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, it's different in that it reuses the sender and doesn't require renegotiation, but the seamlessness of replaceTrack() is not illustrated when you stop and wait, from a user perspective you might as well have removed it.
The suggested changes would be good to illustrate the two device case in a seamless way. And for two devices there is no need to "reboot" the camera so it's a good way to do demo it.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the sender can be reused after removeTrack sigh)
Went to the audio demo for a "seamless restart" thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in some cases a sender can be reused, but "addTrack, O/A, removeTrack, addTrack" does not reuse it because the MID has been chosen.
As I mention in #1009 let's avoid any mute-by-replaceTrack examples. What if we switched in another live track? That seems more rewarding to me, and the kind of thing we should steer people toward replaceTrack for. The challenge perhaps is that most people on desktop don't have two cameras (and phones generally won't let you open both), perhaps we can use |
People will use them anyway, if we show this at least we know they do it in a way we expect? canvas.captureStream isn't universally supported sadly. For QA purposes we could detection multi-camera laptops and conditionally enable switching. This would also be great for facing mode. @KaptenJansson what is your take? |
Because it has all these problems, and it's really not why we added I think way more people have browsers that support |
move to https://github.com/webrtc/test-pages instead of a sample |
based on pc1 sample and the following two PRs: webrtc/samples#1009 webrtc/samples#996
demonstrating one use of replaceTrack. From https://bugs.chromium.org/p/chromium/issues/detail?id=803004