create fallback turn server option #4391

CicadaCinema · 2021-11-01T17:26:12Z

PR Moved to #5781

Fixes #2329
Fixes #3063
You need to enable the fallback option in Settings > Voice & Video to use this feature:

Pull Request Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

bmarty

Thanks for the PR.
Please see my first remarks.
Also can you add a file for the changelog as per https://github.com/vector-im/element-android/blob/main/CONTRIBUTING.md#changelog ?

Thanks

bmarty · 2021-11-02T09:29:12Z

vector/src/main/java/im/vector/app/features/call/webrtc/WebRtcCall.kt

@@ -269,17 +271,28 @@ class WebRtcCall(
    private fun createPeerConnection(turnServerResponse: TurnServerResponse?) {
        val peerConnectionFactory = peerConnectionFactoryProvider.get() ?: return
        val iceServers = mutableListOf<PeerConnection.IceServer>().apply {
-            turnServerResponse?.let { server ->
-                server.uris?.forEach { uri ->
+            if (vectorPreferences.useFallbackTurnServer()) {


I'm not a specialist on this part of the code, but maybe use the fallback if and only if `turnServerResponse?.uris is null or empty, and the user has enable the fallback from the setting?

vector/src/main/java/im/vector/app/features/call/webrtc/WebRtcCall.kt

CicadaCinema · 2021-11-02T13:28:49Z

Thank you for reviewing this! I actually don't know if this change fixes #2329 at all, I've asked people there to test out my builds. I'll finish up work on this PR if I get positive feedback on the changes.

bmarty · 2021-11-02T15:26:18Z

Ok, thanks. I have enabled the CI for this PR so that APKs will be built and can be downloaded from here to be tested.

CicadaCinema · 2021-11-02T17:18:02Z

Thank you!

DoM1niC · 2021-11-06T14:08:37Z

Nice PR @CicadaCinema but I prefer to use the TURN first from Homeserver instead of general Matrix to use.... I think, you can read this from API or you switch the Server URLs to gradle vars to find and setup faster the own TURN Server like me....

But anyway good work 👍🏻

CicadaCinema · 2021-11-06T14:29:15Z

Nice PR @CicadaCinema but I prefer to use the TURN first from Homeserver instead of general Matrix to use.... I think, you can read this from API or you switch the Server URLs to gradle vars to find and setup faster the own TURN Server like me....

But anyway good work 👍🏻

Thank you!

If this setting is enabled, I plan to only use turn.matrix.org when the user's homeserver does not have one that they can use (similarly to the way the setting in Element Web works).

ouchadam · 2021-11-08T17:27:32Z

awesome change! with the fallback only being used when no turn server uris are returned by the server and the preference is enabled this looks like it ticks everyone's boxes

bmarty · 2021-11-15T16:55:43Z

vector/src/main/java/im/vector/app/features/call/webrtc/WebRtcCall.kt

-                server.uris?.forEach { uri ->
+                val useFallback = server.uris.isNullOrEmpty() && vectorPreferences.useFallbackTurnServer()
+                val serverList = if (useFallback) stringArrayProvider.getStringArray(R.array.fallback_ice_servers).toList() else server.uris
+                serverList?.forEach { uri ->


Maybe a stupid question, but do we want to use the fallback if turnServerResponse is null? Here this will not be the case.

Yes we should only use the fallback if the home server does not define any ice servers (consitent with web).

Ok, so the code turnServerResponse?.let { server -> has to be updated IIUC.

there, I've handled that now

bmarty

LGTM, just one question, but not blocking the merging

BillCarsonFr · 2021-11-15T17:31:29Z

Notice that using a fallback while revealing your IP to the relay will also hide it to your contact.
In other app this option can be presented as always relay Calls (to avoid revealing your IP adress to your contact).
So the current text copy is maybe a bit too negative

BillCarsonFr · 2021-11-15T17:33:21Z

Also the old android app was prompting the user to enable this option when you have a call that failed to connect and that your home server had no ice server config. Maybe a nice to have feature

BillCarsonFr · 2021-11-16T17:33:16Z

vector/src/main/java/im/vector/app/features/call/webrtc/WebRtcCall.kt

                    add(
                            PeerConnection
                                    .IceServer
                                    .builder(uri)
-                                    .setUsername(server.username)
-                                    .setPassword(server.password)
+                                    .setUsername(if (useFallback) "xxxxx" else server.username)


The fallback is stun, should do (from old sdk)

if (!defaultIceServerUri.startsWith("stun:")) { defaultIceServerUri = "stun:" + defaultIceServerUri; } defaultIceServer = PeerConnection.IceServer.builder(defaultIceServerUri).createIceServer();

BillCarsonFr · 2021-11-16T17:34:52Z

vector/src/main/res/values/config.xml

@@ -7,6 +7,12 @@
    <string name="matrix_org_server_url" translatable="false">https://matrix.org</string>
    <string name="piwik_server_url" translatable="false">https://piwik.riot.im</string>
    <string name="bug_report_url" translatable="false">https://riot.im/bugreports/submit</string>
+    <string name="fallback_stun_server_url" translatable="false">turn.matrix.org</string>
+    <string-array name="fallback_ice_servers" translatable="false">
+        <item>turn:turn.matrix.org:3478?transport=udp</item>


I don't think this will work, turn.matrix.org requires auth.
Just use stun: would work

fallback_ice_servers can just be removed now I think

yep, I've removed it

BillCarsonFr · 2021-11-16T17:41:30Z

vector/src/main/res/values/strings.xml

@@ -724,7 +724,7 @@
    <!-- Note to translators: the translation MUST contain the string "${app_name}", which will be replaced by the application name -->
    <string name="template_settings_call_ringtone_use_app_ringtone">Use default ${app_name} ringtone for incoming calls</string>
    <string name="settings_call_ringtone_use_default_stun">Allow fallback call assist server</string>
-    <string name="settings_call_ringtone_use_default_stun_sum">Will use "%s" as assist when your homeserver does not offer one (your IP address will be shared during a call)</string>
+    <string name="settings_call_ringtone_use_default_stun_sum">Will use %s as assist when your homeserver does not offer one (your IP address will be shared during a call)</string>


I think this copy is missleading. IP will be shared with who? the peer yes, the stun yes.
Not using it will still share your IP with the peer.
So maybe your IP will be seen by the stun server? (not sure how valuable/actionable this information is for the user, given that not doing it will make calls fails depending on your network)

BillCarsonFr

Hello,
The default server will only do stun, please see my comments.

also clarify option description, and mark old string to be removed

CicadaCinema · 2021-11-23T17:57:49Z

@BillCarsonFr

I adjusted the behaviour of the fallback to use the stun server instead, and I adjusted the option description.

Also the old android app was prompting the user to enable this option when you have a call that failed to connect and that your home server had no ice server config. Maybe a nice to have feature

I'm not sure how to even begin implementing this unfortunately - I only took a look at this issue because I noticed I couldn't make calls myself.

CicadaCinema · 2022-08-26T21:55:53Z

PR Moved to #5781

create fallback turn server option

4668f5d

CicadaCinema mentioned this pull request Nov 1, 2021

allow to use fallback call assist server (turn.matrix.org) #2329

Open

bmarty reviewed Nov 2, 2021

View reviewed changes

split long line

018ff66

CicadaCinema added 5 commits November 6, 2021 14:31

Add untranslatable string to preference summary

41f4e22

Merge branch 'develop' into develop

a70d817

tidy strings

bfb7d64

tidy strings

1c770c9

only use fallback if no other servers can be found

a8c6aa3

CicadaCinema marked this pull request as ready for review November 6, 2021 22:00

CicadaCinema requested a review from bmarty November 6, 2021 22:00

ouchadam approved these changes Nov 8, 2021

View reviewed changes

bmarty reviewed Nov 15, 2021

View reviewed changes

bmarty approved these changes Nov 15, 2021

View reviewed changes

BillCarsonFr reviewed Nov 16, 2021

View reviewed changes

BillCarsonFr requested changes Nov 16, 2021

View reviewed changes

CicadaCinema added 3 commits November 23, 2021 15:51

correct logic

196a77a

use stun server instead of turn

b3cd87e

also clarify option description, and mark old string to be removed

Merge branch 'develop' into develop

5b5effb

CicadaCinema requested a review from BillCarsonFr November 23, 2021 17:57

bmarty added the Z-Community-PR Issue is solved by a community member's PR label Nov 30, 2021

accommodate for turnServerResponse == null

760ab42

CicadaCinema requested a review from bmarty December 6, 2021 18:04

CicadaCinema added 2 commits December 6, 2021 18:15

appease the linter

ad95964

remove unnecessary string array

15677d7

CicadaCinema mentioned this pull request Apr 17, 2022

create fallback turn server option #5781

Closed

CicadaCinema closed this Apr 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create fallback turn server option #4391

create fallback turn server option #4391

CicadaCinema commented Nov 1, 2021 •

edited

Loading

bmarty left a comment

bmarty Nov 2, 2021

CicadaCinema commented Nov 2, 2021

bmarty commented Nov 2, 2021

CicadaCinema commented Nov 2, 2021

DoM1niC commented Nov 6, 2021 •

edited

Loading

CicadaCinema commented Nov 6, 2021 •

edited

Loading

ouchadam commented Nov 8, 2021

bmarty Nov 15, 2021

BillCarsonFr Nov 16, 2021

bmarty Nov 25, 2021

CicadaCinema Dec 6, 2021

bmarty left a comment

BillCarsonFr commented Nov 15, 2021

BillCarsonFr commented Nov 15, 2021

BillCarsonFr Nov 16, 2021

BillCarsonFr Nov 16, 2021

bmarty Dec 7, 2021

CicadaCinema Dec 8, 2021

BillCarsonFr Nov 16, 2021

BillCarsonFr left a comment

CicadaCinema commented Nov 23, 2021

CicadaCinema commented Aug 26, 2022

create fallback turn server option #4391

create fallback turn server option #4391

Conversation

CicadaCinema commented Nov 1, 2021 • edited Loading

PR Moved to #5781

Pull Request Checklist

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CicadaCinema commented Nov 2, 2021

bmarty commented Nov 2, 2021

CicadaCinema commented Nov 2, 2021

DoM1niC commented Nov 6, 2021 • edited Loading

CicadaCinema commented Nov 6, 2021 • edited Loading

ouchadam commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

BillCarsonFr commented Nov 15, 2021

BillCarsonFr commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

CicadaCinema commented Nov 23, 2021

CicadaCinema commented Aug 26, 2022

PR Moved to #5781

CicadaCinema commented Nov 1, 2021 •

edited

Loading

DoM1niC commented Nov 6, 2021 •

edited

Loading

CicadaCinema commented Nov 6, 2021 •

edited

Loading