Skip to content

Commit

Permalink
Support for specifying STUN server: frontend (3/3) (#1647)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@jotaen4tinypilot
jotaen4tinypilot authored Oct 31, 2023
1 parent a8428c6 commit 82f37f8
Show file tree
Hide file tree
Showing 3 changed files with 407 additions and 9 deletions.
10 changes: 8 additions & 2 deletions app/static/js/controllers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ export async function getVideoSettings() {
"defaultMjpegQuality",
"h264Bitrate",
"defaultH264Bitrate",
"h264StunServer",
"defaultH264StunServer",
"h264StunPort",
"defaultH264StunPort",
].forEach((field) => {
// eslint-disable-next-line no-prototype-builtins
if (!data.hasOwnProperty(field)) {
Expand All @@ -273,6 +277,8 @@ export async function saveVideoSettings({
frameRate,
mjpegQuality,
h264Bitrate,
h264StunServer,
h264StunPort,
}) {
return fetch("/api/settings/video", {
method: "PUT",
Expand All @@ -288,8 +294,8 @@ export async function saveVideoSettings({
frameRate,
mjpegQuality,
h264Bitrate,
h264StunServer: null, // TODO(jotaen) Remove placeholder once FE is done
h264StunPort: null, // TODO(jotaen) Remove placeholder once FE is done
h264StunServer,
h264StunPort,
}),
}).then(processJsonResponse);
}
Expand Down
127 changes: 120 additions & 7 deletions app/templates/custom-elements/video-settings-dialog.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,11 @@
margin-bottom: 1.5rem;
}

.setting-mjpeg,
.setting-h264 {
:host(:not([streaming-mode="MJPEG"])) .setting-mjpeg,
:host(:not([streaming-mode="H264"])) .setting-h264 {
display: none;
}

:host([streaming-mode="MJPEG"]) .setting-mjpeg,
:host([streaming-mode="H264"]) .setting-h264 {
display: flex;
}

.streaming-mode-value-mjpeg,
.streaming-mode-value-h264 {
display: none;
Expand Down Expand Up @@ -80,6 +75,42 @@
font-size: 0.85em;
opacity: 0.85;
}

.advanced-settings {
display: none;
margin-left: 10%;
margin-right: 10%;
position: relative;
}

:host([show-advanced-h264-settings]) .advanced-settings {
display: block;
}

h4 {
border-bottom: 1px solid #6c6c6c;
padding-bottom: 0.3em;
margin-top: 2em;
}

#advanced-settings-show-button {
font-size: 0.9em;
}

:host([show-advanced-h264-settings]) #advanced-settings-show-button {
display: none;
}

#advanced-settings-hide-button {
position: absolute;
margin-top: 0.1em;
margin-left: 6em;
font-size: 0.9em;
}

#stun-validation-error {
margin-top: 2em;
}
</style>
<div id="loading">
<h3>Retrieving Video Settings</h3>
Expand Down Expand Up @@ -138,6 +169,23 @@ <h3>Change Video Settings</h3>
Reset to Default
</div>
</div>
<div class="setting-h264">
<div id="advanced-settings-show-button" class="btn-text">
Show Advanced Settings
</div>
<div class="advanced-settings">
<span id="advanced-settings-hide-button" class="btn-text"
>(Hide)</span
>
<h4>Advanced Settings</h4>
<video-settings-h264-stun></video-settings-h264-stun>
</div>
</div>
<inline-message variant="error" id="stun-validation-error">
<strong>Invalid STUN address:</strong> the server address must consist
of valid host and port values. The host part can either be a (fully
qualified) domain name, or an IP address.
</inline-message>
</div>
<button class="save-btn btn-success" type="button">Apply</button>
<button class="close-btn" type="button">Cancel</button>
Expand Down Expand Up @@ -219,6 +267,12 @@ <h3>Applying Video Settings</h3>
h264BitrateRestoreButton: this.shadowRoot.querySelector(
"#h264-bitrate-reset"
),
h264StunSettings: this.shadowRoot.querySelector(
"video-settings-h264-stun"
),
h264StunValidationError: this.shadowRoot.querySelector(
"#stun-validation-error"
),
};
this.elements.closeButton.addEventListener("click", () =>
this.dispatchEvent(new DialogClosedEvent())
Expand Down Expand Up @@ -265,6 +319,29 @@ <h3>Applying Video Settings</h3>
this._setH264Bitrate(this._defaultSettings.h264Bitrate);
}
);
this.elements.h264StunSettings.addEventListener(
"h264-stun-value-changed",
() => {
this._refreshButtons();
this.elements.h264StunValidationError.hide();
}
);
this.elements.h264StunSettings.addEventListener(
"h264-stun-submission-requested",
() => {
this.elements.saveButton.click();
}
);
this.shadowRoot
.querySelector("#advanced-settings-show-button")
.addEventListener("click", () => {
this.toggleAttribute("show-advanced-h264-settings", true);
});
this.shadowRoot
.querySelector("#advanced-settings-hide-button")
.addEventListener("click", () => {
this.toggleAttribute("show-advanced-h264-settings", false);
});
}

