nextcloud / spreed

🗨️ Nextcloud Talk – chat, video & audio calls for Nextcloud
https://nextcloud.com/talk
GNU Affero General Public License v3.0
1.6k stars 428 forks source link

Kill refreshing of signaling settings? #6778

Open nickvergessen opened 2 years ago

nickvergessen commented 2 years ago

How to use GitHub


Steps to reproduce

  1. Change from one room to another
  2. Have the network tab open
  3. See a request to GET /ocs/v2.php/apps/spreed/api/v3/signaling/settings?token=TOKEN

In case internal signaling is used or if external signaling is used with only 1 HPB configured, the request will always return the same signaling server, so we will always NOT hit: https://github.com/nextcloud/spreed/blob/b7538684b9cce53ff9aa087ec6e00b94934b9001/src/utils/webrtc/index.js#L91-L104

So the signaling settings are always the same and unless you join the call it is not needed. Only when you join the call a refresh of the TURN details would make sense to ensure that your TURN ticket is new enough, ref https://github.com/nextcloud/spreed/issues/6323

But since that ticket is still open we could also have a look at that after this change.

Thoughts? @fancycode @danxuliu

nickvergessen commented 2 years ago

If in the meantime a second server would have been added, or signaling mode changed, the talk hash would be dirty and clicking on another chat would reload the page preventing old usage anyway.

nickvergessen commented 2 years ago

I think the request is not very heavy, but for the company call scenario that is at:

      9 /2022:13:59
     27 /2022:14:00
     14 /2022:14:01
      8 /2022:14:02
      9 /2022:14:03

67 requests done or not

fancycode commented 2 years ago

Sounds good to me. Every request removed is a good thing ;-)

Once you join the call (or subscribe other peer connections), you could then check if you still have valid TURN credentials and if so, get rid of this request, too (#6323).

danxuliu commented 2 years ago

Yes, I think that refreshing the signaling settings can be removed if there is no external signaling server or if there is only one external signaling server.

However, refreshing the signaling settings when joining a conversation would be still needed if there is more than one external signaling server, so I guess that the signaling settings response should include something like totalServerCount: int, serverCanChange: boolean or something like that so the clients know if they need to refresh the settings when joining a conversation or not.

nickvergessen commented 2 years ago

I would just go with a new singaling mode string similarly to the conversation clustering one

nickvergessen commented 1 month ago

@danxuliu this is now somewhat obsolete?

fancycode commented 1 month ago

For hello-v2, a JWT token is part of the signaling settings which has an expiration time. Depending on the validity, it can happen that the token expires while the user still has the browser open, so (re-)connecting might not work in these cases if the settings are not updated.

See error in #11216 ("The token is expired").

danxuliu commented 1 month ago

@danxuliu this is now somewhat obsolete?

Not necessarily. Refreshing the signaling when changing the conversation would be needed when switching from non-federated to federated and the other way around, but it would not be strictly needed when changing between conversations in the same local server (except for the mentioned cases like refreshing TURN server credentials, which does not really need to be done when joining the conversation but the call, or the JWT token, which could be done if an error is triggered).