Correct msid handling for RtpSender #217
Merged
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
The previous logic would put negotiation into an endless loop after running `remove_track` due to incorrectly implementing the negotiation check and handling of stream ids.
k0nserv
commented
Jul 7, 2022
| @@ -447,27 +447,48 @@ impl RTCPeerConnection { | |||
| // if t.stopping && !t.stopped { | |||
| // return true | |||
| // } | |||
| let m = get_by_mid(t.mid().await.as_str(), local_desc); | |||
| let mid = t.mid().await; | |||
| let m = get_by_mid(&mid, local_desc); | |||
| // Step 5.2 | |||
| if !t.stopped.load(Ordering::SeqCst) && m.is_none() { | |||
| return true; | |||
| } | |||
| if !t.stopped.load(Ordering::SeqCst) { | |||
| if let Some(m) = m { | |||
| // Step 5.3.1 | |||
There was a problem hiding this comment.
See https://www.w3.org/TR/webrtc/#ref-for-dfn-check-if-negotiation-is-needed-1 for the steps outlined by the specification
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
==========================================
- Coverage 44.01% 43.79% -0.22%
==========================================
Files 68 68
Lines 9658 9667 +9
Branches 2794 2820 +26
==========================================
- Hits 4251 4234 -17
- Misses 3350 3374 +24
- Partials 2057 2059 +2
Continue to review full report at Codecov.
|
The way this code was written the `set_sending_track` call could sometimes not run if `sender.stop()` failed. With this change we make sure to attempt both operations always, even if one fails.
Match libWebrtc's behaviour which doesn't update `transcevier.[[Direction]]` instead of following the specification. See: https://webrtc.googlesource.com/src/+/c501f30333ce8b46a92b75a6bf75733ddb0e9741/pc/sdp_offer_answer.cc#2018
lolgesten
approved these changes
Jul 11, 2022
robashton
added a commit
to robashton/webrtc
that referenced
this pull request
Oct 10, 2022
robashton
added a commit
to robashton/webrtc
that referenced
this pull request
Oct 13, 2022
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.
The previous logic would put negotiation into an endless loop after
running
remove_trackdue to incorrectly implementing the negotiationcheck and handling of stream ids.
Upon calling
remove_trackthe following steps were carried out:remove_trackis associated withNone.There were three problems with this:
remove_trackimplementation is mostly correct, but it didn't always update the transceivers direction.In total this meant that when removing a track, at least some times, we'd end up in an endless negotiation loop where the check if negotiation was needed was always returning true, even after negotiating with the peer.