Skip to content

Commit

Permalink
Use uStreamer launcher. (#1252)
Browse files Browse the repository at this point in the history
Resolves #1249

Now that the [uStreamer launcher
script](tiny-pilot/ansible-role-ustreamer#89)
dynamically determines the correct uStreamer command-line options, we
don't need to run our Ansible playbook to apply video settings.

This PR removes the use of the `update-video-settings` script and uses
the [backported `video-service`
module](#1251) to apply the
current video settings found in `/home/tinypilot/settings.yml`.

### Notes

1. This PR is based on @mtlynch's [uStreamer launcher proof of concept
PR](tiny-pilot/tinypilot-pro#701) and resolves
the following missing parts:
> - Add JS logic to make sure that uStreamer/Janus is up and serving
again before reloading the page
> - If we reload too early, we'll get an HTTP 502 error because
uStreamer isn't listening for inbound connections yet. I've seen this in
MJPEG and not in H264 mode, but I'm not sure whether it could happen in
H264 as well.
    
To ensure that the MJPEG stream is available, I
[poll](https://github.com/tiny-pilot/tinypilot/blob/54b6abfa03b2ae2e055261659b0e4f160af60fba/app/templates/custom-elements/video-settings-dialog.html#L450-L456)
the [`/stream` endpoint until it gives us a `200
OK`](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/static/js/controllers.js#L314-L326).
   
As for the H264 stream, our connection to Janus does disconnect once the
new video settings are applied (because [Janus is
restarted](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/video_service.py#L19)),
but once we [reload the
page](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/templates/custom-elements/video-settings-dialog.html#L464)
Janus is already back online. I also didn't manage to get Janus to
return an error after the page has been reloaded. I do think it's still
technically possible to get an error in H264 mode, but rather unlikely.
If we did want to eliminate any possibility of the error, we would have
to implement "automatic reconnection" logic in `webrtc-video.js`. Some
investigation reveals that we just need to [recreate the `Janus`
instance](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/static/js/webrtc-video.js#L44-L56)
when the [client is
disconnected](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/static/js/webrtc-video.js#L48-L55).
The `janus.js` library does have a [`recover`
method](https://github.com/meetecho/janus-gateway/blob/v0.9.2/html/janus.js#L531)
to reconnect/reclaim to an existing session, but this doesn't work if
the Janus service has been restarted because Janus forgets all sessions
when restarted.

    > - Find a way to make video settings work in dev mode
> - It used to work through the mock script, but now we're no longer
calling an external script, so we need another solution.
> - Maybe just check the debug mode [in
video_settings.apply](https://github.com/tiny-pilot/tinypilot-pro/blob/1a8bf29292b0075e362711148645fa42a981bea8/app/video_settings.py#L18)?

This is no longer an issue because applying video settings now just
means restarting uStreamer/Janus, and those video services are restarted
in a best effort manner (i.e., [we ignore any errors that might
occur](https://github.com/tiny-pilot/tinypilot/blob/378f6919f1c4c347517ac7a39e8a2b8c8f6c6899/app/video_service.py#L34))
and only log any potential errors. So in dev mode, the command would
silently fail and the error is logged as:

    ```
2023-01-12 16:27:15.361 video_service ERROR Failed to restart ustreamer:
Command '['sudo', '/usr/sbin/service', 'ustreamer', 'restart']' returned
non-zero exit status 1.
2023-01-12 16:29:16.778 video_service ERROR Failed to restart janus:
Command '['sudo', '/usr/sbin/service', 'janus', 'restart']' returned
non-zero exit status 1.
    ```

> - Dump uStreamer settings in log collection script instead of [systemd
settings](https://github.com/tiny-pilot/tinypilot/blob/1dff200fa1915f3a37bccf7d09b67a4387892b45/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs#L140)
> - Now the systemd settings are going to be uninteresting. In
collect-debug-logs, we should be dumping the contents of files in
`/opt/ustreamer-launcher/configs.d/`


[Done](https://github.com/tiny-pilot/tinypilot/blob/92f2dac143824abdee3dd6c05419f94075b654e3/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs#L140).
As @mtlynch
[mentioned](#1252 (comment)),
we just want to log the filenames and contents of each uStreamer config
file used. For example:
    ```
    uStreamer configuration
    ==> /opt/ustreamer-launcher/configs.d/000-defaults.yml <==
    ---
    ustreamer_encoder: hw
    ustreamer_format: jpeg
    ustreamer_h264_sink: tinypilot::ustreamer::h264
    ustreamer_h264_sink_mode: 777
    ustreamer_h264_sink_rm: true
    ustreamer_interface: 127.0.0.1
    ustreamer_persistent: true
    ustreamer_port: 8001
    ustreamer_resolution: 1920x1080
    ustreamer_desired_fps: 30
    
    ==> /opt/ustreamer-launcher/configs.d/100-tinypilot.yml <==
    ustreamer_desired_fps: 26
    ustreamer_encoder: hw
    ustreamer_format: jpeg
    ustreamer_persistent: true
    ustreamer_port: 8001
    ustreamer_resolution: 1920x1080
    ```

3. Now that we use
[`video_service.restart`](https://github.com/tiny-pilot/tinypilot/blob/68db1dde24a71a1e2a948e0f06a9dd093fde9637/app/api.py#L315)
to apply video settings, our [video settings module looks kind of
awkward](https://github.com/tiny-pilot/tinypilot/blob/68db1dde24a71a1e2a948e0f06a9dd093fde9637/app/video_settings.py).
However, it still made sense to me to keep the default video settings
there.
4. We needed to make [`/home/tinypilot/settings.yml` world readable
(`644`)](https://github.com/tiny-pilot/tinypilot-pro/blob/d77720a744fcfb61fd4ce25864a62d204aa3df64/bundler/bundle/install#L129)
so that the uStreamer launcher could access the file via the newly
created [`100-tinypilot.yml` symbolic
link](https://github.com/tiny-pilot/tinypilot/blob/68db1dde24a71a1e2a948e0f06a9dd093fde9637/debian-pkg/debian/tinypilot.postinst#L13)

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1252"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
  • Loading branch information
jdeanwallace authored Jan 13, 2023
1 parent 98f29a1 commit f902ccb
Show file tree
Hide file tree
Showing 10 changed files with 39 additions and 132 deletions.
9 changes: 4 additions & 5 deletions app/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import update.settings
import update.status
import version
import video_service
import video_settings

api_blueprint = flask.Blueprint('api', __name__, url_prefix='/api')
Expand Down Expand Up @@ -309,10 +310,8 @@ def settings_video_apply_post():
"""Applies the current video settings found in the settings file.
Returns:
Empty response on success, error object otherwise.
Empty response.
"""
try:
video_settings.apply()
except video_settings.Error as e:
return json_response.error(e), 500
video_service.restart()

return json_response.success()
14 changes: 14 additions & 0 deletions app/static/js/controllers.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,17 @@ export async function getLicensingMetadata() {
redirect: "error",
}).then(processJsonResponse);
}

export async function isMjpegStreamAvailable() {
try {
const response = await fetch("/stream", {
method: "HEAD",
mode: "same-origin",
cache: "no-cache",
redirect: "error",
});
return response.ok;
} catch (error) {
return false;
}
}
9 changes: 9 additions & 0 deletions app/templates/custom-elements/video-settings-dialog.html
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ <h3>Applying Video Settings</h3>
getVideoSettings,
saveVideoSettings,
applyVideoSettings,
isMjpegStreamAvailable,
} from "/js/controllers.js";
import { poll } from "/js/poll.js";

(function () {
const template = document.querySelector("#video-settings-template");
Expand Down Expand Up @@ -445,6 +447,13 @@ <h3>Applying Video Settings</h3>
h264Bitrate: this._getH264Bitrate(),
})
.then(applyVideoSettings)
.then(() =>
poll({
fn: isMjpegStreamAvailable,
validate: (isAvailable) => isAvailable,
interval: 1000,
})
)
.then(() => {
// Note: After the video stream stops, it doesn't try to
// reconnect. So in order to restart the video stream, we need to
Expand Down
39 changes: 0 additions & 39 deletions app/video_settings.py
Original file line number Diff line number Diff line change
@@ -1,42 +1,3 @@
import logging
import subprocess

_UPDATE_SCRIPT_PATH = '/opt/tinypilot-privileged/scripts/update-video-settings'
DEFAULT_FRAME_RATE = 30
DEFAULT_MJPEG_QUALITY = 80
DEFAULT_H264_BITRATE = 5000
logger = logging.getLogger(__name__)


class Error(Exception):
pass


class VideoSettingsUpdateError(Error):
pass


def apply():
"""Apply the current video settings found in the settings file.
This runs the ustreamer ansible role using the systemd-config tag.
Args:
None
Returns:
A string consisting of the stdout and stderr output from the
update-video-settings script.
Raises:
VideoSettingsUpdateError: If the script exits with a non-zero exit code.
"""
logger.info('Running update-video-settings')
try:
output = subprocess.check_output(['sudo', _UPDATE_SCRIPT_PATH],
stderr=subprocess.STDOUT,
universal_newlines=True)
except subprocess.CalledProcessError as e:
raise VideoSettingsUpdateError(str(e.output).strip()) from e
logger.info('update-video-settings completed successfully')
return output
1 change: 1 addition & 0 deletions bundler/bundle/install
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,4 @@ ansible-playbook \
# Persist installation settings.
cp "${INSTALL_SETTINGS_FILE}" "${TINYPILOT_SETTINGS_FILE}"
chown tinypilot:tinypilot "${TINYPILOT_SETTINGS_FILE}"
chmod 0644 "${TINYPILOT_SETTINGS_FILE}"
10 changes: 10 additions & 0 deletions debian-pkg/debian/tinypilot.postinst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ set -u
readonly TINYPILOT_USER="tinypilot"
readonly TINYPILOT_GROUP="tinypilot"
readonly TINYPILOT_HOME_DIR="/home/${TINYPILOT_USER}"
readonly TINYPILOT_SETTINGS_FILE="${TINYPILOT_HOME_DIR}/settings.yml"
readonly USTREAMER_CONFIG_FILE="/opt/ustreamer-launcher/configs.d/100-tinypilot.yml"

# Create tinypilot group if it doesn't already exist.
getent group "${TINYPILOT_GROUP}" > /dev/null || \
Expand All @@ -25,4 +27,12 @@ adduser \

chown -R "${TINYPILOT_USER}:${TINYPILOT_GROUP}" /opt/tinypilot

# Use TinyPilot's settings to override uStreamer's runtime variables.
if [[ ! -L "${USTREAMER_CONFIG_FILE}" ]]; then
ln \
--symbolic \
"${TINYPILOT_SETTINGS_FILE}" \
"${USTREAMER_CONFIG_FILE}"
fi

#DEBHELPER#
1 change: 0 additions & 1 deletion debian-pkg/etc/sudoers.d/tinypilot
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/change-hostname
tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/collect-debug-logs
tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/read-update-log
tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/update
tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/update-video-settings
tinypilot ALL=(ALL) NOPASSWD: /sbin/shutdown
tinypilot ALL=(ALL) NOPASSWD: /usr/sbin/service janus restart
tinypilot ALL=(ALL) NOPASSWD: /usr/sbin/service tinypilot-updater start
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ print_info "Checking TinyPilot updater logs..."
print_info "Checking uStreamer configuration..."
{
printf "uStreamer configuration\n"
cat /lib/systemd/system/ustreamer.service
tail --lines +1 /opt/ustreamer-launcher/configs.d/*
printf "\n"
} >> "${LOG_FILE}"

Expand Down
77 changes: 0 additions & 77 deletions debian-pkg/opt/tinypilot-privileged/scripts/update-video-settings

This file was deleted.

9 changes: 0 additions & 9 deletions dev-scripts/mock-scripts/update-video-settings

This file was deleted.

0 comments on commit f902ccb

Please sign in to comment.