fix(RTC): Set transceiver direction after RTCRtpSender#replaceTrack. #1618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
957a6b4 to
37a0e71
Compare
paweldomas
reviewed
May 21, 2021
modules/RTC/TPCUtils.js
Outdated
| const transceiver = oldTrack.isVideoTrack() && browser.doesVideoMuteByStreamRemove && oldTrack.isMuted() | ||
| ? this._findTransceiver(mediaType) | ||
| : this._findTransceiver(mediaType, oldTrack); | ||
| const track = mediaType === MediaType.AUDIO ? stream.getAudioTracks()[0] : stream.getVideoTracks()[0]; |
There was a problem hiding this comment.
why do you need to fetch the track this way? shouldn't it be accessible from the newTrack via getTrack() getter?
Btw. naming of jitsi tracks and webrtc media stream/tracks is very confusing. Maybe it would help to rename the arguments to oldJitsiTrack and newJitsiTrack.
| this.pc.localTracks.delete(oldTrack.rtcId); | ||
| this.pc.localSSRCs.delete(oldTrack.rtcId); | ||
| }); | ||
| } else if (newTrack && !oldTrack) { | ||
| const ssrc = this.pc.localSSRCs.get(newTrack.rtcId); | ||
|
|
||
| return this.addTrackUnmute(newTrack) | ||
| .then(() => this.setEncodings(newTrack)) |
There was a problem hiding this comment.
is setEncodings needed for the if (oldTrack && newTrack) case as well?
There was a problem hiding this comment.
It's not needed if there was a source already and only the stream is being replaced on the sender. However, if the client starts muted and the source is being added on unmute, then the encodings need to be configured.
paweldomas
reviewed
May 21, 2021
This fixes the issue where TRACK_REMOVED event is not fired when a remote track is removed from the peerconnection. Fixes jitsi#1612 and jitsi/jitsi-meet#8482.
If the msid attribute is missing, then remove the ssrc from the transformed description so that a source-remove is signaled to Jicofo. This happens when the direction of the transceiver (or m-line) is set to 'inactive' or 'recvonly' on Firefox. Not signaling these source updates creates issues with remote track handling on the other endpoints in the call.
f9a0c0e to
0478900
Compare
The tracks will not be added when the call switches from jvb to p2p for an endpoint that joins muted by focus.
… or 'recvonly'. This is needed only for JVB connections. Add unit tests for LocalSdpMunger.
…e ssrcs. The direction was marked as 'inactive' only on Firefox as Safari had audio issues when an inactive mid is re-used. Chrome (in unified-plan) needs the direction of the mid in remote desc to be set to 'inactive' for a 'removetrack' to be fired on the associated media stream whenever a remote source is removed.
0478900 to
968a4db
Compare
paweldomas
reviewed
May 26, 2021
modules/sdp/LocalSdpMunger.js
Outdated
| !msidExists && mediaSection.ssrcs.push({ | ||
| id: source, | ||
| attribute: 'msid', | ||
| value: `${this.localEndpointId}-${mediaSection.mLine?.type}-${pcId} ${trackId}-${pcId}` |
There was a problem hiding this comment.
can we extract msid generation into a function? it seems to be duplicated with the logic at line 190?
4b25a6f to
c1dab39
Compare
paweldomas
approved these changes
May 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 fixes the issue where TRACK_REMOVED event is not fired when a remote track is removed from the peerconnection.
Fixes #1612 and jitsi/jitsi-meet#8482.