Skip to content

Create snapshot functionality from History view#22639

Open
nrlcode wants to merge 4 commits intoblakeblackshear:devfrom
nrlcode:snapshot-pr-clean
Open

Create snapshot functionality from History view#22639
nrlcode wants to merge 4 commits intoblakeblackshear:devfrom
nrlcode:snapshot-pr-clean

Conversation

@nrlcode
Copy link
Copy Markdown

@nrlcode nrlcode commented Mar 25, 2026

Please read the contributing guidelines before submitting a PR.

Proposed change

This PR adds snapshot support to the History/Recording player flow.

  • Adds a snapshot action in History player controls.
  • Captures the snapshot from the active playback frame (video element), not from the live snapshot endpoint.
  • Uses playback/timeline timestamp for filename generation (with timezone-aware formatting) so downloaded filenames align with the History timeline context.

This improves user workflows when reviewing recordings and exporting a frame from a specific playback moment.

I tried to re-use the functionality from Frigate+ and keep the scope of changes minimal.

  • VideoControls.tsx: adds snapshot button API/UI.
  • HlsVideoPlayer.tsx: wires button to capture/download in History player.
  • snapshotUtil.ts: playback-timestamp + timezone filename and capture-from-current-video support.
  • DynamicVideoPlayer.tsx: passes supportsSnapshot down.
  • RecordingView.tsx: enables snapshot for History.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code
  • Documentation Update

Additional information

For new features

AI disclosure

  • No AI tools were used in this PR.
  • AI tools were used in this PR. Details below:

AI tool(s) used (e.g., Claude, Copilot, ChatGPT, Cursor):
ChatGPT/Codex

How AI was used (e.g., code generation, code review, debugging, documentation):
Used for implementation assistance, debugging, and refinement of targeted frontend changes.

Extent of AI involvement (e.g., generated entire implementation, assisted with specific functions, suggested fixes):
Assisted with specific functions and targeted fixes. Final scope and selected commits were manually controlled.

Human oversight: Describe what manual review, testing, and validation you performed on the AI-generated portions.
I manually reviewed all changed files, validated commit scope against origin/dev, and tested snapshot behavior in the History viewer. I also ran frontend checks used during iteration (lint/build flow) and validated behavior in containerized test deployment.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I can explain every line of code in this PR if asked.
  • UI changes including text have used i18n keys and have been added to the en locale.
  • The code has been formatted using Ruff (ruff format frigate) [N/A]

setFullResolution: React.Dispatch<React.SetStateAction<VideoResolutionType>>;
toggleFullscreen: () => void;
containerRef?: React.MutableRefObject<HTMLDivElement | null>;
supportsSnapshot?: boolean;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supportsSnapshot is a pure pass-through prop that adds noise here. Since it ends up as snapshot: supportsSnapshot in the features object inside HlsVideoPlayer, the cleaner approach is just to infer it from whether onSnapshot is provided - the same way plusUpload is already gated on onUploadFrame existing. That would eliminate this prop chain entirely.

I would just mirror what we already do with onUploadFrame.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the supportsSnapshot prop chain entirely and now infer snapshot support directly from onSnapshot presence in HlsVideoPlayer (same pattern as other callback-gated features like upload).

const currentTime = videoRef.current?.currentTime;

if (!currentTime) {
if (currentTime == undefined) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change here seems unrelated to snapshots. Am I missing something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. That change was unrelated to snapshot scope. I reverted the logic back to its previous behavior so this PR only carries snapshot-specific changes.

const frameTime = getVideoTime();

if (frameTime && onUploadFrame) {
if (frameTime != undefined && onUploadFrame) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also seems unrelated to snapshots.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this to the previous check to keep scope tight and avoid unrelated behavioral changes in this snapshot PR.

const frameTime = getVideoTime();

if (frameTime) {
if (frameTime != undefined) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also seems unrelated to snapshots.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also reverted for the same reason

});

const safeTimestamp =
timestamp === "Invalid time"
Copy link
Copy Markdown
Collaborator

@hawkeye217 hawkeye217 Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatUnixTimestampToDateTime returns the string "Invalid time" on bad input, and this is meant for UI display. But you are pattern-matching on that magic string as error handling. If that return value ever changes, this silently breaks. I would just validate the input before calling the formatter instead, or don't use a formatter at all (since Live view's snapshot download has no formatting, either).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I removed the check and now validate the input timestamp before calling the formatter (finite-number validation). If invalid/missing, it falls back to current time; if valid, it uses the playback timestamp. This avoids coupling to formatter UI text.

@nrlcode
Copy link
Copy Markdown
Author

nrlcode commented Mar 25, 2026

I pushed a cleanup commit that narrows this PR to snapshot functionality only: removed supportsSnapshot pass-through, restored unrelated time guard changes, and replaced "Invalid time" string matching with explicit input validation in snapshot filename generation.

return currentTime + inpointOffset;
}, [videoRef, inpointOffset]);

const handleSnapshot = useCallback(async () => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need some kind of loading guard that disables the button while the snapshot is being downloaded (like is done in LiveCameraView's snapshot handler), because a user could spam click the snapshot button, which in most cases will cause the browser to just download a bunch of black frames.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the guarding features!

toast.error(t("snapshot.captureFailed", { ns: "views/live" }), {
position: "top-center",
});
if (isSnapshotLoading) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some unnecessary redundancy here, I would remove this guard from the callback entirely and let VideoControls handle it. The onClick in VideoControls already has if (snapshotLoading) return, so the callback never fires while loading. That would match the existing pattern of other things here where the component prevents re-triggering, not the handler.

? "cursor-not-allowed opacity-50"
: "cursor-pointer",
)}
title={snapshotTitle}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this. None of the other buttons here have a title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants