Skip to content
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

Remove reference to legacy USE_WEBRTC_REMOTE_SCREEN variable. #1255

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Jan 9, 2023

Part of #1249
Depends on #1251

We no longer use the USE_WEBRTC_REMOTE_SCREEN environment variable and instead we save the current streaming mode to our local database.

This PR removes the use of USE_WEBRTC_REMOTE_SCREEN.

Review on CodeApprove

Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jotaen4tinypilot please review this Pull Request

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a 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 on CodeApprove
✔️ Approved

Oh, cool that you noticed this! This should actually have been part of the upstream merge of #1166, but I apparently forgot that we had another instance of that variable in Pro back in the day.

Fortunately, this didn’t create a security problem here (video_service.py is currently used as part of the process of revoking user credentials in Pro): the env variable only refers to Janus-related logic, not to uStreamer – i.e., the restart of the latter wasn’t affected.


👀 @jdeanwallace it's your turn please take a look

Base automatically changed from video-service to master January 10, 2023 21:00
jdeanwallace added a commit that referenced this pull request Jan 10, 2023
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>
@jdeanwallace jdeanwallace merged commit 7d90d95 into master Jan 10, 2023
@jdeanwallace jdeanwallace deleted the restart-video-service branch January 10, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants