jotaen4tinypilot
commented
10 months ago @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
Related https://github.com/tiny-pilot/tinypilot/issues/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 https://github.com/tiny-pilot/tinypilot/pull/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:
createIceServerUrlsto account for IPv6 addresses.webrtc-video.jsis 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.TINYPILOT_JANUS_STUN_PORTassignment, where we have to apply a rather ugly workaround. As far as I can see, there is not much we can do about that.