Feature/mqtt screenshots#1466
Open
lwsrbrts wants to merge 12 commits into
Open
Conversation
Previously, MQTT screenshot publishing would silently fail whenever no WebRTC client was streaming, because video_capture_jpeg() required the V4L2/VENC streaming loop to be active to supply a frame. Changes: - Refactor do_jpeg_capture into a synchronous do_jpeg_encode_frame helper that handles VENC JPEG_CHANNEL setup/teardown and returns raw bytes without touching the mutex/cond signaling machinery. - Add video_capture_jpeg_standalone() which opens /dev/video0 independently, allocates DMA buffers from the existing memPool, warms up the capture pipeline by discarding one stale frame, then encodes the second frame via do_jpeg_encode_frame and tears everything down cleanly. - Update video_capture_jpeg() to dispatch to the standalone path when streaming_flag is false, instead of returning -ENODATA. - Prevent HDMI sleep mode from engaging when MQTT screenshot publishing is active (ScreenshotIntervalSec > 0), matching the existing pattern that blocks sleep when WebRTC sessions are active. This avoids capture timeouts that would otherwise occur once the capture chip powers down. - Add an amber warning card in the MQTT settings UI below the Screenshot Interval field (shown only when interval > 0) informing the user that HDMI Sleep Mode has no effect while screenshot publishing is active. - Add corresponding i18n string mqtt_screenshot_interval_sleep_warning.
When screenshot_interval_sec is 0, the periodic publish is disabled but the HA 'Capture Screenshot' button is still available. In that case the HDMI sleep mode can engage after the idle timeout, causing the standalone V4L2 capture to time out when the button is pressed. publishScreenshot() now: - Checks whether the capture chip is in sleep mode before attempting capture. - If sleeping, disables sleep mode and waits 5 s for the HDMI signal to re-lock (matching the same delay used in VideoStart). - Defers a call to startVideoSleepModeTicker() so the chip returns to sleep after its normal idle timeout once the capture is complete. Since publishScreenshot is always invoked in its own goroutine the 5-second wait does not block any other path.
Previously, startVideoSleepModeTicker() was only called when the chip was woken from sleep. If the button was pressed while the chip was already awake (e.g. shortly after a previous capture), the idle countdown was not reset and the chip could sleep sooner than expected. Move the defer startVideoSleepModeTicker() call outside the sleeping block so the countdown is reset on every capture attempt, regardless of whether the chip needed waking. Repeated button presses will continuously push back the sleep deadline, which is the expected behaviour when the user is actively requesting screenshots.
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds MQTT-based screenshot support (scheduled publishing + Home Assistant discovery entities) and prevents HDMI sleep mode from interfering with screenshot capture.
Changes:
- Introduces screenshot capture path end-to-end (native capture → gRPC → MQTT publish).
- Adds UI + localization for configuring scheduled screenshots and an optional HA “Capture Screenshot” button.
- Updates HDMI sleep behavior to stay awake while scheduled screenshot publishing is enabled.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| video.go | Skips entering HDMI sleep when scheduled MQTT screenshot publishing is active. |
| ui/src/routes/devices.$id.settings.mqtt.tsx | Adds UI controls/validation for screenshot publishing and HA button. |
| ui/localization/messages/en.json | Adds strings for the new screenshot settings section. |
| mqtt_publish.go | Adds screenshot capture + MQTT binary publish helper. |
| mqtt_discovery.go | Publishes HA discovery for screenshot image + optional capture button. |
| mqtt_commands.go | Adds MQTT command handler for on-demand screenshot capture. |
| mqtt.go | Extends MQTT config and starts a periodic screenshot ticker on connect. |
| internal/native/video.go | Adds native interface method for capturing a JPEG frame. |
| internal/native/proxy.go | Adds proxy forwarding for JPEG capture over gRPC client. |
| internal/native/proto/native.proto | Adds gRPC method/response for JPEG retrieval. |
| internal/native/interface.go | Extends the native interface with VideoCaptureJPEG. |
| internal/native/grpc_servermethods.go | Implements the new gRPC server method returning JPEG bytes. |
| internal/native/grpc_clientmethods.go | Implements the new gRPC client method to fetch JPEG bytes. |
| internal/native/empty.go | Adds failsafe-mode implementation returning “not supported”. |
| internal/native/cgo_notlinux.go | Adds platform-not-supported stub for JPEG capture. |
| internal/native/cgo_linux.go | Adds CGO binding for JPEG capture and copies returned buffer safely. |
| internal/native/cgo/video.h | Declares native C API for capturing a JPEG frame. |
| internal/native/cgo/video.c | Implements JPEG snapshot capture in streaming and standalone modes. |
| internal/native/cgo/ctrl.h | Declares exported ctrl-layer wrapper for JPEG capture. |
| internal/native/cgo/ctrl.c | Adds ctrl-layer wrapper calling video_capture_jpeg(). |
| config.go | Adds MQTT config defaults for new screenshot-related fields (partial). |
| .devcontainer/docker/devcontainer-lock.json | Adds devcontainer feature lockfile entry. |
Files not reviewed (2)
- internal/native/proto/native.pb.go: Language not supported
- internal/native/proto/native_grpc.pb.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+677
to
+683
| struct buffer bufs[3]; | ||
| memset(bufs, 0, sizeof(bufs)); | ||
| int bufs_allocated = 0; | ||
|
|
||
| for (int i = 0; i < input_buffer_count; i++) { | ||
| struct v4l2_plane *plane = &bufs[i].plane_buffer; | ||
| memset(plane, 0, sizeof(*plane)); |
Comment on lines
+1084
to
+1087
| // Capture JPEG if requested (before returning the V4L2 buffer) | ||
| if (jpeg_requested) { | ||
| do_jpeg_capture(&stFrame, width, height); | ||
| } |
Comment on lines
+219
to
+222
| // Start periodic screenshot publishing if configured | ||
| if config.MqttConfig != nil && config.MqttConfig.PublishScreenshot && config.MqttConfig.ScreenshotIntervalSec >= 10 { | ||
| m.startScreenshotTicker() | ||
| } |
Comment on lines
+296
to
+313
| func (m *MQTTManager) startScreenshotTicker() { | ||
| interval := config.MqttConfig.ScreenshotIntervalSec | ||
| if !config.MqttConfig.PublishScreenshot || interval < 10 { | ||
| return | ||
| } | ||
| go func() { | ||
| ticker := time.NewTicker(time.Duration(interval) * time.Second) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| m.publishScreenshot() | ||
| case <-m.done: | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| } |
| EnableHADiscovery: false, | ||
| EnableActions: true, | ||
| DebounceMs: 500, | ||
| PublishScreenshot: false, |
| value={settings.screenshot_interval_sec.toString()} | ||
| error={fieldErrors.screenshot_interval_sec} | ||
| onChange={e => { | ||
| updateField("screenshot_interval_sec", parseInt(e.target.value) || 60); |
…T macro Using a const local variable as an array size causes a VLA (variable-length array) in C, which is a compile-time ambiguity and can lead to out-of-bounds access if the compiler does not treat it as a compile-time constant. Replace with a #define so that all buffer and loop declarations that reference the buffer count are unambiguously fixed-size constants.
jpeg_requested is written by video_capture_jpeg() under jpeg_mutex from the caller's thread, but was read in run_video_stream()'s per-frame loop without holding the mutex. This is a data race: the compiler and CPU are free to cache or reorder the read, producing incorrect behaviour. Fix by reading jpeg_requested under the same mutex that all writers already hold. The lock is uncontested in the common case (no JPEG capture pending), so the per-frame overhead is negligible.
startScreenshotTicker() is called from the reconnect handler, so every reconnection to the broker would spawn an additional ticker goroutine. Over time this causes screenshot publishes to pile up and wastes resources. Add a screenshotRunning atomic.Bool guard: if a ticker goroutine is already running the function returns early; the goroutine clears the flag when it exits so a fresh goroutine can start after a genuine restart.
getDefaultConfig() initialised MqttConfig with PublishScreenshot and PublishScreenshotButton but omitted ScreenshotIntervalSec, leaving it as 0. A zero interval causes the screenshot ticker to fire as fast as possible, flooding the broker on first run before the user saves a configuration. Set the default to 60 seconds to match the documented behaviour.
All three parseInt() calls in the MQTT settings form (port, screenshot interval, debounce ms) were missing the required radix argument. Without an explicit radix, some environments may parse a leading-zero value as octal, producing a silently wrong number. The fallback pattern `parseInt(...) || default` also silently replaces the value 0 with the default, so entering 0 for debounce_ms was impossible. Fix all three handlers: - Pass radix 10 to parseInt(). - Use isNaN() to detect invalid input and fall back to the default only in that case, allowing 0 to be stored as a valid value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1459
Summary
Adds MQTT screenshot publishing for the JetKVM's HDMI output, with full Home Assistant auto-discovery support. Two new opt-in settings are exposed in the MQTT UI: a Scheduled Screenshots switch that periodically captures and publishes a JPEG at a configurable interval (≥10 s), and a Register Button switch that registers a one-shot Capture Screenshot button entity in HA. Both are disabled by default; images are not retained on the broker, and the image entity is disabled by default in HA to avoid large payloads on reconnect.
For what it's worth, I realise there's a lot of additions here - I added this for a personal requirement but can see the benefit of it being made available to a wider audience. Pick, choose, discard, close as you feel necessary. It's worth reading the issue #1459 to see the changes and considerations made from initial work.
Checklist
make test_e2elocally and passedCloses #<issue-number>)Running make test_e2e gives me 7 errors but none seem related to this code 🤷🏻. Output below:
7 failed [remote-agent] › e2e/remote-agent/ra-all.spec.ts:437:3 › Remote Host Agent › keyboard: toggle keys with LED round-trip [ui] › e2e/log-level.spec.ts:45:3 › Log level filtering › live changes filter TRACE/DEBUG/INFO/WARN/ERROR logs without restart [ui] › e2e/log-level.spec.ts:88:3 › Log level filtering › reverts INFO to WARN after reboot ──── [ui] › e2e/settings-local-auth.spec.ts:39:3 › Settings Local Auth Tests › password minimum length validation in settings create modal [ui] › e2e/timesync-custom-ntp.spec.ts:74:3 › Custom NTP time sync › custom NTP server is queried after settings change [ui] › e2e/welcome-password.spec.ts:22:3 › Welcome Password Flow Tests › password minimum length validation during welcome [ui] › e2e/zz-login-rate-limit.spec.ts:13:3 › Login Rate Limiting › rate limiting after multiple failed login attempts