Fix removal of stale peers when a screen is shared #1569
Merged
Conversation
This file contains hidden or 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
When a screen is shared a new offer is sent, just like when a video (or audio only) connection starts; any offer coming from the same peer has the same id, even if the type of the offer is different and separate Peer objects are created for each offer. Due to this now a peer is stale only if both the id and type of the new offer matches an existing peer; otherwise when the screen offer was received the video peer was seen as stale and removed. Note, however, that even if the id and type of the new offer matches an existing peer its participant should not be marked as disconnected, as that removes all its Peer objects. If that happens then the connection becames unstable with the peers sending offers back and forth due to the difference in how they are started (immediately or delayed for video depending on the ids of both peer, immediately for screens but only for the peer that shares), the removal of Peers when a new offer is received, and the creation of new Peers (and, with them, new offers) when the signaling returns the user list. Moreover, if a stale peer is found it is going to be replaced by a fresh peer, so there it is not really needed to perform the whole disconnection process. Therefore, now the stale peer object is simply ended instead (and in the case of video peers their video and speaker are explicitly removed; this should probably be done automatically when handling the peer end, like done for screen peers, but for the time being an explicit call is used just like in other areas of the file). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a stale peer reconnects its Peer object, and thus its video view, is removed and created again. The video view always set the screen icon as not available and it was later changed to available if a screen offer was received, so if the peer sent the screen offer before the video offer the screen icon was never set as available. Now whether the screen is shared or not is taken into account when setting the initial state of the video view. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This ensures that receiving a screen offer will not abort a delayed reconnection of the video. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
added
3. to review
bug
regression
feature: WebRTC 🚡
25 tasks
nickvergessen
approved these changes
Feb 21, 2019
nickvergessen
deleted the
fix-removal-of-stale-peers-when-a-screen-is-shared
branch
Member
|
/backport to stable15 |
|
The backport to stable15 failed. Please do this backport manually. |
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.
Fixes #1559
Follow up to #1533
When a screen is shared a new offer is sent, just like when a video (or audio only) connection starts; any offer coming from the same peer has the same id, even if the type of the offer is different and separate Peer objects are created for each offer. Due to this now a peer is stale only if both the id and type of the new offer matches an existing peer; otherwise when the screen offer was received the video peer was seen as stale and removed.
Besides fixing #1559 the first commit also addresses the issues from #1533 but for shared screens instead of videos. However, there is still a pending issue: if the signaling returns a wrong user list to the user A causing it to disconnect from user B while user B is sharing the screen, when user A reconnects only video will be received, but not the screen. However, this should be a pretty uncommon scenario, and even if it happens stopping the screen sharing and starting it again makes it work again, so I will dig into it in the future (I will open an issue to not forget about it).
The second commit fixes the screen icon in the VideoView when there is a reconnection caused by a wrong user list returned by the signaling (like the issues in #1533). The third commit is just a "formal" fix; theoretically it could be a problem if user A shares the screen while user B is waiting for a delayed reconnection (pretty uncommon scenario too), but I have not actually tested it (nor found it "in the wild").
Note that the second commit can not be automatically backported to stable15 due to depending on #1513.
Finally, two users sharing their screens do not work, but it is not a recent issue; it has been broken since the MCU support was added, so please share only a single screen at a time when testing this ;-)