get state() {
Expand All @@ -282,6 +359,12 @@ <h3>Applying Video Settings</h3>

initialize() {
this.state = this.states.LOADING;

// Reset all transient view state.
this.elements.h264StunValidationError.hide();
this.toggleAttribute("show-advanced-h264-settings", false);

// Fetch and fill in data from server.
getVideoSettings()
.then(
({
Expand All @@ -292,6 +375,10 @@ <h3>Applying Video Settings</h3>
defaultMjpegQuality,
h264Bitrate,
defaultH264Bitrate,
h264StunServer,
defaultH264StunServer,
h264StunPort,
defaultH264StunPort,
}) => {
this._setStreamingMode(streamingMode);
this._initialSettings.streamingMode = streamingMode;
Expand All @@ -308,6 +395,15 @@ <h3>Applying Video Settings</h3>
this._initialSettings.h264Bitrate = h264Bitrate;
this._defaultSettings.h264Bitrate = defaultH264Bitrate;

this.elements.h264StunSettings.initialize(
h264StunServer,
h264StunPort
);
this._initialSettings.h264StunServer = h264StunServer;
this._defaultSettings.h264StunServer = defaultH264StunServer;
this._initialSettings.h264StunPort = h264StunPort;
this._defaultSettings.h264StunPort = defaultH264StunPort;

this._refreshButtons();
this.state = this.states.EDIT;
}
Expand Down Expand Up @@ -399,12 +495,17 @@ <h3>Applying Video Settings</h3>
* the currently selected input values.
*/
_refreshButtons() {
const { h264StunServer, h264StunPort } =
this.elements.h264StunSettings.getValue();

// Save Button: only enable if the user actually changed some value.
const hasChangedValues = [
[this._getStreamingMode(), this._initialSettings.streamingMode],
[this._getFrameRate(), this._initialSettings.frameRate],
[this._getMjpegQuality(), this._initialSettings.mjpegQuality],
[this._getH264Bitrate(), this._initialSettings.h264Bitrate],
[h264StunServer, this._initialSettings.h264StunServer],
[h264StunPort, this._initialSettings.h264StunPort],
].some(([actualValue, initialValue]) => actualValue !== initialValue);
this.elements.saveButton.disabled = !hasChangedValues;

Expand Down Expand Up @@ -436,11 +537,16 @@ <h3>Applying Video Settings</h3>

_saveSettings() {
this.state = this.states.SAVING;

const { h264StunServer, h264StunPort } =
this.elements.h264StunSettings.getValue();
return saveVideoSettings({
streamingMode: this._getStreamingMode(),
frameRate: this._getFrameRate(),
mjpegQuality: this._getMjpegQuality(),
h264Bitrate: this._getH264Bitrate(),
h264StunServer,
h264StunPort,
})
.then(applyVideoSettings)
.then(() =>
Expand All @@ -463,6 +569,13 @@ <h3>Applying Video Settings</h3>
location.reload();
})
.catch((error) => {
if (error.code === "INVALID_STUN_ADDRESS") {
// Display validation errors inline in order to make it more
// convenient for the user to correct them.
this.elements.h264StunValidationError.show();
this.state = this.states.EDIT;
return;
}
this.dispatchEvent(
new DialogFailedEvent({
title: "Failed to Change Video Settings",
Expand Down
Loading

0 comments on commit 82f37f8

Please sign in to comment.