Skip to content

Conversation

@brknkfr
Copy link

@brknkfr brknkfr commented Apr 20, 2021

  • Provide multiple STUN servers to webrtc client when specified in the nextcloud backend instead of just 1 random one

@nickvergessen nickvergessen requested a review from danxuliu April 26, 2021 15:19
@nickvergessen nickvergessen added 3. to review enhancement feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients labels Apr 26, 2021
@nickvergessen nickvergessen added this to the 💖 Next Major (22) milestone Apr 26, 2021
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in the pull request for the TURN server settings the returned stunservers object resembles the RTCIceServer dictionary, but it is just data passed to the clients which each client parses on its own way.

In the case of the STUN server the url parameter must be kept too (it is used by the iOS client and the Android client; the WebUI uses the stunservers object as is). Moreover, note that the url parameter of the STUN server must be a string, not a list with a single element.

Similarly to the other pull request, composing the urls should be done in SignalingController::getSettings. In this case then it is probably not needed to have a Config::getStunSettings method, as it will just forward to Config::getStunServers. Although for simmetry with the TURN servers you could also add a Config::getStunSettings that returns a hardcoded stun: scheme, but I have no preference, as you prefer.

@nickvergessen
Copy link
Member

In the case of the STUN server the url parameter must be kept too (it is used by the iOS client and the Android client;

Same comment here as with the turn:
We can change that and break it (if it is any good and better or clearer or ....), as there is no compatibility possible between Talk 11 (conversations api v1-3) and Talk 12 (conversations api v4) as we can not make the multiple sessions compatible with uniqueness, we can also add a breaking signaling thing on top, especially when it's just about changing a single character

@brknkfr
Copy link
Author

brknkfr commented May 5, 2021

Thank you for the review.

Similarly to the other pull request, composing the urls should be done in SignalingController::getSettings. In this case then it is probably not needed to have a Config::getStunSettings method, as it will just forward to Config::getStunServers. Although for simmetry with the TURN servers you could also add a Config::getStunSettings that returns a hardcoded stun: scheme, but I have no preference, as you prefer.

I completely removed Config::getStunServer and Config::getStunSettings and moved building of the urls to SignalingController::getSettings.

@danxuliu
Copy link
Member

danxuliu commented May 6, 2021

Like explained in the other pull request I have added a fixup to keep the format, and I have extracted the format changes to their own pull request.

Please remember to adjust the commit message after doing the fixup :-)

Copy link
Author

@brknkfr brknkfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unifying urls will be done #5582

Copy link
Author

@brknkfr brknkfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unifying urls will be done #5582

@brknkfr brknkfr requested a review from danxuliu May 7, 2021 08:05
- Provide multiple STUN servers to webrtc client when specified in
  nextcloud backend instead of just 1 random one

Signed-off-by: Sebastian L <[email protected]>
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested* and works 👍

*Only in the WebUI, although from reading the mobile apps code it should work as well.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release client: 🤖🍏 mobile enhancement feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants