Don't call .stop() in onremotetrack when track is notified as removed by janus.js (fixes #3055) #3056
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for the issue reported in #3055, where the symptom was the multistream VideoRoom demo breaking when someone left a room and then joined again (or someone else joined): for existing subscribers, m-lines would be reused, and media would be broken. This was caused by us closing the remote stream in the
onremotetrack
callback for a removed track: since the new track (new user) actually remains the same after the renegotiation, this ends up breaking the rendering.Initially we thought it might be caused by us re-using track IDs in SDPs from Janus, but even after making sure we generate fresh ones when needed, the problem remained the same, and we could see the
ontrack
callback of the PeerConnection would still report the previous track ID as being added: not sure why, but maybe because as far as SDP is concerned the fact we changed the ID of the track didn't change anything, since the m-line still "lives", and so the browser assumed the previous one was still in use and ignored the new one.As such, we decided to go for a different fix, and just removed all calls to
.stop()
for the remote track when one is removed. We gave this some thought and realized this could have caused issues also when a track was only temporarily paused, e.g., freezed because of losses: in fact, theonremotetrack(false)
event is fired in response to anonmuted
event on the track, notonended
, which is by definition not conclusive. Besides, the.stop()
calls we had were probably a leftover when we still cloned tracks before using them in demos, which is something we stopped doing in #3009.Not sure if this can cause leaks in demos, now, but it definitely fixes the problem. Feedback would be welcome before we merge, of course. Also pinging @fippo who may point out gross mistakes we're doing 🤭