Support for specifying STUN server: configure client (2.5/3)

[jotaen4tinypilot]

Related #1460.

Now that the backend is able to re-write the Janus configuration to make Janus connect to a STUN server, this PR adds the client counterpart. That means when the user enabled STUN, the frontend’s WebRTC library will also be configured to use the same STUN server.

This PR complements #1647, and strictly speaking, it even should have preceded it.

Note that the PR review is only about the code itself, but it does not include an end-to-end test of the full STUN functionality on device. Charles thankfully did that separately. (To sum that up: the testing procedure turned out to be rather tricky, unfortunately… We still aren’t 100% confident that our STUN implementation reliably solves connection issues in remote access scenarios, but given the testing complexity, we decided that the status quo has yielded enough evidence for us to move forward.)

Some notes on the code:

• Since the STUN server address comes from the backend and are hence validated, I used a rather pragmatic approach in createIceServerUrls to account for IPv6 addresses.
• webrtc-video.js is of type “module”, so it’s a bit tricky to pass parameters to it from the parent script. I think using global variables is not super beautiful, but it gets the job done. I’m open for better ideas, but the current mechanism would also be “good enough” for me.
• There is, unfortunately, one quirk with ESLint and Jinja templates, regarding the TINYPILOT_JANUS_STUN_PORT assignment, where we have to apply a rather ugly workaround. As far as I can see, there is not much we can do about that.

[jotaen4tinypilot]

@mtlynch I don’t have a fully-fledged test setup for STUN, so I’m not sure how to ultimately verify this PR on my end. I was successfully able to trace down the ICE-Server/STUN configuration in the Janus client, but I could only see that the STUN config gets passed on correctly inside the Janus client, however I was not able to verify that it actually has the desired effect. It neither says something in the logs (not even in verbose/debug mode), nor does it reveal anything in the Network tab of the browser console.

Would it make sense to loop in Charles here, and ask him to perform an end-to-end test of the STUN feature in its entirety? (I could set up a test bundle that contains all the things, including the yet-to-be-merged UI.) As an alternative, I could try to reproduce the test setup, but I’m not sure how long that takes.

[mtlynch]

Would it make sense to loop in Charles here, and ask him to perform an end-to-end test of the STUN feature in its entirety? (I could set up a test bundle that contains all the things, including the #1647.)

Yeah, that sounds like the best path here.

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved

---

In: app/static/js/webrtc-video.js:

> Line 208
  const server = stunServer.includes(":") ? `[${stunServer}]` : stunServer;

Is the ipv6 stuff necessary? From what I can see, ipv6 is disabled by default:
https://github.com/meetecho/janus-gateway/blob/v1.0.1/conf/janus.jcfg.sample.in#L214

---

In: app/templates/index.html:

> Line 92
    {% if janus_stun_server and janus_stun_port %}

Could we immediately initialize an ICE Servers variable? It doesn't seem like we need the separate server and port variables. Something like:

window.JANUS_ICE_SERVERS = [{urls: `stun:${stunSterver}:${stunPort}`}];

---

In: app/templates/index.html:

> Line 102
    </script>

An alternative to the eslint issue can be something like this:

<script id="janus-stun-server" type="application/json">{{ janus_stun_server|tojson|safe }}</script>
<script id="janus-stun-port" type="application/json">{{ janus_stun_port|tojson|safe }}</script>
<script>
  window.TINYPILOT_JANUS_STUN_SERVER = JSON.parse(document.getElementById("janus-stun-server").textContent);
  window.TINYPILOT_JANUS_STUN_PORT = JSON.parse(document.getElementById("janus-stun-port").textContent);
</script>

---

👀 @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: app/static/js/webrtc-video.js:

> Line 208
  const server = stunServer.includes(":") ? `[${stunServer}]` : stunServer;

As far as I understand, Janus provides out-of-the-box support for IPv6, and the media.ipv6 option is for explicitly specifying one IP mode, instead of having Janus selecting that on its own, e.g. when gathering DNS candidates. To me, the config file documentation isn’t super clear here, unfortunately. The CLI docs seem a bit clearer on the semantics here (the --ipv6-candidates CLI flag is the equivalent of the media.ipv6 file preference).

For example, even without media.ipv6 or --ipv6-candidates, Janus still accepts and processes an IPv6 STUN address:

A few notes on that screenshot:

• 2607:f8b0:4023:1401::7f is stun.l.google.com
• Janus doesn’t bother to format the IPv6 address correctly in the logs (by wrapping the server part in square brackets). The Janus frontend library would yield an error if we passed 2607:f8b0:4023:1401::7f:19302 instead of [2607:f8b0:4023:1401::7f]:19302.
• IPv6 support is disabled would be IPv6 support is enabled if media.ipv6 or --ipv6-candidates was set. The STUN server connection seems to work regardless, either way.

In regards to IPv4/IPv6 in general, my thinking was to consider and provide IPv6 support for the STUN feature from the start, seeing that we don’t need much extra effort as far as Janus is concerned. E.g., the backend implementation also supports IPv6 all the way.

---

In: app/templates/index.html:

> Line 92
    {% if janus_stun_server and janus_stun_port %}

My thinking was that the assembly of that ICE-server value was logic that’s closely related to Janus and our WebRTC initialization procedure in webrtc-video.js, so despite the higher LOC cost, I’d think it makes for slightly better encapsulation. (Think: setting the global variables is like specifying method arguments on the call-side, and the implementation details are hidden inside the module of the method body.)

---

In: app/templates/index.html:

> Line 102
    </script>

That’s an interesting approach as well – although, I feel that it probably also wouldn’t spare us a comment to explain why we take a detour through JSON-land, instead of assigning the values directly. I don’t feel strongly about either approach, so unless you do, I’d just keep the current code.

[jdeanwallace]

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

Review approved on CodeApprove