From ddf3dc0c8407a904f9b8aa707d527fc809f7c545 Mon Sep 17 00:00:00 2001 From: maziggy Date: Fri, 1 May 2026 08:43:16 +0200 Subject: [PATCH] fix(camera): skip MJPEG warm-up frame, return second representative frame (#1177) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _capture_mjpeg_frame returned the very first JPEG it found in the bytes stream, but many MJPEG sources — go2rtc most notably, and several IP cameras — emit a warm-up frame on the byte that follows connection accept: usually the last keyframe held in the encoder, typically black or stale until the encoder catches up to live content. Subsequent frames on the same connection are fine. Result: every code path that opened a fresh capture (snapshot UX, finish photos in notifications, timelapse, plate-detection CV, Obico ML inference, Settings → Test button) returned a black image on go2rtc-fronted cameras. Reporter's support log showed every black frame was 11095 bytes (pure-black 1280x720 JPEG ≈ 10-15 KB) while real-content frames from the same source were 30-45 KB. Fix: - Read past the first complete JPEG, return the second. - Fall back to the first frame if the connection closes / times out / hits the 5 MB buffer cap before a second arrives. Without that fallback, slow / single-frame streams that pre-fix returned the warm-up would post-fix return None — a regression. The fallback guarantees we never do worse than current behaviour. - Inner while-loop now drains every complete frame already in the buffer before pulling the next chunk so high-FPS sources that pack multiple frames per chunk are handled correctly. Untouched: snapshot / rtsp / usb capture paths, generate_mjpeg_stream (live-view fan-out). 7 new regression tests in TestCaptureMjpegFrameWarmupSkip cover two-frames-in-two-chunks, two-frames-in-one-chunk, partial-frame- split-across-chunks, single-frame fallback, timeout fallback, zero- frame stream returns None, non-200 returns None. Latency penalty: at most one frame interval (typically 50 ms - 1 s on a steady stream), well within every caller's tolerance window. --- CHANGELOG.md | 2 + backend/app/services/external_camera.py | 69 ++++--- .../unit/services/test_external_camera.py | 193 ++++++++++++++++++ 3 files changed, 240 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d21a2601f..9f4b05d6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ All notable changes to Bambuddy will be documented in this file. - **Filament Track Switch (FTS) support — print modal filament dropdown is no longer empty when an X2D / H2D has the FTS accessory installed** ([#1162](https://github.com/maziggy/bambuddy/issues/1162), reported by @mkavalecz) — When the FTS accessory is installed the printer's MQTT changes one nibble of the per-AMS `info` bitmask: bits 8-11 flip from a fixed extruder ID (0x0 / 0x1) to `0xE` ("uninitialized"), because the AMS is no longer wired to a single nozzle — the FTS dynamically routes any slot to either extruder. Bambuddy's MQTT parser already skipped 0xE entries when building `ams_extruder_map` (matching BambuStudio's reading for boot-time transient state), so with the FTS installed the map ended up empty and the print modal's filament dropdown — which filters by `extruderId === nozzle_id` to prevent cross-nozzle assignment ("position of left hotend is abnormal" failures) — filtered out *every* loaded slot. Net effect: empty Filament Mapping dropdown on every dual-nozzle print with the FTS, even when the AMS was fully loaded with the right material. Detection comes from a new MQTT field — `print.device.fila_switch` — which is non-null only when the accessory is installed; it carries the routing topology as two arrays: `in[track] = currently fed slot (-1 = empty)` and `out[track] = extruder this track terminates at`. The fix surfaces this through a new `FilaSwitchState` dataclass on `PrinterState` (`installed`, `in_slots`, `out_extruders`, `stat`, `info`) and the equivalent `FilaSwitchResponse` Pydantic schema on the `GET /printers/{id}/status` route. Frontend (`useFilamentMapping.ts` + `FilamentMapping.tsx`) skips the per-extruder filter when `printerStatus.fila_switch?.installed === true` so any compatible AMS slot can satisfy any nozzle's filament requirement, since the FTS handles the routing. Slots currently fed into a track also get a routing badge in the dropdown — `[L]` or `[R]` — so the user can tell at a glance which slot the FTS is currently routing where (idle slots get no badge: they can be routed to either extruder on demand). The hard "no cross-nozzle assignment" filter on real dual-nozzle printers without the FTS stays untouched (still trips the same way it always has — `fila_switch == null` keeps the existing behaviour). 4 backend tests in `test_bambu_mqtt.py::TestFilamentTrackSwitchDetection` (default-not-installed, detect-from-MQTT-using-the-reporter's-bundle, no-fila_switch-field-stays-not-installed, missing-in-out-arrays-don't-crash) and 2 frontend tests in `useFilamentMapping.test.ts` (FTS-active drops the nozzle filter; explicit `fila_switch: null` keeps the filter applied). Upstream fila_switch payloads with anything other than the documented shape are tolerated — `installed` flips on the *presence* of the field, the routing arrays default to empty lists if missing, and the dropdown skips the badge for slots not currently in `in_slots`. ### Fixed +- **External-camera frames returned as black on go2rtc and other MJPEG sources** ([#1177](https://github.com/maziggy/bambuddy/issues/1177), reported by @nkm8) — `_capture_mjpeg_frame` returned the very first JPEG it found in the stream's bytes (`backend/app/services/external_camera.py:282`), but many MJPEG sources — go2rtc most notably, and several IP cameras — emit a "warm-up" frame on the byte that follows connection accept: usually the last keyframe held in the encoder, which is often black or stale until the encoder catches up to live content. Subsequent frames on the same connection are fine. The reporter saw it across snapshot UX, finish photos in notifications, and timelapse — every code path that opens a fresh capture connection (snapshot endpoint, `[PHOTO-BG]` finish photo, plate-detection CV, Obico ML inference, layer timelapse, Settings → Test). His own observation that go2rtc's `/api/frame.jpeg` (single-frame, internally already warmed) is never black while the first frame off `/api/stream.mjpeg` is, matched the hypothesis exactly. Support-bundle evidence was clean: every black notification frame in his log was 11095 bytes (a pure-black 1280×720 JPEG encodes to ~10–15 KB on standard libjpeg quality settings), while every captured-after-warm-up frame from the same source was 30–45 KB. Fix: read past the first frame and return the second; if the connection closes / times out / hits the 5 MB buffer cap before a second frame ever arrives, fall back to the first so callers still get *something* (degrading slow / single-frame streams to None would regress every code path that relied on pre-fix behaviour). The inner-loop now drains every complete frame already in the buffer before pulling the next chunk so high-FPS sources that pack multiple frames per chunk are handled correctly. The `snapshot` / `rtsp` / `usb` capture paths and the live-view streaming endpoint (`generate_mjpeg_stream`) are untouched. 7 new regression tests in `test_external_camera.py::TestCaptureMjpegFrameWarmupSkip` cover (a) two-frames-in-two-chunks → second returned, (b) two-frames-in-one-chunk → second returned, (c) frame split across chunk boundary → assembled correctly, (d) single-frame stream → first returned via fallback (no None regression), (e) timeout after first frame → first returned via fallback, (f) zero-frame stream → None, (g) non-200 status → None. Latency penalty: at most one frame interval (typically 50 ms – 1 s on a steady stream). + - **MakerWorld sidebar entry visible to every user regardless of group permissions** ([#1175](https://github.com/maziggy/bambuddy/issues/1175)) — Backend already enforced `makerworld:view` on every `/makerworld/*` route (`backend/app/api/routes/makerworld.py:145, 157, 242, 406`), the permission was correctly granted to the admin and standard-user role defaults (`permissions.py:298, 364, 454`), and the frontend `Permission` type union already included `'makerworld:view' | 'makerworld:import'` (`client.ts:2498`) — but the sidebar's hand-maintained `navPermissions` map in `Layout.tsx:278` had no entry for `makerworld`, so `isHidden('makerworld')` always returned false and the entry rendered for every authenticated user. Users without the permission saw the entry, clicked, and the page rendered while every API call inside it 403'd. Two-line fix: (1) `Layout.tsx:278` — add `makerworld: 'makerworld:view'` to the map, matching every other sidebar entry's gating shape; (2) `App.tsx:200` — wrap the route in `` for defence in depth, so a user who knows the URL can no longer reach the page directly (matches the existing pattern on `settings`, `groups/new`, `groups/:id/edit` two lines below). 2 new Layout tests pin the contract: with auth enabled and a user lacking `makerworld:view`, the sidebar `` link is absent (other links like `/files` still render); with the permission granted, the link renders. - **Printer Info modal: serial-number and IP-address copy buttons silently did nothing on plain-HTTP LAN deployments** ([#1174](https://github.com/maziggy/bambuddy/issues/1174), reported by @BurntOutHylian) — `PrinterInfoModal`'s `CopyButton` only tried `navigator.clipboard.writeText()`, which is gated by the secure-context requirement (HTTPS or localhost). On the typical Bambuddy deployment shape — bare-IP HTTP on the LAN — `navigator.clipboard` is undefined; the existing `try/catch` swallowed the resulting `TypeError`, the icon never flipped to the tick, and nothing landed on the user's clipboard. Fixed by adding the same off-screen-textarea + `document.execCommand('copy')` fallback that `CameraTokensPage`'s plaintext-token modal already uses for plain-HTTP LAN deployments: gate on `navigator.clipboard && window.isSecureContext`, fall back to the legacy path otherwise, and surface the success-tick only when the copy actually landed (return early without flipping `copied` if `execCommand('copy')` returns false). The `try/finally` around the textarea guarantees DOM cleanup even when the browser throws on a restricted context. 3 new component tests in `PrinterInfoModal.test.tsx` cover (a) secure-context happy path uses `navigator.clipboard.writeText`, (b) plain-HTTP fallback path actually invokes `execCommand('copy')` and leaves no leaked textarea in the DOM, (c) `finally` cleanup removes the textarea even when `execCommand` throws synthetically. Thanks to @BurntOutHylian for the precise file/line pointer in the report. diff --git a/backend/app/services/external_camera.py b/backend/app/services/external_camera.py index 8f88bc4d1..59836d791 100644 --- a/backend/app/services/external_camera.py +++ b/backend/app/services/external_camera.py @@ -280,18 +280,32 @@ async def _capture_usb_frame(device: str, timeout: int) -> bytes | None: async def _capture_mjpeg_frame(url: str, timeout: int) -> bytes | None: - """Extract single frame from MJPEG stream. + """Extract a single representative frame from an MJPEG stream. - Note: This function intentionally makes requests to user-configured URLs. - External camera support requires connecting to user-specified camera endpoints. - URL is sanitized and dangerous destinations are blocked. + Many MJPEG sources — go2rtc most notably (#1177), and several IP cameras — + emit a "warm-up" frame on the byte that follows connection accept: usually + the last keyframe held in the encoder, which is often black or stale until + the encoder catches up to live content. To return a frame that's actually + representative of the scene we read past the first frame and return the + second; if the connection closes / times out / hits the buffer cap before + a second frame ever arrives we fall back to the first so callers still + get *something* (better than degrading slow / single-frame streams to None, + which would regress every code path that consumed pre-fix behaviour). + + Note: this function intentionally makes requests to user-configured URLs. + External camera support requires connecting to user-specified camera + endpoints. URL is sanitized and dangerous destinations are blocked. """ - # Sanitize URL - returns reconstructed URL from validated components safe_url = _sanitize_camera_url(url, ("http", "https")) if not safe_url: logger.error("Invalid MJPEG URL format: %s...", url[:50]) return None + jpeg_start = b"\xff\xd8" + jpeg_end = b"\xff\xd9" + first_frame: bytes | None = None # warm-up frame; fallback if no second arrives + buffer = b"" + try: async with ( aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=timeout)) as session, @@ -301,38 +315,45 @@ async def _capture_mjpeg_frame(url: str, timeout: int) -> bytes | None: logger.error("MJPEG stream returned status %s", response.status) return None - # Read chunks until we find a complete JPEG frame - buffer = b"" - jpeg_start = b"\xff\xd8" - jpeg_end = b"\xff\xd9" - async for chunk in response.content.iter_chunked(8192): buffer += chunk - # Look for complete JPEG frame - start_idx = buffer.find(jpeg_start) - if start_idx == -1: - continue - - end_idx = buffer.find(jpeg_end, start_idx + 2) - if end_idx != -1: - # Found complete frame + # A single chunk can carry multiple frames (e.g. high-FPS sources) + # or a partial frame. Drain every complete frame we already have + # before pulling the next chunk. + while True: + start_idx = buffer.find(jpeg_start) + if start_idx == -1: + # No frame start yet — drop trailing garbage, keep waiting. + break + end_idx = buffer.find(jpeg_end, start_idx + 2) + if end_idx == -1: + # Partial frame; trim already-discarded prefix so the + # buffer stays bounded across long-running streams. + if start_idx > 0: + buffer = buffer[start_idx:] + break frame = buffer[start_idx : end_idx + 2] - return frame + buffer = buffer[end_idx + 2 :] + if first_frame is None: + first_frame = frame # warm-up; keep but don't return yet + continue + return frame # representative second frame - # Keep searching, but limit buffer size if len(buffer) > 5 * 1024 * 1024: # 5MB limit logger.warning("MJPEG buffer exceeded 5MB without finding frame") - return None + break # exit chunk loop, fall through to first_frame fallback except TimeoutError: logger.warning("MJPEG frame capture timed out after %ss", timeout) - return None except (aiohttp.ClientError, OSError) as e: logger.error("MJPEG frame capture failed: %s", e) - return None - return None + # Stream ended / timed out / buffer cap before a second frame arrived. + # Return whatever warm-up frame we managed to read; better an iffy frame + # than None for callers that need *some* image (snapshot UX, plate-detect + # CV, finish photo). None only if no frame ever arrived at all. + return first_frame async def _capture_rtsp_frame(url: str, timeout: int) -> bytes | None: diff --git a/backend/tests/unit/services/test_external_camera.py b/backend/tests/unit/services/test_external_camera.py index 503abaa0f..40f06a096 100644 --- a/backend/tests/unit/services/test_external_camera.py +++ b/backend/tests/unit/services/test_external_camera.py @@ -8,6 +8,199 @@ from unittest.mock import patch import pytest +JPEG_START = b"\xff\xd8" +JPEG_END = b"\xff\xd9" + + +def _make_jpeg(payload: bytes = b"\x00" * 100) -> bytes: + """Build a synthetic JPEG byte sequence (SOI + payload + EOI).""" + return JPEG_START + payload + JPEG_END + + +class _FakeMjpegResponse: + """Drop-in for aiohttp's response that drives `iter_chunked` from a fixed + list of byte chunks. Each chunk is yielded once; if the iterator runs out + the response is treated as closed (which is the realistic behaviour for an + MJPEG stream the upstream server has finished). An optional `raise_after` + raises the supplied exception after N chunks to simulate timeout / IO + failure mid-stream.""" + + def __init__(self, chunks, status=200, raise_after=None, raise_exc=None): + self.status = status + self._chunks = list(chunks) + self._raise_after = raise_after + self._raise_exc = raise_exc + self.content = self # the function calls `response.content.iter_chunked(...)` + + def iter_chunked(self, _size): # noqa: ARG002 — chunk size is informational + chunks = self._chunks + raise_after = self._raise_after + raise_exc = self._raise_exc + + async def _gen(): + for i, chunk in enumerate(chunks): + if raise_after is not None and i >= raise_after: + raise raise_exc + yield chunk + + return _gen() + + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return None + + +class _FakeMjpegSession: + """Drop-in for aiohttp.ClientSession; `get(url)` returns a pre-baked + `_FakeMjpegResponse`.""" + + def __init__(self, response): + self._response = response + + def get(self, _url): + return self._response + + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return None + + +def _patch_mjpeg_session(response): + """Patch `aiohttp.ClientSession` inside the external_camera module so the + real `_capture_mjpeg_frame` runs against our fake stream.""" + + def _factory(*_args, **_kwargs): + return _FakeMjpegSession(response) + + return patch("backend.app.services.external_camera.aiohttp.ClientSession", _factory) + + +class TestCaptureMjpegFrameWarmupSkip: + """Regression for #1177. Many MJPEG sources (notably go2rtc) emit a + warm-up / black frame on the first byte that follows connection accept; + `_capture_mjpeg_frame` must skip past it and return the second frame. + Where the stream ends or times out before a second frame ever arrives the + function falls back to the warm-up frame so callers still get *something* + — returning None there would regress every code path that consumed the + pre-fix behaviour (snapshot UX, plate-detection CV, finish photo, + timelapse, Obico inference).""" + + @pytest.mark.asyncio + async def test_skips_warmup_frame_returns_second_frame(self): + # Two frames arriving in two chunks — typical of a steady MJPEG feed. + # Pre-fix this returned `warm`; post-fix returns `live`. + from backend.app.services.external_camera import _capture_mjpeg_frame + + warm = _make_jpeg(b"\x10" * 50) # warm-up — encoder hasn't caught up + live = _make_jpeg(b"\x20" * 200) # representative scene + response = _FakeMjpegResponse(chunks=[warm, live]) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame == live + assert frame != warm + + @pytest.mark.asyncio + async def test_two_frames_in_single_chunk_returns_second(self): + # High-FPS sources often pack multiple frames into one chunk delivered + # in a single iteration of `iter_chunked`. The inner while-loop must + # drain every complete frame from the buffer before reading more. + from backend.app.services.external_camera import _capture_mjpeg_frame + + warm = _make_jpeg(b"\x10" * 50) + live = _make_jpeg(b"\x20" * 200) + response = _FakeMjpegResponse(chunks=[warm + live]) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame == live + + @pytest.mark.asyncio + async def test_partial_frame_split_across_chunks_assembles_correctly(self): + # Realistic chunking: TCP doesn't respect frame boundaries, so a + # single frame can straddle two chunks. The fix's buffer-trim path + # must still find the SOI / EOI pair across the boundary. + from backend.app.services.external_camera import _capture_mjpeg_frame + + warm = _make_jpeg(b"\x10" * 50) + live = _make_jpeg(b"\x20" * 200) + # Split `live` mid-payload + split_at = len(JPEG_START) + 100 + chunks = [warm + live[:split_at], live[split_at:]] + response = _FakeMjpegResponse(chunks=chunks) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame == live + + @pytest.mark.asyncio + async def test_single_frame_stream_falls_back_to_first_frame(self): + # Critical no-regression case. A snapshot-style endpoint that emits + # exactly one frame and closes the connection (or a slow stream that + # only delivers one frame within the timeout window) must still hand + # back that one frame — not None. Pre-fix users on these sources got + # the frame; the warm-up skip would otherwise turn that into None + # silently. + from backend.app.services.external_camera import _capture_mjpeg_frame + + only = _make_jpeg(b"\xab" * 80) + response = _FakeMjpegResponse(chunks=[only]) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame == only + + @pytest.mark.asyncio + async def test_timeout_after_first_frame_falls_back_to_first(self): + # Timeout mid-stream — the warm-up frame has already arrived but the + # second hasn't. Same fallback: hand back what we have, never None. + from backend.app.services.external_camera import _capture_mjpeg_frame + + warm = _make_jpeg(b"\x10" * 50) + response = _FakeMjpegResponse( + chunks=[warm, b""], # second yield will raise instead + raise_after=1, + raise_exc=TimeoutError(), + ) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame == warm + + @pytest.mark.asyncio + async def test_no_frames_returns_none(self): + # Server replied 200 but emitted zero JPEG bytes before closing — + # there's nothing to return, so None is the correct answer. + from backend.app.services.external_camera import _capture_mjpeg_frame + + response = _FakeMjpegResponse(chunks=[b"\x00\x01\x02\x03"]) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame is None + + @pytest.mark.asyncio + async def test_non_200_status_returns_none(self): + # Invariant: a 4xx/5xx is never a valid frame source. + from backend.app.services.external_camera import _capture_mjpeg_frame + + response = _FakeMjpegResponse(chunks=[], status=404) + + with _patch_mjpeg_session(response): + frame = await _capture_mjpeg_frame("http://camera.example/stream", timeout=15) + + assert frame is None + class TestFormatMjpegFrame: """Tests for MJPEG frame formatting."""