[tridge]

Accept authenticated chunked Matroska publishers and preserve native FFV1 samples and frame metadata through fan-out and recording. Cache codec headers for late viewers and rotate recordings at complete cluster boundaries, retaining existing admission, viewer and quota policies.

Provide a greyscale desktop thermal viewer and browser guidance, with tests for authentication, fragmented input, reconnects, slow viewers and recording rotation.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-21)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_22_AIReview/devcall_pr_reviews.html#prSupportProxy-45

Reviewed at head 1f56a602ac.

COMMENT. Careful work, and I verified three of its load-bearing claims by running the proxy rather than reading it. Two things worth fixing before merge, neither a memory-safety or data-integrity problem.

1. ISSUE — videoview.cpp:296 — the viewer path is built from the current publisher state, so /vN.mkv only resolves while a Matroska publisher is latched. Before the publisher arrives and after it disconnects, scanner.matroska is false (reset by latch_publisher at video.cpp:727 and by end_stream → scanner.reset() at video.cpp:1622), so the request 404s. Measured against a running proxy:

EARLY /v1.mkv -> 404 not found   body: "Try /v1.ts on this port."
EARLY /v1.ts  -> 503 stream not ready
LIVE  /v1.mkv -> 200 OK
AFTER /v1.mkv -> 404 not found

This is the documented flow — the README tells viewers to use http://HOST:PORT/v3.mkv and says "a publisher disconnect closes its viewers, who must reconnect to the next session", and that reconnect lands on the 404. scripts/view_raw_thermal.py survives it because it retries every second, but the README's ffplay http://HOST:PORT/v3.mkv just fails, and the 404 body actively points at the wrong path. Suggest accepting both extensions in the same test and letting begin_stream()'s existing 503 handle "no publisher yet".

2. ISSUE — video.cpp:758 and :804 — nothing about a Matroska publisher failing is logged at all. handle_mkv() answers 403/409/400 and closes but never calls log_reject(), so s.rejects isn't incremented and no line is printed — every other publisher transport goes through log_reject (:906, :1221). And close_mkv() takes a why string but passes it only to end_stream(), where it becomes the viewers' drop reason and is discarded when there are none; close_rtsp prints "publisher gone (%s)", the UDP idle path prints "publisher idle, releasing", close_mkv prints nothing. Reproduced by publishing a valid header and Cluster, then a chunk with a garbage EBML ID, then a wrong password — the entire proxy.log for that run has no teardown line and no rejection line.

The commit immediately before this one on the branch is 21474d0 "video: say when a wrong publish password is judged", so this is the same gap reopened for the new transport — and the malformed-stream case is the security-relevant one. Suggest log_reject(…) on the 403 branch (and VIDEO_ADMIT_SLOT_BUSY on 409), plus a printf in close_mkv mirroring close_rtsp. Also worth splitting the single "Matroska publisher disconnected or malformed" string at :1909 — pump_mkv already distinguishes a clean recv()==0, a chunk-framing failure and scanner.failed, and those are three very different operator problems.

A refuted finding, so you don't go chasing it. A cold second pass reported the !length test at videomkv.h:103 as a data-loss BUG — that a graceful chunked upload loses its final payload for live viewers when the last data chunk and the terminating 0\r\n\r\n arrive in the same read. The code behaviour is as described, but I checked the publisher side and it isn't reachable: send_chunk() in AP_CameraGimbal has exactly two call sites (support_video.cpp:222 with the header, :265 with a frame), neither can be zero-length, and nothing there ever writes a terminating zero chunk. So I've dropped it to a note. What's genuinely wrong today is only that a clean end is logged as "disconnected or malformed", which compounds finding 2 — and if the inline comment means the zero-chunk handling is deliberate, a distinct "clean end" result would match it, and would matter for any future RFC-conformant publisher.

Notes (detail in the report): the lapped-anchor fallback becoming a hard fail(503) is an unannounced behaviour change to the existing .ts browser player, and on the WebSocket path fail() emits no body so the viewer is dropped with no close frame; HTTP GET viewers now authenticate through ws_authorise(), so the 60 s ?t= view token works on plain HTTP for all slots including existing .ts ones — I see no weakness (HMAC'd and slot-bound) but it's an auth-surface widening bundled into a Matroska PR and undocumented, and the pw local at :302-306 is now dead; the slot-busy test at :759 omits the VIDEO_PUB_IDLE_S allowance the other two sites have, so after an unclean publisher drop a reconnecting camera gets 409 for up to 15 s where RTSP would take the slot; the per-tick stats line is still TS-specific for a Matroska publisher; and s.scanner.cluster = [&s](…) makes Slot self-referential, safe today but a future std::vector<Slot> would dangle.

What I checked and cleared. The lossless claim is clear and verified empirically — pump_mkv writes raw bytes to the ring and the recorder writes prefix + Cluster verbatim; byte-compared, 12 frames reassemble byte-for-byte across 6+ rotated segments and a late joiner gets HEADER + cluster(N) byte-identical. Truncated-Matroska validity is better than I expected: the recorded header carries the unknown-size Segment so a killed process leaves a playable file with no finalisation step, and I probed a mid-Cluster disconnect (100 bytes of a 200-byte Cluster, then close) — the recording contained exactly HEADER + cluster(1) + cluster(2), 473 bytes, byte-identical. No partial Cluster is ever written, because the incomplete element stays in pending and the callback never fires. EBML and chunk parser bounds are sound (vint() rejects a zero first byte, caps ID/size lengths, returns "need more"; VideoChunks::feed checks the size before the multiply). Path traversal is clear — nothing vehicle-supplied reaches a path, and the request target is compared for exact equality against a snprintf, never concatenated. Resource lifetime, transport mutual exclusion, backpressure, partial writes and disk-full all traced clean.

Cross-repo agreement with ArduPilot/AP_CameraGimbal#31 (head b784b62): exact. Request line, the CL+TE-smuggling rejection, byte-identical 100 Continue strings, the EBML → unknown-size Segment → Info → Tracks → Cluster order against the proxy's four-stage sequence (hand-decoded against your vint()), size limits and chunk framing all match. One gap worth noting: the two sides share no version or profile marker — the proxy hard-codes the element order, so any future camera-side muxer change (a SeekHead, a Void, Tags, a CRC-32 element) becomes a silent failed = true teardown with, per finding 2, no log line at all.

[AP-Review]

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 7705195a25.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/supportproxy/45/1.html#prSupportProxy-45

Re-review after the rebase: the last review was at 1f56a602ac, this one covers 7705195a25 and nothing newer. The changed lines are the same and there has been no reply on the thread, so the earlier points were re-checked on unchanged code. I rebuilt the head and the merge-base and re-ran the probes.

Nothing blocking. The relay and recording are byte-exact in the PR's tests, the parsers are bounded, admission matches the other transports, and the merged camera publisher sends exactly what handle_mkv() expects. Worth fixing before merge:

1. The new tests don't run in CI. tests/test_video_matroska.py is not in PHASES in scripts/run_tests.py; run_tests.py --list collects 680 tests and none from it.

2. Rejected and failed Matroska publishers leave no log line. handle_mkv() never calls log_reject() and close_mkv() prints nothing. A wrong password, a second publisher and a malformed stream all went unlogged in my run.

3. /vN.mkv is 404 unless a Matroska publisher is live (videoview.cpp:296), and while one is live /v1.ts gets a 404 saying "Try /v1.ts on this port." Lower severity than last time: .ts is refused too (503) when idle, so this is about the right answer, not availability.

4. The desktop viewer exits with a traceback on a second S press for the same frame (view_raw_thermal.py:111, FileExistsError).

Smaller: existing MPEG-TS viewers whose join point has been overwritten now get 503 where the merge-base served them (videoview.cpp:103, :526) - fine if intended, but unmentioned; the pw local above videoview.cpp:306 is dead; the README section landed under License; and the rotation test sorts segments by mtime with no tie-break, which is worth changing to a name sort before the file goes into CI (no failure seen, UNCONFIRMED on older kernels).

Settled from last time: the zero-chunk case at videomkv.h:103 stays a note, since the camera never sends a terminating chunk, and the self-referential lambda point is withdrawn.

Not tested by any pass: a real camera, FFV1 decode through PyAV, the OpenCV window, ffplay, a browser. CI was still pending for this head.