Fix handling offers in already established connections #1823
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
danxuliu
added
bug
3 tasks
danxuliu
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
from
f952442 to
f3049e6
Compare
Collaborator
Lint
SpotBugs (new)
SpotBugs (master)
|
nextcloud-command
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
from
f3049e6 to
98bbae6
Compare
nextcloud-command
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
from
98bbae6 to
9ab11fb
Compare
Member
|
/rebase |
nextcloud-command
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
from
9ab11fb to
b8304f9
Compare
danxuliu
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
2 times, most recently
from
7cc5ec0 to
f32cd44
Compare
Although the OfferNickProvider itself is still added and removed based on the call participant the listeners for the video and screen connections are now added and removed based on the actual peer connections that they listen to. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signaling messages related to peer connections include a "sid" parameter to be able to identify the connection. At any given time it is expected that there will be a single peer connection for certain session and type, and the sid is used to verify that both ends are using the same peer connection. Signaling messages with a different sid can be ignored, unless they are offers; if an offer is received for an existing peer connection with a different sid that means that the existing peer connection is stale, so it should be removed and a new one should be created instead. This can happen, for example, if a new offer is requested to the HPB (which creates a new peer connection in the HPB, so the existing one can no longer be used). On the other hand, if an offer with the same sid as the existing peer connection is received that means that the offer is a renegotiation, and it should be handled by the existing peer connection. Note that a signaling message may not contain a "sid" parameter if an older HPB is being used (external signaling server < 0.5.0), or when the other participant is using an older app version. In that case it is not possible to know if a signaling message for certain session and type actually refers to the existing connection or a different one, so it is always handled as if it was for the existing connection (which was the behaviour until now). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Once a RTCPeerConnection is established new offers can still be received for that connection. These offers will update/renegotiate the connection (for example, to add a video track to an audio only connection) and need to be handled like the offers to establish the original connection (the offer needs to be set as the remote description and a local answer needs to be created for it, set as the local description and sent to the other peer). In the PeerConnectionWrapper the same SdpObserver object is called when setting both local and remote descriptions, so the answer should be created only when a remote description was set. Before the answer was created if there was no local description already, so this only worked when establishing the initial connection. Once the connection is established new answers need to replace the current local description, so now the creation of new answers is based on the signaling state instead. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
fix-handling-offers-in-already-established-connections
branch
from
53dd98c to
1b5be43
Compare
Contributor
Contributor
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/1823-talk.apk |
Member
|
Whats the status? |
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 #1815
Requires #2600 (so the first commit of this pull request is Add and remove nick provider listeners based on the peer connections)
Needed for nextcloud/spreed#6896
Once a RTCPeerConnection is established new offers can still be received for that connection. These offers will update/renegotiate the connection (for example, to add a video track to an audio only connection) and need to be handled like the offers to establish the original connection (the offer needs to be set as the remote description and a local answer needs to be created for it, set as the local description and sent to the other peer).
Pending:
sidas the one from the current connection is provided in the offersidparameter at all; in that case a renegotiation would be started as well, as withoutsidthere is no way to differentiate between connections (although renegotiations would not be expected anyway if using those older versions)sidis received (as done in the WebUI; this is actually a bug in the Android app)How to test
Result with this pull request
In the Android app the video is now visible (note that, due to a bug in the WebUI, the video may be visible even before being actually enabled in the browser)
Result without this pull request
In the Android app the video is not visible