-
-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for specifying a STUN server for Janus #1460
Comments
I started to look into this, and read up on the (super helpful) research, and also STUN and NAT issues in general. I’d like to bounce and discuss a few thoughts around the implementation, to get a better understanding, and to ensure alignment in regards to our approach. Once I’m back from my break after next week, I could start to dive into this more, and come up with a more concrete plan of action. BackendBackend-wise, I’m wondering what the best approach would be to tackle this. In order to toggle and configure STUN mode in Janus, we have to write the STUN server address and port into the Janus config file, and then restart the uStreamer and Janus systemd services to apply the changes. “Historically”, we would have created two variables However, as we are moving away from Ansible, that’s probably not the way we want to do it. So the questions are: how do we maintain state, and who is in charge for applying it? Some options:
In regards to (a), I think (a.1) would be the easiest and most consistent place – we also still store all uStreamer parameters there. (a.2) would make it difficult to process the data outside of the Python app (e.g. via bash scripts, or other utilities), and (a.3) would feel a bit hacky, since we’d have to deal with config syntax idiosyncracies, and we’d still need a privileged script for accessing the file. In regards to (b), I think it might depend a bit how tight we want to couple uStreamer and Janus on our end. (b.1) would couple tighter but would also better ensure “atomicity”, whereas (b.2) keeps things more separate, and is therefore “cleaner” in terms of code structure. I’m not sure (b.3) is an option, since as far as I’m aware the Janus systemd config is currently not in our control. I’d probably tend towards an independent privileged script for simplicity – like (b.2) –, but I also need to look more into it, and get more familiar with the whole topic. If you have thoughts or preferences here, I’d be curious to hear. FrontendI’m wondering whether we’d even need a separate “advanced” view of the video settings, or if we’d get away with an inlined solution. Below a rough mock-up of what would be possible – just as a starting point for further exploration. Would something like this be an option for us worth to consider, or would you prefer to stash the STUN-related things away in a separate place? (As they might be a bit more “special” compared to the other settings.) Screen.Recording.2023-07-07.at.17.15.01.movThe suggested dropdown list is natively supported by browsers, it might also have multiple entries. I’m wondering, however, whether it’s a good idea to hard-code external addresses in our UI. They ultimately aren’t in our control and might change over time. In this case, our UI could get stale, or worse, appears to be broken. If we’d be worried about that, we could consider to e.g. either link to a support page on |
Sidenote: We should come up with more general rules about what goes in
Yeah agreed. I lean towards I definitely don't want to keep state in the Janus config file because that feels too messy.
I don't think I have a lot of extra context, but my preference is
Yeah, I'd definitely like to bury them. Probably 80% of users won't have to use them, and I think less than 3% of users will understand what the settings even mean, so I want to avoid throwing confusing settings at people until they ask. It's similar to how we bury the "mount mode" for virtual media:
Let's hardcode it for now for simplicity. If we need something more flexible in the future, we can add a server component, but anytime we bring in client-server interaction, it's like an order of magnitude jump in complexity/maintenance cost. |
Based on our discussion above, I was looking into the UI some more, specifically into the question how we can hide away the STUN settings. Despite the ticket being blocked right now, I’d like to present three possible UI/UX approaches that I drafted – it’s just food for thought for now, we can defer the discussion to when we resume this ticket. Some general notes upfront:
(1) Separate ViewScreen.Recording.2023-07-24.at.17.49.14.movIn this design, we’d transition the entire dialog to a new “state”/view. While this results in a clear visual division of the settings, it also poses some UX challenges – mainly what happens after you click “Save” of “Apply”:
(2) Expand / Collapse new sectionScreen.Recording.2023-07-24.at.17.49.56.movIn this design, we’d only collapse / expand the advanced video settings, where they are just a sub-section of the main video settings. In expanded view, this design looks more crowded, but the advantage is that the control flow in regards to saving/applying is obvious and transparent. (3) Expand / Collapse STUN sectionScreen.Recording.2023-07-24.at.18.30.19.movSimilar to (2), but slightly less crowded in expanded view, would be to use the checkbox as means to toggle the STUN input controls. It’s a bit more clean overall, but the “STUN” topic is also more prominent in collapsed mode. The fact that the STUN config is considered to be an “advanced setting” also wouldn’t be as explicit anymore. I’d prefer (2) or (3), despite the fact that it would make the video settings dialog look quite “comprehensive” in expanded view. (It probably would be our most complex and advanced view in the entire app.) |
(Update: it occurred to me that there is also a third option, which I added above.) |
Please excuse the unprompted feedback! I note that @mtlynch mentioned placing this under the “Video” settings, but pragmatically it’s a network and privacy setting. Would it therefore make more sense to put it below the “Connection” header under “Security” settings instead? |
Cool, these look good. I'd like to move forward with (2). I think (1) is unexpected from a user's perspective since the flow would be different from most of our other dialogs. (3) is pretty similar to (2), but I like the look of (2) better. We also need the dialog to cover a few other things:
I'd also like the Google STUN server to be the default if the user enables STUN rather than something they have to do extra. For probably 95% of users, they'll choose the Google server anyway, so we want to make it easy and obvious. What do you think of instead of a text field, it's primarily a dropdown where the first option is the Google STUN server, then maybe a couple other public ones, and then the last option is "Custom" and we only show a free text field if the user selects "Custom?"
Oh, good point. I still think users will view this as a video settings feature rather than a security feature. It's a video settings feature that has privacy implications, but the reason that users would use this setting is to fix problems with their video rather than to adjust their security. Contrast this with something like user auth where the purpose of the feature is to adjust security rather than to achieve something else that has security implications. |
Cool, thanks for your feedback!
Oh, the mapping somehow fell through the cracks… 🤔 I had one technical question about the STUN<>1:1 duality, which also would have UI implications. Apart from that, I saw that the Janus I’m not sure how common this scenario is (this is the original PR, for context), but I was wondering whether we should draw a line somewhere in regards to complexity on our end, or whether we should go all in with the flexibility that Janus provides here.
That sounds good! |
I’ve continued to work on this, and wanted to discuss a few things about the WIP state. UI iterationScreen.Recording.2023-08-16.at.18.11.20.mov
ComplexityI realized that the tree of possible config states and choices has quite some complexity to it, and I think it’s tricky to model this in the UI in a way that is neither overwhelming nor bouncy / dynamic. The logical structure is this right now, where all nodes are conceptually different and mutually exclusive:
Implementation-wise, I’d anticipate that most effort and code complexity will be on the UI side. I think we should try to keep things tidy, e.g. by putting the advanced settings section into its own component, separate from the In any event, I wanted to double-check that we are still on track with our original estimate, and the amount of work/time we want to invest into this feature. Potential alternatives could include:
Writing the Janus configsSince #1496 has landed, we can move forward with the backend side of things. I have created a minimal implementation, but unfortunately I ran into an issue. The
This creates two problems:
For (a), we could change from |
Cool, this is looking good so far!
Yeah, I'd like to abstract away the
These seem trustworthy:
Writing the Janus configs
I'm confused by this part. |
GMX has been around for ages in Germany, and they are primarily known for their email offering (
Yeah, me as well 😆 I think I got tripped up by the whole initial confusion stemming from #1571, and then I probably misinterpreted what I saw, or forgot to run the script as |
I submitted a patch for #1571 (#1572), since this issue here is blocked on the fix. (We have to run the |
As discussed with @mtlynch in our video call today, we restrict the scope of this issue to STUN-only in the first step. We can add support for 1:1 mapping later. This procedure allows us to iterate in smaller chunks, and the STUN feature brings most value. |
I’ve created #1577 to track the 1:1 mapping functionality separately. |
With the elimination of 1:1 mapping option, I think we can get rid of the "Connection method" dropdown and just have a single dropdown in the Advanced section called "STUN Server" where the options are:
|
Part of #1460. This PR templatizes the Janus main config file, and adds a privileged script for (re-)generating the config from the template plus from the properties in `settings.yml` (i.e., `ustreamer_janus_stun_server` and `ustreamer_janus_stun_port`). ## Notes - In the Debian `postinst` routine, we deliberately delegate to a privileged script instead of just inlining the logic ([like we do for writing the app settings](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/debian-pkg/debian/tinypilot.postinst#L73-L80), for example), since we want to re-use the `configure-janus` logic in the backend in a later step. - Later, when calling the `configure-janus` privileged script from the backend, we also need to restart the uStreamer and Janus systemd services manually. To me, it felt safer and more explicit to let the caller take care of that, instead of making the service restart part of `configure-janus`. I also wasn’t sure we should be issuing direct calls to `systemctl` or `/usr/sbin/service` anyways during the Debian install procedure, instead of using `deb-systemd-invoke`. - I debated whether we should add a comment above the `nat {}` block in the Janus config, however, I wasn’t sure whether a generic explanation of what STUN is would add enough value. - Since neither `ustreamer_janus_stun_server` nor `ustreamer_janus_stun_port` do have default values, it doesn’t make sense to list them [in `settings.py`](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/app/update/settings.py#L24-L31). We currently don’t have an authoritative place anymore that lists and documents all supported properties in `settings.yml`, so the two newly added STUN properties are implicit and undocumented now. I have opened #1573 for tracking that, since I think this is a broader issue. A few more things on testing: - I have tested this PR quite a bit on device with the [latest bundle build off `af5ec0d`](https://output.circle-artifacts.com/output/job/29dc16fa-0b35-4b41-b920-85e61fa8883f/artifacts/0/bundler/dist/tinypilot-community-20230822T1618Z-1.9.0-66+af5ec0d.tgz), so if you like, I’d suggest that a brief spot-check QA on your end would suffice. - Please note that testing this PR is only about verifying that we generate a valid Janus config [according to the documentation](https://github.com/meetecho/janus-gateway/blob/7c7093e1885834ac4154244bd99f8dd93ae71170/conf/janus.jcfg.sample.in#L259-L288), not about actually verifying the STUN mechanism itself, as the setup for that [is quite complex and laborious](tiny-pilot/tinypilotkvm.com#939 (comment)). We already gathered enough evidence that Janus with STUN in itself does the job, and solves the use-case scenarios we are after. - We [have an issue](#1578) where Janus would fail to come up right after booting, if a STUN server is specified. We’ll fix this separately. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1579"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
Just a quick update here, as “fyi”: I’ve been working a branch for the backend part (which is not completely finished yet, though), and also started to work on a frontend branch. I’ll stack the latter onto the former, and then plan to open both PRs at the same time, so that we can review/test/QA the feature in its entirety. For the frontend, we might want to consider some refactoring in the |
@mtlynch could you briefly look over the following demo video regarding the frontend? Screen.Recording.2023-09-28.at.16.43.33.movA few notes/questions from my side:
|
Cool, I think this looks really good! It's slightly jarring that the control changes when the user selects "Custom," since I think it violates expectations a little of how dropdowns work. The alternative is that picking "Custom" keeps the dropdown as-is, but it makes a new textbox appear, and that might be more confusing. I don't think it's a huge deal, and it might be the best option we have, but it did throw me a little bit.
Sure, here's my suggestion for the text: <p>A STUN server enables H.264 streaming when your web browser is unable to make a direct network connection to your TinyPilot device.</p>
<a href="https://tinypilotkvm.com/faq/stun-server">Do I need a STUN server?</a> It oversimplifies things a bit, but we can go into more detail in the FAQ.
I've created a ticket for a new one: https://github.com/tiny-pilot/tinypilotkvm.com/issues/1053
My thinking is that each one appeals to a slightly different type of user:
Maybe we can cut out WHOI since it's less recognizable than UCSB but serves the same purpose? I guess the hypothetical user who's skeptical of Google but supports Barracuda is probably pretty rare too. Let's cut out WHOI and Barracuda. |
I’m close to finishing the STUN feature, and just set up 3 PRs which are almost ready-to-go:
Unfortunately, there are still some minor issues… Public STUN serversI could have noticed that earlier, but it appears that the aforementioned list of “public” STUN servers isn’t up-to-date anymore. Neither UCSB, WHOI, nor Barracuda appear to have a functioning STUN server anymore, at least I wasn’t able to connect to them. I found another curated list of “public” STUN servers (see the I deliberately put “public” in quotes, because I came to realize that these STUN servers are not necessarily intended for usage by the “general public”. My impression is rather that some random people just gather random STUN servers that they happen to find somewhere, and compile them into these lists. The respective STUN providers, however, might not even be aware that their STUN server is used by random people on the internet. There is hence not really any reliability guarantee, and it’s also debatable whether it’s okay to just use and promote these servers only because they are publicly accessible. So I think we might want to reconsider which “public” STUN servers we want to put into our UI. Google and GMX seem to be somewhat reliable and okay-to-use, but other than those, I’m not sure there are viable candidates? We can also research this topic further, and/or back-paddle with the 3rd-party STUN servers for now? Systemd restart on failure→ #1648 With this issue, it might happen that the user enters a “stuck” state, where (once the Janus service enters |
Just discussed with @mtlynch in person:
|
Related #1460. This PR adds the backend validation logic for processing the user-provided input values for the STUN server (server + port). These values will later [come from the UI](#1647). ## Notes - I added comments in the `janus.jcfg` file to shed some more light on how the config parameters work. - Based on these rules, I implemented the request validators. - I created a unified, public `parse_h264_stun_address` validation, to enforce that they are either both present or both absent. - The `_SERVER_PATTERN` is intentionally broad, I’m not sure it would be worth to have a stricter validation here. (At least I couldn’t find any reasonably simple way, without either making the regex more complex, or pulling in an external library.) For our purposes, it’s probably enough to provide a sanity check, rather than a bullet-proof validation. - The IP address validation can be done conveniently via the `ipaddress` native Python package. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1645"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
Related #1460. Stacked on #1645. This PR processes the STUN values in the API, and applies them to Janus. For testing, it’s probably most convenient to use the [next (and last) PR in the stack](#1647). ## Notes - The placeholders in `controllers.js` are only for making this PR self-contained, because the backend would now technically enforce those two fields being present. - Note that I ran into #1648 while testing. We’ll tackle that separately – it’s slightly edge-casey, and we decided [it’s non-blocking for now](#1460 (comment)).
The implementation work for integrating STUN into TinyPilot is almost done, so we have everything in place now for the user to specify a STUN server in the video settings dialog. The UI is not yet released, though, so the feature is not available to end-users. While we have tested everything inside the TinyPilot system (e.g., that our Python backend correctly applies the STUN parameters to the Janus config), we weren’t able to verify the external communication with the STUN server, and that our implementation is really fixing H.264 streaming in a scenario where STUN is actually required. You know how to create a network setup where STUN is required for H.264 video streaming, right? Would you be able to carry out the following check on your end, to verify that our STUN implementation works “in the wild”? The testing procedure would be this:
You wouldn’t need to check anything else than that, as all pending changes have already passed code review and general QA. |
@jotaen4tinypilot - Sure thing, I'd be happy to get this tested! For reference, you can test STUN without needing a complex network if you have two Internet connections with a public IPv4 address each. It should be as simple as port forwarding a TinyPilot device on one connection and then accessing it using the other. The H.264 connection should fail at first but work after enabling STUN. This scenario is a typical example of what a customer might face, so it's a good test to try. @mtlynch - I don't have access to a second connection, so we used a temporary cloud server as a relay last time. I looked at the Exploding Servers tool, but I don't think the provisioned servers have any ports open to the Internet other than SSH. Can you please set the cloud server back up so I can test this? |
@cghague - This is the type of scenario I built Exploding Servers to address, so it'd be good if we can make the tool work here so that you and Jan can self-provision. Are there specific ports that needs to be exposed, or do all the ports need to be exposed? |
@cghague - From my experiments, servers on Exploding Servers already have all ports open. I just tried this: sudo apt update && sudo apt install -y ncat Then I tried listening on 8585 and then hitting the IP+ port from my browser, and the request came through fine:
Where did port listening fail for you? |
Thanks @mtlynch, I must admit I didn't check if the ports were open, as it's standard for cloud servers to require users to allow ports explicitly, and I expected the cloud servers created by Exploding Servers would be the same. I couldn't see any options to allow ports in the web interface, so I assumed it still needed to be implemented. @jotaen4tinypilot - I started working on this but realized that, as the bundle isn't for TinyPilot Pro, it doesn't support user authentication or HTTP encryption. I'm not comfortable exposing devices on my home network directly to the Internet without those features, so could you please provide a TinyPilot Pro build of this for me to test? |
@cghague - Can you try generating the Pro bundle? Jan provided feature branch. If you merge those changes into a branch on Pro, it will generate a Pro bundle for you. Keep in mind that we can't link a Pro bundle/build in this thread, as it's public. |
Thanks @mtlynch. I've tried that and successfully merged these changes into a test branch on tinypilot-pro. I've given it a very quick smoke test - it appears to work, and the STUN changes are present. I should be able to test STUN in full tomorrow - I'll let you both know how it goes! |
I've tested this, and the results are as follows:
I can see in the service logs that Janus is correctly using both the GMX STUN server and the STUN server provided by my ISP when selected. They return the correct IP address when queried during the Janus startup procedure. This result suggests that the server side is working correctly. However, the client fails to identify the correct candidate using anything other than the Google STUN servers. I'm unsure why this is, and there's nothing helpful in Is there anything else I can test here? |
@cghague - The client and server can use separate STUN servers, right? I'm wondering if there's some kind of client bug that accidentally pins to Google even if we specify a different server, but that wouldn't explain why it breaks if the server uses a different STUN server. @jotaen4tinypilot - Any ideas? |
@cghague thanks for testing and providing such a detailed report! Are you sure that your ISP’s STUN server is usually working, though? We had already discovered earlier that only because a STUN server is advertised somewhere it doesn’t mean that it is also functioning correctly, or generally available at all. The same might be the case for GMX – we don’t have any definite verification so far that GMX’s STUN server is usable for our purposes, except that the server is reachable. Could you do me a favour and give the following STUN servers a try? According to this random STUN connection testing website that I found, they seem promising.
Should these fail, could you also double-check in a private browser window? Just so that we can rule out any client-side quirks, such as caching. Thoughts on further investigationI’d think it would be worthwhile to wait whether any of the 3 aforementioned alternatives will be successful. My level of trust in regards to publicly available STUN servers is slightly damaged, based on our earlier findings about their availability or reliability. Another factor is that there are two RFCs for the STUN protocol (3489 and 5389), and from what I’ve read, a STUN connection can also fail if the server still uses the old RFC while the client uses the new one. If it continues to only work with Google, then I think we should investigate this more deeply. It might then probably make sense for me to produce a STUN scenario on my end. Otherwise/additionally, it might also be an option for me and @cghague to connect for a debugging session. Regarding the client library: Google’s STUN server indeed sticks out strangely right now, but I think we should also gather more data points before making conclusions. In any event, in the Janus client JS library, |
I carried on looking into this after my last comment. I had many of the same thoughts as @jotaen4tinypilot, and I've ruled out several possible problems and made a small amount of progress. I started by setting a few breakpoints using the debugger in the browser's developer tools, and the correct value appears to be getting set and propagated correctly. For a belt-and-braces approach, I also tried intentionally mangling the default value in My ISP markets itself at technical users (they're genuinely excellent and are even "xkcd/806" compliant), so I'd be surprised if their STUN server didn't work. However, to be safe, I verified all of the STUN servers I used with the I finally tried connecting from Safari on my iPhone, and it did work with the STUN server provided by my ISP. My iPhone uses the same Internet connection as my desktop, so I have no explanation for the different outcomes. I tested Safari on my desktop to rule out a browser issue, and it behaved identically to Firefox (which I used for the earlier tests due to the excellent The above findings suggest that STUN makes a difference and that the changes made by @jotaen4tinypilot work and are configuring Janus correctly. Consequently, I speculate that the failures are likely due to the testing environment we're using. I'll try a few more things tomorrow, including the STUN servers @jotaen4tinypilot suggested. |
@cghague Thanks for looking into this so deeply!
Just wondering, did you get a chance already to try the other STUN servers? In general, I’m thinking that if we have evidence that the STUN feature does the job even with custom servers, then we should be good to proceed with releasing this. I’m sure there are a few things left for us to be learned about the whole STUN topic, though, so maybe/hopfully we are be able to keep refining the feature as needed. |
Thanks @jotaen4tinypilot! I tested the alternate servers, but initially, none worked, so I dug into this further. I tried:
During these tests, I observed the Based on my findings, we can deduce that the issue is affecting the client side of the connection, but we don't have enough information to say why. It's plausible that the complex testing environment required to force STUN usage on my home network is causing problems. I'd suggest that our next step would be to try and arrange a less complicated STUN testing environment. The ideal would be for us to implement the following basic scenario:
I don't have access to two suitable connections, so this isn't something I can try at home. Do either of you have the resources to attempt this? |
Oh ok, interesting findings… 🤔 I’ll try to set up port forwarding in my home network, and then try to access my device through a remote connection (e.g., my mobile carrier). Maybe/Hopefully that works out 🤞 |
I’m afraid to report that I wasn’t able to produce a network setup on my end where my device is publicly exposed to the internet. I couldn’t figure out what the problem is exactly, but I wasn’t able to get my router to forward the ports of my device, and to exempt them from the firewall. So when I tried reaching my device (connected to my home network) at it’s IP address via my mobile carrier’s network, it couldn’t establish a connection. The router which I own is an average consumer model provided by my ISP, so it’s nothing advanced, and it also only offers limited customization options. I’m therefore not sure it’s worth to pursue this route any further, as I think I’ve exhausted all config options and combinations I could think of. I’m wondering how best to proceed with this – current thoughts:
|
My take on this is that in scenarios where H.264 already works, it will continue to do so regardless of whether STUN is enabled, and in situations where H.264 doesn't currently work, enabling STUN will either fix the problem or have no effect. It therefore follows that there is no downside to providing the STUN functionality. We should, however, continue investigating this to understand why some servers don't work, as that will allow us to provide better customer support. |
Yeah, I think we should move forward with the current implementation. It seems strange that there would be something specific about Janus' client implementation that would make Google the only valid STUN server. Given the cost of testing the STUN setup and the possibility that the effect we're seeing is just testing error, I think the current implementation is worth moving forward with. @cghague - Can you share more detail about the debugging steps you took in case we want to re-do that in the future? Was it all through
|
I primarily based my investigations on the output of
From the
In terms of tweaking the behavior of Janus, I made all changes in Regarding Firefox, the browser allows you to override many STUN/TURN settings in
Any active overrides also conveniently show up in |
@cghague - Thanks, that's helpful! |
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.
Resolves #1460. Stacked on #1646, blocked on #1657 and tiny-pilot/tinypilotkvm.com#1053. This PR adds the UI for specifying a STUN server, and eventually makes the feature available to the end-user. → [Latest bundle off this branch](https://output.circle-artifacts.com/output/job/f2379490-a459-43d5-b1fb-ce009c971fcd/artifacts/0/bundler/dist/tinypilot-community-20231005T1841Z-1.9.1-27+3ff1062.tgz), for testing the entire PR stack. ## Demo https://github.com/tiny-pilot/tinypilot/assets/83721279/8108dcf2-1fe4-42d1-8c2a-4066cc669ad0 ## Notes - In contrast to the mockups in the ticket (e.g. [the latest one](#1460 (comment))), the proposed implementation here has a separate field for the host part and the port part of the STUN address. I realized that this simplifies a lot of things, because we don’t need to parse and split a unified address string (e.g. `stun.example.org:3478`), or serialize it again correctly. That would be especially tricky, since Janus supports IPv6, where the serialized format would look like e.g. `[12:c1:1832::c1:2]:3478`, so we can’t just naively split and join at the `:` character. - I separated the STUN input part into its own component, otherwise the `<video-settings-dialog>` would have become pretty large. - I debated whether to split off the entire “Advanced Settings” section, or just the STUN input. I eventually settled with the latter, because I felt it was conceptually more fitting, and I thought it would be the more granular approach, e.g. should we add more controls to the advanced settings in the future. I’m not married to it, though – I also think the entire `<video-settings-dialog>` component is borderline big anyway, so should we ever expand it further, it would probably be worth to consider a refactoring. - I had to [refactor (reverse) the CSS logic for showing/hiding the `.setting-...` classes](https://github.com/tiny-pilot/tinypilot/blob/e305ca104b54e485ee4e32876f1f8d181103b810/app/templates/custom-elements/video-settings-dialog.html#L30-L38). With the current implementation, it only allowed for `display: flex`, but with the new code, we also need `display: block` (on `.advanced-settings`). - Note that the `#stun-validation-error` is still part of `<video-settings-dialog>`, since validation errors are supposed to be “dialog-wide” errors, displayed at the bottom of the dialog, near the call-to-action button. If there were more inline / validation errors in the future, we’d also add them there, at top level. - One note about terminology: in the Janus config, the two address components are called [`server` and `port`](https://github.com/tiny-pilot/tinypilot/blob/0a03d2caf54aa083ef66c1606e28321f41aca781/debian-pkg/usr/share/tinypilot/templates/janus.jcfg.j2#L24-L25). I took over that terminology in the entire code, but in the UI I labelled the `server` fragment “Host”. To me, that’s technically more accurate, and since we already say “STUN Server” next to the dropdown button already, I thought “Server” might be confusing. I’m not sure there is a “right” answer here. We could otherwise also call it “Address”, but I feel that’s even broader. - The UI implementation has a few edge-cases with slightly unexpected/quirky behavior. We’d need to pay with more code and complexity to sort them out, though, and I felt that wouldn’t be worth it. Examples: - If you select “Custom” in the dropdown and then manually fill in the Google server details (`stun.l.google.com` + `19302`), then it would display the “Google” option the next time you open the dialog, and not the custom input fields. That’s because both ways look the same internally, and we’d need to maintain extra state to distinguish whether this was a custom input or one of the predefined selections. - If you select “Custom” and enter an invalid or incomplete address, and then choose “MJPEG” as streaming mode again, and then hit the “Apply” button, it still displays the STUN validation error. The reason is that the entire code around processing video settings is designed to always send and apply **all** values, regardless of what mode you eventually chose. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1647"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
Blocked on #1496There are a few common scenarios where the browser and the Janus server on the TinyPilot device can't establish a WebRTC connection. See @cghague's investigations for details (especially the Conclusions note at the end):
https://github.com/tiny-pilot/tinypilotkvm.com/issues/939#issuecomment-1600620310
We should add an "Advanced" section to the Video Settings page that allows the user to configure a STUN server
or 1:1 mappingfor Janus to use in the WebRTC connection. For privacy, I'd like the STUN server to be off by default, but if they enable it, we should default to a Google STUN server but allow the user to enter any STUN server they choose.UPDATE (2023-08-21, @jotaen4tinypilot): Initially, we wanted to implement both STUN and 1:1 mapping. However, we decided to restrict the work to STUN in the first step, and push 1:1 mapping support down the agenda.
The text was updated successfully, but these errors were encountered: