-
Notifications
You must be signed in to change notification settings - Fork 35
Update recording server and headful image #41
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
Conversation
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.
What Changed
This PR focuses on improving the performance and reliability of the recording server. Key changes include updating the chromium-headful image to use a static build of FFmpeg 7.x and reducing the default screen resolution from 1080p to 1024x768 to lower compute requirements. The server configuration has been updated to cap the recording frame rate at 20 FPS. Additionally, the FFmpeg shutdown process was made more robust with longer timeouts, and video encoding arguments (-profile:v high, -pix_fmt yuv420p) were added to improve web compatibility.
Risks / Concerns
A significant security concern was identified in the images/chromium-headful/Dockerfile. The new process for installing FFmpeg downloads a binary from an external source without any integrity verification. As noted in the review on line 96, this could allow for the execution of a malicious binary if the source is compromised. It is highly recommended to add a checksum validation step (e.g., sha256sum) to mitigate this risk.
9 files reviewed | 1 comments | Review on Mesa | Edit Reviewer Settings
| # install ffmpeg manually since the version available in apt is from the 4.x branch due to #drama. | ||
| # as of writing these static builds will be the latest 7.0.x release. | ||
| RUN set -eux; \ | ||
| URL="https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz"; \ |
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.
Security concern: Downloading and executing binaries from an external source without integrity verification. Consider adding checksum validation or using a specific versioned URL with known checksums to ensure the downloaded binary hasn't been tampered with.
| URL="https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz"; \ | |
| # install ffmpeg manually since the version available in apt is from the 4.x branch due to #drama. | |
| # as of writing these static builds will be the latest 7.0.x release. | |
| RUN set -eux; \ | |
| URL="https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz"; \ | |
| EXPECTED_SHA256="<insert_known_checksum_here>"; \ | |
| echo "Downloading FFmpeg static build from $URL"; \ | |
| curl -fsSL "$URL" -o /tmp/ffmpeg.tar.xz; \ | |
| echo "$EXPECTED_SHA256 /tmp/ffmpeg.tar.xz" | sha256sum -c -; \ | |
| tar -xJf /tmp/ffmpeg.tar.xz -C /tmp; \ | |
| install -m755 /tmp/ffmpeg-*/ffmpeg /usr/local/bin/ffmpeg; \ | |
| install -m755 /tmp/ffmpeg-*/ffprobe /usr/local/bin/ffprobe; \ | |
| rm -rf /tmp/ffmpeg* |
Type: Security | Severity: High
TL;DR
Updated the recording server and Chromium headful image to use FFmpeg 7.x, reduce video resolution to 768p for performance, and improve FFmpeg shutdown reliability.
Why we made these changes
To reduce computational load by dropping high-resolution recording (1080p), leverage a newer FFmpeg version (7.x) for better features and stability via static builds, and enhance the robustness of the recording server's FFmpeg process management.
What changed?
nekoserver readiness check inwrapper.sh.-profile:v high,-pix_fmt yuv420p) for better web compatibility.oapi.goandopenapi.yamlto reflect updated server API specifications.Validation
config_test.goto reflect new frame rate limits.ffmeg_test.goto expect specific exit status errors during FFmpeg recorder stop, indicating refined shutdown handling.