-
Notifications
You must be signed in to change notification settings - Fork 13
Run uStreamer via a launcher #89
Conversation
We still need them for backwards compatibility
Automated comment from CodeApprove ➜⏳ @jdeanwallace please review this Pull Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
In: files/launch:
> Line 55
yaml_value="$(cat /opt/ustreamer-launcher/configs.d/* | yq "${yaml_path}")"
I'm confused about how this bash command works. I did some local testing with multiple yaml files and yq
doesn't merge the yaml files by default:
$ ls /tmp/*.yml
/tmp/a.yml /tmp/b.yml
$ cat /tmp/*.yml
---
ustreamer_persistent: yes
---
ustreamer_persistent: off
$ cat /tmp/*.yml | yq '.ustreamer_persistent'
yes
---
off
I followed these yq
docs to merge all the yaml files together:
$ cat /tmp/*.yml | yq eval-all '. as $item ireduce ({}; . * $item ) | .ustreamer_persistent'
off
In: files/launch:
> Line 111
/opt/ustreamer/ustreamer "${USTREAMER_ARGS[@]}"
Can we use exec
here so that it replaces the current process?
In: tasks/install_launcher.yml:
> Line 9
ustreamer_yq_architecture: "{% if ansible_architecture == 'x86_64' %}amd64{% elif ansible_architecture == 'armv7l' %}arm{% elif ansible_architecture == 'aarch64' %}arm64{% else %}{{ ansible_architecture }}{% endif %}"
(optional) To avoid the if-else statements, we could also define the architecture mapping as a dictionary. For example:
# This can be defined as a fact or as a default value in `defaults/main.yml`
- name: define ansible to yq architecture mapping
set_fact:
ustreamer_yq_arch_map:
x86_64: amd64
armv7l: arm
aarch64: arm64
# This can be defined as a fact or as a default value in `defaults/main.yml`
- name: canonicalize yq binary architecture
set_fact:
ustreamer_yq_arch: "{{ ustreamer_yq_arch_map[ansible_architecture] | default(ansible_architecture) }}"
In: tasks/install_launcher.yml:
> Line 26
- name: create uStreamer launcher configs folder
(optional) We could also collapse the creation of the above 2 directories into a single task using loop
. For example:
- name: create uStreamer launcher directories
template:
path: "{{ item }}"
state: directory
owner: "{{ ustreamer_user }}"
group: "{{ ustreamer_group }}"
loop:
- "{{ ustreamer_launcher_dir }}"
- "{{ ustreamer_launcher_configs_dir }}"
In: vars/main.yml:
> Line 15
ustreamer_launcher_script: /opt/ustreamer-launcher/launch
Can we use the ustreamer_launcher_dir
variable to build the path? Something like:
ustreamer_launcher_script: "{{ ustreamer_launcher_dir }}/launch"
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
In: files/launch:
Whoops, that's embarrassing. Thanks for catching that!
I tested for this issue and found a similar solution, but I must have just applied it on the command-line during testing and forgot to integrate it back into the script.
I blame it on holiday brain.
In: files/launch:
Sure, that sounds fine. What's the advantage in this case? I get the idea of replacing the process as opposed to starting a subshell, but I'm not clear on the implications.
In: tasks/install_launcher.yml:
Oh, nice! I like that. Fixed.
In: tasks/install_launcher.yml:
Oh, yeah this is so much cleaner. Thanks!
In: vars/main.yml:
Sure, fixed.
👀 @jdeanwallace it's your turn please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved
Looks good and I've tested it on device. Changing video settings is super fast now!
In: files/launch:
From what I understand, using exec
cuts out the middleman process and preserves termination signals which I think is important for systemd to make restart decisions.
In: files/launch:
> Line 45
MERGED_YAML="$(yq eval-all '. as $item ireduce ({}; . * $item )' \
(nit): Can we mark MERGED_YAML
as readonly
after setting its value?
In: files/launch:
> Line 82
[[ "${yaml_value_lowercase}" != 'no' ]]; then
(nit, optional): Would this if statement become clearer if we matched the positive condition? For example:
if [[ "${yaml_value_lowercase}" == 'true' ]] && \
[[ "${yaml_value_lowercase}" == 'on' ]] && \
[[ "${yaml_value_lowercase}" == 'yes' ]] && \
[[ "${yaml_value_lowercase}" == 'y' ]]; then
(Reference: Possible yaml boolean values)
In: tasks/install_launcher.yml:
> Line 79
mode: '0755'
Can we restart uStreamer if the launcher file changes? Something like:
notify: restart uStreamer
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
In: files/launch:
Oh yeah, that's a good idea. I was thinking that anything that isn't false is implicitly true (e.g., ustreamer_persistent: banana
) counts as true, but we don't need to be so permissive, and it's clearer to just allow the explicit things we expect.
In: files/launch:
> Line 45
MERGED_YAML="$(yq eval-all '. as $item ireduce ({}; . * $item )' \
Ah, right. Fixed.
In: tasks/install_launcher.yml:
> Line 81
mode: '0755'
Oh, good point. I added it to the "write uStreamer runtime variables to file" play as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
Part of #1249 This PR contains an exact copy of https://github.com/tiny-pilot/tinypilot-pro/blob/bc0707716f01fc3c5291ec2c320eb44daa97bb41/app/video_service.py Thanks to our [uStreamer launcher script](tiny-pilot/ansible-role-ustreamer#89), we now dynamically determine the correct uStreamer command-line options each time the systemd `ustreamer` service is started. That means we no longer need to run an ansible playbook to apply the video settings sitting in `/home/tinypilot/settings.yml`. Instead, we can just restart `ustreamer.service` to redetermine the correct video options. Seeing as we already have a `video_service` module in TinyPilot Pro that restarts uStreamer & Janus, I thought it would be a good idea to backport `video_service.py` to TinyPilot Community. ### Notes 1. I noticed that we're still referencing a [legacy environment variable: `USE_WEBRTC_REMOTE_SCREEN`](https://github.com/tiny-pilot/tinypilot-pro/blob/bc0707716f01fc3c5291ec2c320eb44daa97bb41/app/video_service.py#L16). I've fixed it in a [follow-up PR](#1255). <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1251"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
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>
This changes the uStreamer installation so that the uStreamer systemd service runs a uStreamer launcher script rather than running uStreamer directly.
The launcher script, at runtime, reads a set of configuration files, translates those configuration files to uStreamer's command-line flags, and launches uStreamer with those flags.
In the previous implementation of this role, clients needed to re-run Ansible in order to change any of uStreamer's settings (e.g., frame rate, quality) because Ansible was responsible for generating the command-line string to launch uStreamer.
In this implementation, clients can change uStreamer's settings without using Ansible. Clients can simply change settings in the uStreamer launcher's config files and restart the uStreamer service. When the service restarts, the uStreamer launcher will pick up the new configuration.
This reduces the time to change video settings by 91%.
See a rough proof of concept in TinyPilot Pro: https://github.com/tiny-pilot/tinypilot-pro/pull/701
Implementation notes
files/launch
deliberately doesn't use Ansible role variables because I want to make it easy to move this script to Debian packages in the future./opt/ustreamer-launcher/configs.d/
is to make it easier to translate the logic to Debian packages in the future./opt/ustreamer-launcher/configs.d/000-defaults.yml
./opt/ustreamer-launcher/configs.d/100-tinypilot.yml
.sites-enabled
so that different Debian packages can own files that affect uStreamer's behavior without colliding on trying to own the same files.debug
play, which we normally don't do, but I think we should take advantage of it more.systemd-config
tags and tracking their removal in Remove systemd-config tags #90Benefits
If we switch TinyPilot over to using this role (which should be pretty straightforward), we get a slew of benefits:
/opt/tinypilot-privileged/scripts/update-video-settings
script pointed to a file in/opt/tinypilot-updater
, which was confusing and brittle.Drawbacks
/home/tinypilot/settings.yml
will appear in both000-defaults.yml
and100-tinypilot.yml
/home/tinypilot/settings.yml
and uses those values in the Ansible execution of the TinyPilot and uStreamer role./home/tinypilot/settings.yml
, they get copied to/opt/ustreamer-launcher/configs.d/000-defaults.yml
whenever the uStreamer role runs./opt/ustreamer-launcher/configs.d/100-tinypilot.yml
to/home/tinypilot/settings.yml
because legacy installations wrote their settings tosettings.yml
, and it's hard to change at this point.100-tinypilot.yml
overrides settings in000-defaults.yml
, but it's still a bit untidy.