-
Notifications
You must be signed in to change notification settings - Fork 509
Use renegotiations to update publisher connections #6896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I'm running this branch against strukturag/nextcloud-spreed-signaling#195, how can I test the renegotiation? Starting the call with the camera / microphone disabled and then enabling them later doesn't seem to trigger the new behaviour (a full audio/video SDP is already exchanged initially). |
|
Sorry, I forgot to add the steps to the pull request description, fixed now :-) In any case please note that you need to unselect the video device, it is not enough to just start with video disabled (it should... but not establishing a video connection when starting with video disabled has been broken for a while, I will address that in a different pull request). |
|
Tested with strukturag/nextcloud-spreed-signaling#195 and works. Observations from testing:
|
|
ae6aac2 to
e31893f
Compare
|
@fancycode Sorry for the delay and thanks a lot for the testing!
Currently if you change to a different camera the video should be automatically disabled, so if the new video was sent enabled that is (was? See below) a bug. It is worth noting, however, that people seem to find that disabling the media when changing to another device is surprising and counterintuitive, so that behaviour may be changed. Regarding waiting until the dialog is closed to do the actual switch most settings in Talk and Nextcloud are immediately updated when you change them, so it could be argued that it is expected that the device is switched as soon as you select a different one. Besides that, the code also had to be designed like that to overcome some browser limitations, like not being able to have two different video streams from two different cameras active at the same time. Unfortunately, at least for the time being, this will need to behave the way it does :-) Having said all that, personally I prefer settings dialogs with accept, apply and cancel buttons that do not save the changes until you explicitly tell them to ;-)
When a participant joins a call the current media state is repeteadly sent with an incresing timeout (1, 2, 4, 8, 16 and 32 seconds) to ensure that the other clients will set the right state once they finally established the connection. The mixed messages were probably caused by a bug fixed in #7014. I have rebased on top of that and now the messages should be consistent (unless you manually modify the state while the state messages are still sent). But if you still see something unexpected please let me know :-) |
e31893f to
870dc97
Compare
870dc97 to
0cf40de
Compare
0cf40de to
ef40d4b
Compare
Requires strukturag/nextcloud-spreed-signaling#229
Requires nextcloud/talk-ios#731 and nextcloud/talk-ios#732
Requires nextcloud/talk-android#1823
When the HPB supports the
publisher-updateupdate-sdpfeature sending an updated offer for a publisher will propagate the renegotiation to the subscribers as well. Therefore, in those cases there is no need to force a reconnection, it is enough to send the new offer to trigger the renegotiation (given that all the clients will handle the updated offer using the current connectionThe mobile apps will require a minor change).Note that this can be used only to update an existing connection. If a participant is not sending any media and then starts to do it (no matter if a device was now selected or if the permissions were granted) sending an offer to the HPB would establish the publisher connection, but as the other clients do not have a subscriber connection already they will not receive any updated offer, they would need to explicitly request it. This might be doable with just a small change in the clients, but it will be something for another pull request.
Similarly, when the HPB is not used sending a new offer might work to establish the connection with the other clients, even without changes in the mobile apps. However, stopping the connection (for example, when the permissions are revoked) would require some changes anyway (it might be possible to send an offer to stop sending media, but that would still keep the connection open, even if no media is being transmitted).
Independently of all that, if the external signaling server does not support the new
update-sdpfeature then forced reconnections will still be used like before.Pending:
How to test (scenario 1)
Result with this pull request
In the original window the video is now visible even if no forced reconnection was triggered, only a renegotiation
Result without this pull request
In the private window a forced reconnection was triggered
How to test (scenario 2)
Result with this pull request
In the original window the video is now visible even if no forced reconnection was triggered, only a renegotiation
Result without this pull request
In the private window a forced reconnection was triggered