feat: record + replay vision pipelines from file logs#2491
Open
JosephTLockwood wants to merge 154 commits into
Open
feat: record + replay vision pipelines from file logs#2491JosephTLockwood wants to merge 154 commits into
JosephTLockwood wants to merge 154 commits into
Conversation
check disk space periodically while recording
JavaDoc line-wrap drift collected on six files after recent edits. spotlessApply rewraps long paragraphs and collapses one method-call linebreak in FrameRecorder.sampleTssNow.
github-actions
Bot
added
frontend
Contributor
Author
|
Heads-up for reviewers: while auditing the diff before opening I flagged two slices of pre-fork code from #2183 that this PR carries forward but doesn't otherwise depend on. Calling them out explicitly so a reviewer can decide whether to keep, finish, or drop them — happy to cut either in a follow-up commit if the answer is "drop": 1. Reserve-recording-space (~310 LOC, 7 files) — robot-code-triggered "delete oldest recordings to free disk for an upcoming match" workflow. Files:
Things to weigh: the match-data sort relies on a Replay does not depend on any of this; if it's dropped, the recorder just stops accepting frames when disk fills (existing low-disk-space gate at 2. Both buckets compile and ship cleanly on their own — happy to land them, drop them, or treat them as a separate follow-up. |
Contributor
Author
|
At this point I think the flow is solid and everything is working. I was able to Record a Video with no Apriltag Detection enabled. Run it through repaly and get poses. Then use the replay file AKit replay to correct robot pose. There are a ton of decisions to be made still on what to log, not log, file types, etc... I struggled with it and went through a ton of diffrent version (as you can probably seem from git history) but think it's at least good enough for testing. If you have a chance to test let me know how it goes. Replay is inside of Settings for now. Only thing I want to do is see if I can simplify anything more. Still not familar with PV library. Probably duplicated stuff. Quick Rundown on what is being logged in replay file Here is the file I use for the IO implementation |
Remove the robot-triggered "free disk space before a match" path: PhotonUtils.reserveSpace, PhotonCamera.willRecord, HardwareManager.reserveRecordingSpace, VisionModule.recordingSpaceNeeded, and the NT reserveRecordingSpace subscribers. It was non-functional end to end: the lib published to a root-level topic the server never watched, the server subscribed to one topic name as two conflicting NT types, a usable-space read of -1 would delete every recording, and the match-data comparator could throw on the NT listener thread. Replay does not depend on any of it.
Remove the single-value RecordingStrategy enum and everything wired to it: the FrameRecorder constructor switch, getSupportedStrategies, HardwareConfig.recordingStrategy, HardwareManager.getRecordingStrategy, the UI settings fields, and the client's Recording Strategy select. The enum had exactly one value (SNAPSHOTS) and nothing branched on it; the client's setRecordingStrategy sent a websocket key the server had no handler for. It was pure future-proofing with no behavior behind it.
The writer loop caught only InterruptedException, so an unchecked exception from imwrite (e.g. an empty Mat) silently killed the thread; release() then did a blocking put() of the poison pill, hanging the calling NT listener thread forever on a full queue. Catch unchecked exceptions in the loop body (stop recording, keep draining), reject empty mats in recordFrame before they reach imwrite, and in release() use a timed offer for the poison pill with an interrupt-and-join escalation. Only drain the queue and release the native jpegWriteParams once the writer is confirmed dead, so we never free a MatOfInt still in use inside imwrite.
Two recordings resolving to the same directory within a match would truncate metadata.jsonl while the previous run's frames/*.jpg persisted, so replay silently served stale frames for any dropped sequence. uniquifyOutputPath now suffixes the directory (_2, _3, ...) when the target already holds a recording. Name recording directories timestamp-first with a sanitized match-data token appended, instead of the raw match-data string. The raw string's colons made safeResolve reject the path on Windows, and reused names collided within a match.
Delete the temp zip if ZipUtil.pack throws (it was created just before the pack call and leaked on failure), and make exportAll fail with a clear "No recordings to export" when the recordings root is missing or empty instead of returning a zero-byte, invalid zip. Collapse the duplicated export/exportCamera bodies into a single zipDirectory helper.
startRecording/stopRecording did a get-then-act on an AtomicBoolean and returned a boolean no caller read; reduce them to a plain set. Also drop the leftover second jpegWriteParams.release() in release() now that the native params are freed exactly once, on the writer-confirmed-dead path.
The constructor reaches updateEntries twice (once via updateCameraNickname), and this PR made updateEntries non-idempotent by adding the four replay publishers, so the first set leaked. Close the existing publishers null-safely before recreating them, which also covers later nickname changes.
setRecording/getRecording were reimplemented as identical "must not throw on the NT listener thread" no-ops in FileFrameProvider, LibcameraGpuFrameProvider and FileLogFrameProvider, and TestSource actually violated that contract by throwing UnsupportedOperationException. Make FrameProvider provide the warn-and-ignore no-op (getRecording returns false) and delete the four overrides; USBFrameProvider keeps its real implementation. Also drop the tautological supplierBoundToTheGetterTracksTheSwap test.
pace() parked for the full recorded inter-frame delta with no way to be woken, so requestStop() (cancel/EOF) had to wait out the gap — unbounded on a corrupt sidecar — and VisionModule's 2s swap-back fence routinely timed out and released the provider under a live thread. Record the paced thread and unpark it from requestStop(), looping the park to re-check the deadline and stop flag (parkNanos may return spuriously).
The stopped state had a standalone fallback that parked the vision thread until interrupted when no onEof callback was wired. Every production path (VisionModule.startReplay) wires the callback before installing the provider, so an unwired provider is a bug; throw IllegalStateException rather than silently starve the vision thread. Update the EOF/cancel test coverage accordingly.
The recording get/set callbacks were bound method references (getFrameProvider()::getRecording), so they captured the construction-time provider and never saw the in-place swap to the replay provider. A recording toggle during replay then armed a recorder on the parked live camera. Use lambdas that re-read getFrameProvider() per call so the callbacks follow the live → replay → live swap.
The exporter was created lazily on the first result with the then-current settings hash and kept until replay end, so switching pipelines mid-replay kept appending the new pipeline's results into the previous hash's file, under the previous header. Track the hash the exporter was opened with and, when it changes, close and recreate against the new settings' file. All on the vision thread, so no new locking.
finishReplay flipped isReplaying=false at the top, before the swap-back fence and before closing the JSON exporter, so a status poll could observe "ended" and download a truncated results file. Reorder so the exporter is closed and nulled before isReplaying flips and before publishReplayState. Make jsonExporterDisabled volatile (written on the vision thread, reset by the replay worker), snapshot the exporter in stop() before closing, and guard finishReplay against a double swap-back on a null provider.
The embedded packet's publish timestamp mixed clocks: capture came from the recorded capture_ns (shifted into the TSS base) while publish came from the replay run's NetworkTablesJNI.now(), so publish - capture across the two epochs was garbage for any latency consumer. Derive publish as shifted-capture + the result's own processing latency, so the exporter reads no wall clock and is deterministic. Drop the now-unused clock injection constructor and update the exporter tests to pin the invariant.
startsWith accepted equality, so a terminal ".." or "." segment resolved to the root (or a camera dir) itself and passed — a delete request with recordings=[".."] could wipe the whole recordings tree. Reject any segment that is ".", "..", absolute/drive-relative, or contains a path separator, and reject a resolved path equal to the root. Replace the assertion-free illegal-char test with real assertThrows coverage plus cases for the new rejections.
exportCamera and exportAll wrapped ctx.result(InputStream) in
try-with-resources and deleted the temp zip in a finally, but Javalin
writes the stream asynchronously, so the stream was closed and the file
deleted before Jetty sent the body ("Stream Closed" client-side) — the
exact bug the individual handler documented and worked around by buffering
the whole zip into heap (an OOM risk on coprocessors). Route all three
through one sendZipDownload helper that opens the stream DELETE_ON_CLOSE
and lets Jetty close it. Return 404 (not 500) when a camera has no
recordings.
Return 400 when the delete request omits the recordings key (it NPE'd into a 500), report deleted names with String.join instead of a raw String[].toString(), and republish full settings after delete and nuke so the client's recordings list refreshes instead of listing entries that no longer exist. Also sanitize the results-download filename as a single unit so a name with a quote can't break the Content-Disposition header.
A merge dropped fpsLimitPublisher.close(), inputSaveImgEntry.close(), and outputSaveImgEntry.close() from PhotonCamera.close() while the fields stayed live, leaking those NT publishers past close(). Restore them and remove the duplicate pipelineIndexRequest.close() while here.
Match the class's own getDriverMode precedent and the C++/Python bindings (GetRecording / getRecording). New API in this PR, so nothing external depends on the old name.
The recordingEntry BooleanTopic was declared in the Java and C++ NTTopicSet but never assigned or read anywhere. Remove it, and change the recordingRequest Subscribe(0) to Subscribe(false) in the C++ header to match Java/Python and the adjacent SetDefault(false).
…uard pv-delete-modal exposes onBackup, not backup, so the nuke dialog's "Backup Data" button never rendered — bind :on-backup. Move the selection-seeding side effect out of the camerasWithRecordings computed into an immediate watch so the computed stays pure. Guard the auto-download watch with a per-session key so one replay end can't download its results twice.
Each useReplayStatus() caller opened its own 250ms poll and held its own state, so the settings page double-polled and state died on unmount. Hoist the active ref, fetch, and interval to module scope with a subscriber refcount: polling starts on the first subscriber and stops on the last. Add a staleness cap so a crashed backend clears the replay banner after a run of failed polls instead of pinning it forever.
Bring the record + replay branch current with main. Notable upstream changes integrated: - JDK 25 build toolchain. - Jackson -> Avaje Jsonb migration (PhotonVision#2503): JacksonUtils removed. Ported RequestHandler's recording/replay endpoints to Jsonb.instance().type() with @JSON request records and a @JSON ResultHeader for the results listing; JsonProcessingException -> io.avaje.json.JsonException. - Per-camera isEnabled/disabled NT topics (PhotonVision#2484, PhotonVision#2499): merged the new enabled supplier/consumer + isEnabledListener alongside the replay publishers in NTDataPublisher, and the enabled subscriber/publisher in PhotonCamera (Java + C++), keeping recording/replay wiring intact. - Rubik -> generic TFLite OD refactor (PhotonVision#2516): RequestHandler import. Conflicts resolved in NTDataPublisher, VisionModule, VisionRunner, RequestHandler, and PhotonCamera (.java/.cpp/.h). spotless, JDK 25 compile across all modules, and the replay JUnit suite are green.
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
2 participants
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.
Summary
Adds end-to-end file-log recording and in-place replay to PhotonVision so
robot-side vision pipelines can be tuned against captured frames after a match
without re-running the original camera. Builds on (and supersedes) the recording
work in #2183.
Recording
FrameRecorderwrites a JPEG image-sequence (frames/000000.jpg…) plus ametadata.jsonlsidecar ({"seq":N,"capture_ns":T}) atomically, one frameper file. Chose this over an MP4/AVI container because:
recording/recordingRequestNT topics, orfrom robot code through new
PhotonCamera.setRecording(boolean)/isRecording()(Java, C++, Python).tss.jsonis written alongside the sidecar at recording start, capturingthe time-sync-server offset so per-pipeline JSON exports can later place
capture_nsinto the TSS time base for AKit replay.Replay
FileLogFrameProviderdecodesframes/+metadata.jsonlback into thevision pipeline at the recorded cadence, propagating
capture_nsverbatiminto
Frame.timestampNanosso downstream NT pose observations carry theoriginal capture timestamp.
VisionModule.startReplay(recording)swaps the liveFrameProvideron the runningVisionSource, runs the pipeline against therecorded frames, and swaps back at EOF (or on explicit cancel). No second
VisionModuleinstance, no parallel runner.JsonResultExportertees eachCVPipelineResultto a per-pipeline-hash.jsonfile under the recording directory while a replay is active, so theuser can re-tune pipeline parameters and diff result sets across runs.
NT contract additions
recording,recordingRequest,isReplaying,replayProgressCurrentFrame,replayProgressTotalFrames,replayProgressRecordingName.UI
recordings, inline processed-stream preview during replay, per-row Replay
button + results dropdown with auto-download on replay end.
cameras, polled through a `useReplayStatus` composable against a new
`GET /api/recordings/replay/status` endpoint.
Server
`export`, `delete`, `nuke`) plus replay endpoints (`replay`,
`replay/cancel`, `replay/status`, `results`, `result`). All
user-supplied paths are routed through a new `PathSafety.safeResolve`
helper to block traversal.
Testing
parse-and-pace path, the JSON exporter's TSS-shifting math, and the
end-to-end record → replay → export round-trip (`JsonResultEndToEndTest`,
`FrameRecorderTssSnapshotTest`, `FileLogFrameProviderTest`,
`MetadataSidecarReaderTest`, `VisionSourceFrameProviderSwapTest`, …).
recorded-source enumeration / filter path.
USB camera: record → replay → in-browser preview → JSON download.
Test plan
shows progress, banner appears, results dropdown populates on EOF
`recording` topic and lights up the indicator
under a new hash)
tss.json}` for every recording
Notes for reviewers
/ `setFrameProvider` so every camera type (USB, libcamera, file, test)
shares the same swap surface — no per-source code paths.
attempted-true) because it is called from the NT listener thread; throwing
would propagate into the NT4 listener pool. Same fix applied to
`FileVisionSource` / `LibcameraGpuSource`.