feat: play/duration/fps badge on video stills - #358
Merged
Conversation
lstein
force-pushed
the
lstein/feature/serve-video-media
branch
from
August 17, 2026 00:33
779960e to
af92621
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 00:33
683cf6b to
4aa8cf5
Compare
lstein
force-pushed
the
lstein/feature/serve-video-media
branch
from
August 17, 2026 01:56
af92621 to
3c96dc2
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
2 times, most recently
from
August 17, 2026 02:53
0c8ec09 to
2888a99
Compare
Adds the endpoints and slide payload a video needs, ahead of the directory
walk actually collecting any. Still no behavior change for existing albums:
SlideSummary's new fields default to the pre-video values, so an image
payload is byte-identical to what it was.
/videos/{album_key}/{path} is a separate route rather than a widened /images/
allowlist. SUPPORTED_EXTENSIONS guards serve_image against the
add_album(image_paths=["/etc"]) -> GET /images/<key>/passwd arbitrary-file-read
chain, and widening it to admit videos would have loosened that guard as a
side effect. Two routes, two allowlists; a test asserts the split holds in
both directions, and test_image_type_guard.py is unchanged.
The route returns a FileResponse specifically, because Starlette implements
HTTP Range on it and that is what lets the <video> scrubber seek. Nothing in
PhotoMapAI implements Range itself and no test anywhere covered it, so
rewriting this route as a StreamingResponse (as the HEIC path does) would
silently break seeking with no other symptom. There are now explicit 206 and
Content-Range assertions.
serve_thumbnail resolves a video to its cached still before opening it, which
is what lets the grid, UMAP hover popups, landmark overlays, the back flyout
and the reference-thumbnail strip display videos with no changes of their own
— they are all already index-based. /video_frame/ serves the full-size still
for the slideshow poster. Both go through VideoFrameCache.ensure, so a wiped
cache regenerates rather than leaving broken images.
create_slide_url points image_url at the still and puts the playable bytes in
a new video_url field, so consumers that just want a picture keep working.
The drawer gets a Video panel (duration, fps, resolution, codec, container)
and keeps the EXIF panel underneath — phone videos routinely carry a creation
date and GPS worth showing. Videos never get the "Use as Ref Image" button:
it uploads the file to InvokeAI as a reference image, and handing it an .mkv
is a live bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review of this PR found 15 issues. The worst were not in the
serving logic itself but in what it does to the process around it.
VideoFrameCache.ensure() can spawn ffmpeg and wait up to two full timeouts,
and it was being called directly from `async def` handlers — which FastAPI
runs on the event loop, not in its threadpool. With uvicorn started as a
single worker, one slow or unreadable video froze every request in the
process. Both call sites now go through asyncio.to_thread, the convention
already used for the indexer and cluster labels. Measured before/after with a
real ASGI client: an unrelated request that previously waited behind a 3s
extraction now waits 0.002s.
Four independent crash paths, all reachable from parsing ffmpeg's stderr and
all surfacing as a 500 on /retrieve_image — a blank slideshow, not a missing
field:
- format_fps/format_duration put round() outside their guard, so inf raised
OverflowError and nan raised ValueError. inf is reachable because the
banner's numeric patterns match arbitrarily long digit runs and
float("9" * 400) is inf with no exception.
- _resolution screened falsy values but not unparseable ones, so a width of
"1920.0" raised out of int().
- A non-dict under the reserved key — an older or hand-edited index holding a
JSON string — made the formatter's .get raise AttributeError.
- Even with all of those fixed, a non-finite float on the response model still
500s, because Starlette serializes with allow_nan=False. That is a separate
site from the formatters and needed its own fix at the boundary.
The probe dict is now sanitized once: non-dict rejected, non-finite values
dropped, and the result copied — it previously aliased the dict owned by the
lru_cached npz view, so the response model shared mutable state with the
process-wide index cache.
The drawer panel was wrapped in `video-metadata`, which no stylesheet targets.
metadata-drawer.css styles `.exif-metadata table`, and since the indexer
writes only the video dict this panel is normally the sole panel, so nothing
else pulled the styling in — every video's drawer rendered a borderless,
unpadded table. It now carries the class the drawer actually styles.
serve_thumbnail resolved the still before checking whether the derived PNG was
already cached, so every repaint of a grid of N videos paid the expensive path
N times, and a transient extraction failure 404'd even when a good thumbnail
was on disk. Reordered.
A failed extraction now degrades to a placeholder tile rather than a 404. All
five img.src callers set no onerror handler, so a 404 painted bare
broken-image glyphs across the grid, UMAP hover popups and landmark overlays
at once — and on any platform with no ffmpeg binary that is every video.
URLs are percent-encoded. "beach #2.mp4" made "#2.mp4" a fragment, so the
server saw "videos/<key>/beach " and 404'd; "?" started a query string and a
literal "%" read as a broken escape. html.escape on the drawer link is a
different encoding and does not help.
A NUL byte in the path made Path.resolve() raise ValueError inside
validate_image_access, escaping as a 500 with a traceback instead of the
403/404 the route is designed to return.
Cache headers: /videos/ gets an explicit lifetime, because FileResponse emits
ETag and Last-Modified but implements no conditional handling (only
StaticFiles does), so a revalidation re-transferred the whole clip.
/video_frame/ is no-cache, because it is keyed by index and an index
designates a different file once a delete or reindex reorders the album.
In video_cache: the failure record introduced by the previous review was
permanent and not album-qualified, so one transient ffmpeg failure blanked a
video's tile and poster for the life of the process, across every album
holding that file. It now expires and is scoped like the lock.
Test hygiene: the tests wrote into — and clear()'d from — the developer's real
~/.cache/photomap/video_frames. conftest now redirects the cache root per
test. test_thumbnail_endpoint_still_works_for_images was gated on ffmpeg
despite requesting the .jpeg, so the only guard that the video branch did not
break image thumbnailing skipped silently wherever no binary is installed. And
the fixture's add_album assert sat outside its try, so a setup failure leaked
the album into every later test.
Tests: 50 new. Backend 626 passed, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/feature/serve-video-media
branch
from
August 17, 2026 02:58
3d4e637 to
c33e03c
Compare
Draws a play button with the duration and frame rate over a video's still, in
both the swiper and the grid. Clicking it dispatches a videoPlayRequested
window event; the player that consumes it lands in the next PR, so nothing
opens yet.
The poster underneath stays a real <img>, which is load-bearing rather than
cosmetic. search-ui.js ("search by this image") reads
slide.querySelector("img")?.src, curation.js skips slides without an img, and
grid-view.js updates alt text the same way — keeping an <img> means all three
work unchanged, and "search by this image" then searches by the very frame
that was embedded.
In the swiper the badge is applied inside addSlideByIndex rather than by a
later sweep: slides are also built by prependSlide, _doResetAllSlides and
seekToSlideIndex, and a separate pass would miss all of those. On touch
devices it is appended to the slide itself, deliberately outside
.swiper-zoom-container, or Swiper's zoom module would scale the badge along
with the image on a pinch. In the grid it goes in updateSlideWithMetadata,
the point at which a placeholder tile learns its media type.
The badge swallows mousedown, dblclick, touchstart and touchend as well as
click, because three separate gestures would otherwise fire underneath it:
attachDoubleTapHandler flips to grid view, the grid's inline onclick
re-selects the tile, and touch.js toggles the slideshow on any tap while in
fullscreen.
Containers browsers generally cannot play are styled muted and carry an
explanatory title, but still get a working badge — the video is indexed,
searchable and mapped either way, and the player explains and offers a
download.
Tests: 44 unit tests for the badge module (which imports nothing, so it needs
no mocking), 7 swiper integration tests covering append, prepend and the
touch path, and 2 grid tests. Frontend 479 passed, backend 576 passed, eslint
and prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported from smoke testing: in the swiper view the play button and fps label were unreachable — the control panel sat on top of them. Cause: the badge was bottom-left at z-index 100, and #controlPanel is position:fixed bottom:10px left:20px at z-index 4000. Directly underneath, in both position and stacking order. This is also why it worked in the grid and not the swiper: a swiper slide fills the viewport, so the slide's bottom-left corner IS the screen's bottom-left, whereas a grid tile is small and rarely sits in that corner. Every corner is spoken for — score display top-left, bookmark star top-right, control and search panels along the bottom — so the badge moves to the centre of the picture rather than to another corner. The poster is object-fit: contain, which centres it within the slide, so the centre of the slide is also the centre of the image, including for a portrait video letterboxed on a landscape screen. The play button and the duration/fps label are stacked vertically and centred together. To keep the centred badge from creating a wide dead zone for the double-click-to-grid gesture, the button itself is pointer-events:none with the icon and label opting back in — so only the glyph and the pill take clicks, not the whole bounding box. Grid tiles get a smaller icon, and tiles under 140px drop the label entirely. That threshold is applied in JS from the tile's own inline width rather than in CSS: tile size varies continuously (200 * gridThumbSizeFactor, clamped 75-300), so no fixed selector could match it, and the previous attempt only caught three exact widths. Swiper slides carry no inline width and are therefore never compact. Tests: 3 new for the compact threshold. Frontend 482 passed, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 02:58
2888a99 to
ad612e5
Compare
Adversarial review of this PR found that the tests written to protect the compact badge could not observe it — which is how dead CSS shipped. That is the root of most of what follows. The .video-badge--compact rules never applied to the icon. Verified in headless Chrome: the compact icon computed to 42px, not 30px, because `#gridViewSwiperWrapper .video-badge-icon` is specificity (1,0,1) and beats `.video-badge--compact .video-badge-icon` at (0,2,0). The label correctly hid, so the badge stayed full-size exactly where it most needed to shrink. The compact rules are now scoped under the same wrapper. Now verified at 30px. The grid tests could not have caught it: their fixture assigned innerHTML directly to `.swiper.grid-mode`, so the tile never sat inside #gridViewSwiperWrapper — the scope for every grid rule this PR adds — and carried no inline width, which is the only input isCompactSlide reads. Both tests would have passed with the compact path deleted outright. The fixtures now mirror a real tile, and three tests cover the paths they could not reach. applyVideoOverlay was idempotent but not reconciling: it returned early if any badge was present. A tile's DOM node can outlive the item it showed, because the grid fetches metadata in a staggered background loop and a response can land after the album changed — so a tile that became an image kept offering to play a photo, and a tile that became a different video showed the previous clip's duration and played the previous clip's URL. It now reconciles against the payload's identity. aria-label suppressed the very text the badge exists for. It REPLACES element contents in the accessible-name computation, and the icon is aria-hidden, so a screen reader announced "Play clip.mp4" and never the duration or frame rate. The unplayable branch was worse — a filename-less generic string, so every unplayable tile in a grid announced identically. The name is now built explicitly from both parts. The badge was the only tabbable button in the app. setupAccessibility() takes every button out of the tab order, but runs once at init over buttons that exist then; a dynamically created badge missed it. Tabbing to one inside a slide triggers Swiper's slide-to-focused-element, and a focused button loses Space to the global slideshow shortcut, which preventDefaults it. Now tabIndex -1, matching the app, and blurred after click for the same reason the radio controls are. The unplayable state was not perceptibly muted. Measured over a mid-grey frame, the "dimmed" disc was actually slightly brighter than the playable one, leaving a dashed 1.4-unit stroke as the only real signal — about 3px of dash at the grid's icon size. It is now amber with a slash that scales with the icon. The label rendered in the UA form-control font: a <button> does not inherit the page font and this app has no global button reset, so on Windows every label used Segoe UI except the one in the middle of the picture. Verified now inheriting. Also: src and alt are assigned as properties rather than interpolated into slide markup. A filename needs only a double quote to escape the attribute — `evil" onerror="…` renders as a live event handler. Pre-existing, but this PR edits that exact block. And the touch test restored a process-wide singleton only on its success path, so one failure would have cascaded into unrelated tests through the .swiper-zoom-container branch. Moved to afterEach, restoring the captured original rather than a hardcoded false. Tests: 79 across the three badge files, up from 53. Frontend 492 passed, backend 626 passed, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
enabled auto-merge (rebase)
August 17, 2026 03:41
auto-merge was automatically disabled
August 17, 2026 03:49
Rebase failed
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 4 of 8 in the video-support stack. Stacked on #357.
Draws a play button with duration and frame rate over a video's still, in both the swiper and the grid. Clicking dispatches a
videoPlayRequestedwindow event — the player that consumes it is PR 5, so nothing opens yet. That seam keepsvideo-badge.jsfree of any dependency on the player, and it imports nothing at all, which is why its 44 unit tests need no module mocking.The poster stays a real
<img>Load-bearing, not cosmetic. Three existing call sites reach for it:
search-ui.js— "search by this image" readsslide.querySelector("img")?.srccuration.js— skips slides with noimg(frequency highlighting)grid-view.js— updates alt text the same wayKeeping an
<img>means all three work unchanged. It also means "search by this image" on a video searches by the exact frame that was embedded, which is the semantically right answer.Where the badge is attached
Swiper: inside
addSlideByIndex, at slide construction. Slides are also built byprependSlide,_doResetAllSlidesandseekToSlideIndex— a badge added by a later sweep would be missing on all of them. (bookmarks.jsbadges grid slides only, so there was no existing precedent for a swiper-slide badge to follow.)On touch devices it is appended to the slide itself, deliberately outside
.swiper-zoom-container, or Swiper's zoom module scales the badge along with the image on a pinch. There's a test for that.Grid: in
updateSlideWithMetadata— tiles paint as placeholders first, so that's the earliest point the media type is known.applyVideoOverlayis idempotent because tiles get upgraded in place. (NotemakeSlideHTMLis dead code — only the placeholder path is live.)Gesture collisions
The badge swallows
mousedown,dblclick,touchstartandtouchendas well asclick. Three separate things would otherwise fire underneath it:attachDoubleTapHandlerflips to grid view behind the freshly-opened playeronclick="handleGridSlideClick(i)"re-selects the tiletouch.jshandleTouchEndtoggles the slideshow on any tap in fullscreenUnplayable containers
.avi/.mkv/.wmvget a muted badge and an explanatorytitle, but still a working one — the video is indexed, searchable and mapped either way, and the player (PR 5) explains and offers a download. Theplayableflag is a styling hint only; actual playability depends on the codec inside, so the player always attempts playback and reacts to the element's ownerrorevent.Positioned bottom-left, clear of the bookmark star (top-right) and the score pill.
Tests: 44 badge unit tests, 7 swiper integration tests (append, prepend, touch path, no-badge-for-images), 2 grid tests. Frontend 479 passed / 34 suites; backend 576 passed; eslint + prettier clean.
🤖 Generated with Claude Code