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

Don't use .clone() on tracks to render them in demos #3009

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

lminiero
Copy link
Member

@fippo made me aware of an incorrect use of the WebRTC APIs in our multistream demos, and namely the fact we were using clone() on MediaStreamTrack instances (both local and remote) to render them. The motivation for using .clone() was that, especially for remote streams, we can now have more than one audio and/or video track in the same stream, and so relying on the "original" stream wouldn't allow them to be playable in <audio> or <video> elements. That said, while the principle is correct, the way we were "solving" this wasn't, as explained in this issue where the bug was reported.

As such, this PR changes all the .clone() instances, to pass the track directly in the MediaStream constructor instead. This means that, wherever we had something like this:

stream = new MediaStream();
stream.addTrack(track.clone());

we do this instead:

stream = new MediaStream([track]);

This is a PR and not a direct commit just to give people the chance to test all demos and ensure they still work as expected. I tried a few, especially those involving multiple streams, and they all seemed to be fine, but you may want to test this yourselves as well and provide feedback before I merge (which will be soon).

@lminiero lminiero added the multistream Related to Janus 1.x label Jun 28, 2022
@lminiero
Copy link
Member Author

Merging.

@lminiero lminiero merged commit 52f86d5 into master Jun 30, 2022
@lminiero lminiero deleted the attack-of-the-clones branch June 30, 2022 07:08
@fippo
Copy link
Contributor

fippo commented Jun 30, 2022

and I did not even have to compare track cloning to putting 🍍 on 🍕 ;-)

@nakkag
Copy link

nakkag commented Aug 17, 2022

I have imported this merge into my program and now the video stops in Chrome.

Plug-in is video room.
Reproduced in Chrome on desktop and Chrome on Android.

When you are a subscriber in Chrome, if there is packet loss at the publisher, an onmute/onunmute will occur in the subscriber.
This will trigger the onremotetrack event.
When the track is stopped in the onremotetrack, the onunmute no longer occurs and the video is stopped.

If ".clone()" is added with addTrack, even if the track is stopped in onremotetrack, onunmute will occur and the video will be displayed again.

Reference
#2145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants