Skip to content

Commit

Permalink
Support for specifying STUN server: configure client (2.5/3) (#1657)
Browse files Browse the repository at this point in the history
Related #1460.

Now that the [backend is able to re-write the Janus configuration to
make Janus connect to a STUN
server](#1646), 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](#1460 (comment)).
(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](#1460 (comment)).)

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.
  • Loading branch information
jotaen4tinypilot authored Oct 31, 2023
1 parent 6791c11 commit a8428c6
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 0 deletions.
20 changes: 20 additions & 0 deletions app/static/js/webrtc-video.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ const config = {

// The number of seconds within which the watch request can be retried.
watchRequestRetryTimeoutSeconds: 60,

stunServer: window.TINYPILOT_JANUS_STUN_SERVER,
stunPort: window.TINYPILOT_JANUS_STUN_PORT,
};

// Initialize library.
Expand All @@ -49,6 +52,10 @@ const remoteScreen = document.getElementById("remote-screen");
const janus = new Janus({
server: `${config.useSSL ? "wss" : "ws"}://${config.deviceHostname}/janus/ws`,
success: attachToJanusPlugin,
iceServers:
config.stunServer && config.stunPort
? createIceServerUrls(config.stunServer, config.stunPort)
: undefined,

/**
* This callback is triggered if either the initial connection couldn’t be
Expand Down Expand Up @@ -188,3 +195,16 @@ function attachToJanusPlugin() {
},
});
}

/**
* @param {string} stunServer
* @param {number} stunPort
* @returns {Object[]} - Array of URL objects according to https://developer.mozilla.org/en-US/docs/Web/API/RTCIceServer/urls.
*/
function createIceServerUrls(stunServer, stunPort) {
// If the server value contains a colon, it’s an IPv6 address. In this case,
// we need to wrap the server part into square brackets, in order to be able
// to join it correctly with the port.
const server = stunServer.includes(":") ? `[${stunServer}]` : stunServer;
return [{ urls: `stun:${server}:${stunPort}` }];
}
12 changes: 12 additions & 0 deletions app/templates/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,18 @@
type="text/javascript"
src="/third-party/janus-gateway/1.0.0/janus.js"
></script>
{% if janus_stun_server and janus_stun_port %}
<script>
window.TINYPILOT_JANUS_STUN_SERVER = "{{ janus_stun_server }}";
// ESLint wouldn’t be able to parse the following line if there was just
// the “naked” Jinja2 template expression on the right-hand side of the
// assignment. Hence, we have to wrap the value into a string and call
// `parseInt` to obtain the original value.
// It wouldn’t be possible here to use an ESLint directive, since the
// error already happens at parsing stage.
window.TINYPILOT_JANUS_STUN_PORT = parseInt("{{ janus_stun_port }}");
</script>
{% endif %}
<script type="module" src="/js/webrtc-video.js"></script>
{% endif %}

Expand Down
8 changes: 8 additions & 0 deletions app/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import db.settings
import hostname
import update.settings
from find_files import find as find_files

views_blueprint = flask.Blueprint('views', __name__, url_prefix='')
Expand All @@ -15,9 +16,16 @@ def index_get():
use_webrtc = db.settings.Settings().get_streaming_mode(
) == db.settings.StreamingMode.H264

try:
update_settings = update.settings.load()
except update.settings.LoadSettingsError:
return flask.abort(500)

return flask.render_template(
'index.html',
use_webrtc_remote_screen=use_webrtc,
janus_stun_server=update_settings.janus_stun_server,
janus_stun_port=update_settings.janus_stun_port,
page_title_prefix=_page_title_prefix(),
is_standalone_mode=_is_standalone_mode(),
custom_elements_files=find_files.custom_elements_files())
Expand Down

0 comments on commit a8428c6

Please sign in to comment.