feat: extract and cache a still frame from video files - #356
Merged
Merged
Conversation
lstein
force-pushed
the
lstein/feature/video-frame-extraction
branch
from
August 17, 2026 00:33
49d8609 to
4986ee1
Compare
4 tasks
Adds the engine that turns a video into a (PIL frame, VideoInfo) pair, plus the on-disk cache that keeps that frame around for the grid tile, slideshow poster and UMAP thumbnails. Nothing calls either yet — the directory walk still collects images only — so this changes no behavior. video.py drives the bundled ffmpeg binary directly rather than going through imageio_ffmpeg.read_frames, because read_frames owns a Popen it never exposes: a single wedged ffmpeg on a truncated file or a stalled network mount would hang indexing forever, uncancellably. subprocess.run(timeout=) is the one construct that hard-kills a child on every platform. It is also specifically not Popen + wait(timeout=): a PNG frame is megabytes and the pipe buffer is ~64 KB, so ffmpeg blocks writing while the parent blocks waiting, and the pair deadlocks before the timeout can fire. The frame is taken 1.0s in, since frame 0 of consumer video is so often black or a fade-in, with a fallback to the first frame for videos shorter than the seek. Dimensions come only from the decoded frame — the banner parser has no opinion on them at all, because ffmpeg autorotates on decode and a portrait phone video's banner reports the pre-rotation size. The cache lives in the per-user cache directory rather than beside the index. An album's .npz frequently sits inside the album's own image folder, so a sibling video_frames/ would be inside the scanned tree — and stills are full-resolution, so every one would be re-indexed as a photo and given a still of its own. FRAME_CACHE_DIRNAME is registered in EXCLUDED_SCAN_DIRS as belt-and-braces. Fixtures are five committed files totalling 7.5 KB, each purpose-built: clip.mp4 is red then blue so a test can prove the seek landed past frame 0; short.mp4 is shorter than the seek; rotated.mp4 carries a display matrix so its banner and decoded sizes disagree; broken.mp4 is truncated, which cannot be generated at test time. They live in test_media/ rather than test_images/ because new_album copies every file out of the latter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/feature/video-frame-extraction
branch
from
August 17, 2026 01:07
4986ee1 to
087a77e
Compare
lstein
changed the base branch from
lstein/chore/video-extension-taxonomy
to
master
August 17, 2026 01:09
Adversarial review of this PR found 15 issues, most reproduced empirically
against the bundled ffmpeg. Several were in claims the docstrings made.
Resolution was read AFTER thumbnail(), which resizes in place — so every video
over 2048px recorded the poster's size as its own. A 4K clip reported
2048x1152 instead of 3840x2160, and the test asserting info matched frame.size
locked the bug in. Dimensions are now captured before the downscale; verified
end to end: poster (2048,1152), reported 3840x2160.
The banner parser searched raw stderr, but ffmpeg prints the file's own tag
block BEFORE its own Duration and Stream lines. A clip tagged
comment="Duration: 12:34:56.00" parsed as 45296 seconds instead of 2, and a
tag could equally override codec and fps — for any downloaded video. Tag
blocks are now stripped structurally, by indentation, so a value containing
embedded newlines cannot fake a report line either.
_CONTAINER_RE was greedy and backtracked to the LAST ", from " on the line, so
a video under a path like "/photos/Trip, from Rome/" spliced the user's
absolute filesystem path into the container field and into browser-visible
data. Now non-greedy.
"Never raises" was enforced only around Image.open/load: convert("RGB"),
thumbnail() and the banner parser all sat outside the try, and each can raise
something that is not an OSError. Since the caller hands the frame straight to
encoder.encode_images, an escape took a whole batch of unrelated photos with
it. All of it is inside the try now.
Audio files with cover art present that art to ffmpeg as an "(attached pic)"
video stream, so an .ogg music file would have had its album cover
CLIP-embedded and shown as a phantom video slide with a play badge. Detected
and skipped.
Anamorphic sources — DVD rips, .vob, .mpg, AVCHD — extracted geometrically
squashed, because ffmpeg autorotates but does not apply the sample aspect
ratio: a 720x480 SAR 32:27 clip came out at aspect 1.50 instead of 1.778. That
distorted still is what CLIP embeds, degrading search for the whole class. A
scale=iw*sar:ih filter fixes it; verified 1.500 -> 1.777. The expression
deliberately contains no commas, which inside a filtergraph would need
escaping.
A timeout on the seek attempt aborted both attempts, because _run_ffmpeg
collapsed timeout, missing binary and OSError into one None — so the cheaper
no-seek fallback never ran, in exactly the case it was most likely to help. A
sentinel now distinguishes "nothing to run" from "this attempt failed".
ffmpeg_exe() cached its result in both directions: a transient fork failure
disabled video for the process lifetime, and get_ffmpeg_exe() returns
$IMAGEIO_FFMPEG_EXE unvalidated (and can return the bare name "ffmpeg"), so a
non-None answer was not evidence anything could run. Negative results are no
longer memoized and positive ones are resolved and checked.
In the cache: key_for's OSError guard covered only stat(), while resolve()
raises RuntimeError on a symlink loop (3.10-3.12) and a non-UTF-8 filename
makes encode() raise UnicodeEncodeError — both escaped every caller. discard()
with the default mtime was a guaranteed no-op for its main use, since the
source is already deleted by then; the key is now <path digest>-<mtime digest>
so discard globs every generation of a path. The temp name was per-process but
not per-thread and store() took no lock, so two writers could interleave into
one file and publish the mixture — and get() treated mere existence as a hit,
making a torn JPEG permanent. Now mkstemp, and get() rejects zero-length
entries. store() caught only OSError where the atomic_savez it claims to
mirror catches broadly, so a PIL ValueError escaped past ensure().
ensure() recomputed the key from a live stat four times in one call, so a
video whose mtime changed mid-call was locked under one key and stored under
another; it also held the lock across extraction with no negative caching, so
an unextractable video re-spawned ffmpeg on every request. Key computed once,
failures remembered, lock keyed by album+key rather than key alone.
Dropped "video_frames" from EXCLUDED_SCAN_DIRS. It looked like free insurance
but the walk prunes on a bare name match, so a user's own folder of that name
would stop being scanned and its already-indexed photos would be dropped from
the .npz with no log line. Keeping the cache in the per-user cache directory
is the actual defense.
Album keys are no longer validated against a blocklist but used to *construct*
a safe directory name. The blocklist missed "." (which aimed clear() at the
shared root, wiping every album's stills) and Windows drive-relative names.
Also: prune() collects orphaned .tmp files, clear() no longer aborts its sweep
on the first undeletable file, the lock and failure maps are bounded via the
existing BoundedLRU, and the path digest casefolds to match
embeddings._path_compare_key.
Tests: 27 new, including the hostile-banner cases, cover art, the deleted-file
discard, torn and zero-length entries, negative caching, symlink loops and
undecodable filenames. Backend 563 passed, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
enabled auto-merge (rebase)
August 17, 2026 01:59
5 tasks
CI failure on windows-latest / Python 3.14 only; windows / 3.10 passed, which is the tell. test_a_transient_probe_failure_is_not_cached used a hardcoded "/usr/bin/ffmpeg" as its stand-in binary. ffmpeg_exe() resolves a candidate that is not absolute through shutil.which(), and Python 3.13 changed ntpath.isabs() so a leading slash with no drive letter is no longer absolute on Windows. So on 3.14 the literal stopped being absolute, went down the which() branch, found nothing, and the function correctly returned None — the CI log shows exactly that: "ffmpeg reported as '/usr/bin/ffmpeg' but is not executable". The product behaviour was right in both cases; only the test's assumption was wrong. It now uses a real file under tmp_path, which carries a drive on Windows and is therefore absolute on every version — and being a real file, it also drops the global monkeypatch of Path.exists, which was patching a builtin for every subsequent assertion in the test. Also asserts the probe was actually called twice, so the test would fail if the retry silently stopped happening, and restores the cache in a finally so a failure cannot leak a stubbed binary into later tests. Separately, the symlink-loop test now skips rather than errors where symlink creation needs privileges, which is the normal state on Windows without Developer Mode. It happened to pass on the runner, but nothing guaranteed it. Backend 563 passed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
PR 2 of 8 in the video-support stack. Stacked on #355. No behavior change — nothing calls this yet.
Adds the engine that turns a video into a
(PIL frame, VideoInfo)pair, plus the on-disk cache that keeps that frame for the grid tile, slideshow poster and UMAP thumbnails.Why the ffmpeg binary is driven directly
imageio_ffmpeg.read_framesis a generator owning aPopenit never exposes, so there's no portable way to kill a wedged ffmpeg — one truncated file or stalled network mount would hang indexing forever, uncancellably.subprocess.run(timeout=)is the one construct that hard-kills a child on every platform.It is also specifically not
Popen+wait(timeout=): a PNG frame is megabytes and the pipe buffer is ~64 KB, so ffmpeg blocks writing while the parent blocks waiting, and the pair deadlocks before the timeout can fire.run()goes throughcommunicate(), which drains both pipes concurrently. (SIGALRMisn't an option — absent on Windows, unusable off the main thread.)count_frames_and_secsis likewise avoided: it decodes the entire file to count frames.Frame selection
Seek 1.0s in (frame 0 of consumer video is so often black or a fade-in), with
-ssplaced before-iso it's an O(1) input seek. Falls back to the first frame for videos shorter than the seek. Max two spawns.Dimensions come only from the decoded frame. ffmpeg autorotates on decode, so a portrait phone video's banner reports the pre-rotation size. The banner parser has no width/height code path at all, making this structural rather than merely tested.
Cache location
The per-user cache directory, not beside the index. An album's
.npzfrequently sits inside the album's own image folder, so a siblingvideo_frames/would be inside the scanned tree — and stills are full-resolution, so every one would be re-indexed as a photo and given a still of its own.FRAME_CACHE_DIRNAMEis registered inEXCLUDED_SCAN_DIRSas belt-and-braces. (The pixel gate is no defence here:min_image_dimensionis user-editable down to 1.)Keyed on
blake2b(resolved path + mtime), so an edited video invalidates for free. Atomic write + per-key lock, so a grid painting many tiles spawns ffmpeg once per video.ensure()re-extracts on a miss, so a wiped cache self-heals.Fixtures
Five committed files, 7.5 KB total, each purpose-built:
clip.mp4short.mp4rotated.mp4clip.webmbroken.mp4They live in
test_media/rather thantest_images/becausenew_albumcopies every file out of the latter, which would break the exact-count assertions intest_umap/test_invokeai_board_index, failtest_index'sbad_files == []ordering test, and make every existing index test depend on ffmpeg.Tests: 51 new (
test_video_probe.py,test_video_cache.py) covering banner parsing without a binary, the seek, the fallback, rotation, timeout, a missing binary, concurrentensure, and prune/clear. Full suite: 534 passed.ruff checkclean.🤖 Generated with Claude Code