Skip to content

Play any video: on-demand transcoding, and a lightbox video player - #385

Open
lstein wants to merge 4 commits into
masterfrom
lstein/feature/video-player-and-transcoding
Open

Play any video: on-demand transcoding, and a lightbox video player#385
lstein wants to merge 4 commits into
masterfrom
lstein/feature/video-player-and-transcoding

Conversation

@lstein

@lstein lstein commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Two video problems, fixed together because the second one needs somewhere in the first one's UI to report itself.

1. The player was a dialog wrapped around a picture

.modal-content is a #222 panel with 2em of padding and its own rounded border. Wrapping that around a black <video> — itself a bordered rectangle — read as a frame inside a frame, and the title row above it added a second band of chrome.

It is now a lightbox modelled on InvokeAI 7's preview stage: a bordered, rounded, shadowed media card that shrink-wraps the video at whatever aspect ratio the clip has, sitting directly on the backdrop, with the filename as a caption below and the close button floating over the corner.

It also sat ~40px low on iPad. .modal-overlay is position: fixed; height: 100vh, and after a fullscreen exit iPadOS keeps the taller fullscreen-sized layout viewport — so both resolve against a viewport that is partly off-screen and the flex centring lands below the middle of it. The modal now consumes --visible-viewport-height, the variable panel-anchor.js already publishes for exactly this and that swiper.css already uses for the photo itself. Verified on the device.

Note that every .modal-overlay has this bug; only the video player was reported, so only it was changed. The fix may as well move up into modal-base.css the next time a mispositioned dialog is reported on iPad.

2. Most of a real video collection would not play

DivX/XviD .avi, MPEG-2 .vob and .mpg off DVDs, .wmv, HEVC and 10-bit H.264 in .mkv — <video> shows a black rectangle for all of them, and the player's only answer was a download link. New photomap/backend/video_transcode.py converts them to one H.264/AAC MP4 in the per-user cache directory, which the existing FileResponse path then serves with Range support — so the result is fully seekable and instant on every replay.

Two new endpoints, both behind the same album-access guard as /videos/ (extracted into a shared _resolve_album_video, byte-identical to the checks it replaced):

  • POST /prepare_video/{album}/{path} — idempotent; starts the conversion, reports progress, and doubles as the poll.
  • GET /transcoded_video/{album}/{path} — serves the result.

Design notes worth knowing:

  • Remux before re-encode, decided per stream. Much of that collection is packaged wrongly rather than encoded wrongly: H.264/AAC in an .mkv only needs its streams copied into an MP4, seconds rather than minutes. H.264 + AC-3 copies the video and re-encodes only the audio.
  • Every unknown re-encodes. An unparsed codec or pixel format means we cannot prove a copy would play, and guessing wrong reproduces the exact black rectangle this removes.
  • A file, not a pipe. A StreamingResponse off ffmpeg's stdout starts faster but has no Range support, so the scrubber cannot seek, and every replay pays again.
  • Jobs live only while somebody is watching. Conversions run one at a time; the player's progress poll is also the liveness signal, and a job nobody has polled for 45s is killed. Closing the player is what that looks like from the backend.
  • The cache is byte-bounded (8 GB, LRU), swept against the index, and discarded per-image on delete — all three hooks are needed, since the byte budget only runs when some other conversion finishes.

Triggered automatically: a known-bad container skips the black-rectangle-then-error round trip, anything else is attempted first and converted on the <video> element's own error — which is what catches the cases no container list predicts, like HEVC in Firefox.

3. One play badge for every video

The amber, dashed, slashed badge variant described a real limitation right up until this branch removed it. It now advertises a restriction that no longer exists, most loudly on the files that now work hardest, so it is gone.

Adversarial review

Three fresh-context reviewers attacked the first commit; 17 confirmed findings are fixed in 836bdc1, each with a regression test. The ones worth calling out:

  • Copy safety was defeatable by the file itself. Two pieces of file-controlled text are printed on ffmpeg's own report lines, outside the Metadata: blocks being stripped: a stream's language tag (inline, before the : Video: delimiter) and the input path (echoed in the header — filenames may contain newlines). Both forge report lines, and both degrade toward copy. StreamProbe now carries a trusted flag.
  • Every 16:9 NTSC DVD rip failed. scale=iw*sar:ih truncates; 720×32/27 = 853.33, and libx264 refuses an odd width in yuv420p. Now rounds both axes and sets setsar=1.
  • Wrong-movie collision. Reusing VideoFrameCache.key_for inherited its casefold, so clip.mp4/Clip.mp4 with equal mtimes collided — a wrong thumbnail there, the wrong film here.
  • Conversions outliving a deleted album or a deleted video; an unprotected window after Popen leaking an unwatched ffmpeg and an invisible multi-GB .tmp; non-daemon pool threads hanging shutdown ~47s; the sweeper evicting the film currently being watched.
  • Frontend: ~24px sliced off every video below 800px of viewport, and up to 160px of black beside a portrait clip on a landscape screen.

One reported finding was dismissed after checking in a real browser: a stale error from a dismissed clip cannot destroy the next one, because the HTML media-element load algorithm removes pending tasks when src changes. The proof had dispatched the event manually in jsdom, which bypasses that.

Testing

  • 906 backend, 694 frontend, lint clean.
  • End-to-end conversions exercised against real ffmpeg: XviD/MP3 .avi (full re-encode), H.264/AAC .mkv (remux fast path), anamorphic 720×480 SAR 32:27 DVD, audio-only container, unreadable file.
  • Player geometry measured in headless Chromium across six viewport/aspect combinations.
  • iPad behaviour confirmed on the device by the author.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GwzC2csXxZkd9HsD8A2t7h

lstein and others added 4 commits September 3, 2026 14:35
@lstein @claude
feat: lightbox video player, and transcode formats the browser can't …
ded570c 
…play

Two problems with video support, fixed together because the second one
needs a place in the first one's UI to report itself.

**The player was a dialog around a picture.** `.modal-content` is a #222
panel with a 2em pad and a rounded border; wrapping that around a black
`<video>` — itself a bordered rectangle — reads as a frame inside a frame,
and the title row above it added a second band of chrome. It is now a
lightbox, modelled on InvokeAI 7's preview stage: a media card that
shrink-wraps the video at whatever aspect ratio the clip has, sitting
straight on the backdrop, with the filename as a caption below and the
close button floating over the corner.

It also sat ~40px low on iPad. `.modal-overlay` is `position: fixed;
height: 100vh`, and after a fullscreen exit iPadOS keeps the taller
fullscreen-sized layout viewport — so both resolve against a viewport
that is partly off the screen and the flex centring puts the player below
the middle of it. The modal now consumes `--visible-viewport-height`, the
variable panel-anchor.js already publishes for exactly this and that
swiper.css already uses for the photo itself.

**Most of a real video collection will not play in a browser.** DivX/XviD
`.avi`, MPEG-2 `.vob` and `.mpg` off DVDs, `.wmv`, HEVC and 10-bit H.264
in `.mkv` — `<video>` shows a black rectangle for all of them, and the
player's only answer was a download link. `video_transcode.py` converts
them to one H.264/AAC MP4 in the per-user cache directory, which the
existing FileResponse path then serves with Range support, so the result
is fully seekable and instant on every replay.

Notes on the shape of it:

* **Remux before re-encode.** Much of that collection is packaged wrongly
  rather than encoded wrongly: H.264/AAC in an `.mkv` only needs its
  streams copied into an MP4, which takes seconds where a real encode
  takes minutes. The two streams are decided independently, so the common
  H.264-plus-AC-3 case re-encodes only the audio.
* **Every unknown re-encodes.** An unparsed pixel format or codec means
  we cannot *prove* a copy would play, and the cost of guessing wrong is
  the exact black rectangle this removes. Note `yuv420p10le` contains
  `yuv420p`: a substring test would call 10-bit copy-safe.
* **A file, not a pipe.** A StreamingResponse off ffmpeg's stdout starts
  faster but has no Range support, so the scrubber cannot seek, and every
  replay pays again.
* **Jobs live only while somebody is watching.** Conversions run one at a
  time, so opening several unplayable clips in a row would otherwise
  queue them all. The player's progress poll doubles as the liveness
  signal and a job nobody has polled for 45s is killed — closing the
  player is what that looks like from here.
* The cache is bounded by bytes (these are whole movies, not the stills
  next door) and swept LRU after each conversion; deleting an album drops
  its conversions along with its frames.

Triggered automatically: a known-bad container skips the
black-rectangle-then-error round trip, and anything else is attempted
first and converted on the `<video>` element's own error — which is what
catches the cases no container list predicts, like HEVC in Firefox.

The shared mixed image/video album fixture moved to `fixtures.py`, where
conftest already picks fixtures up, so the new suite does not have to
import it from another test module.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwzC2csXxZkd9HsD8A2t7h
@lstein @claude
fix: adversarial-review findings in video conversion and the player
836bdc1 
Three fresh-context reviewers attacked the previous commit. Confirmed
findings, fixed here; each has a regression test.

**Copy safety could be defeated by the file itself.** The rule is that an
unknown stream re-encodes, never copies — but two pieces of file-controlled
text are printed on ffmpeg's *own* report lines, outside the `Metadata:`
blocks that were being stripped. A stream's language tag is printed inline
before the `: Video:`/`: Audio:` delimiter, so a tag of `x): Video: h264
(High), yuv420p` makes an audio line match the video pattern; and the input
path is echoed in the `Input #0 … from '…':` header, so a filename
containing a newline forges whole report lines. Both degraded *toward*
copy, which is the dangerous direction: a copied HEVC 10-bit stream is
exactly the black rectangle this feature removes, cached and reported
ready. `StreamProbe` now carries `trusted`, cleared when a stream line
holds more than one kind delimiter or when the path contains a newline, and
`plan_for` re-encodes everything from an untrusted probe.

**Every 16:9 NTSC DVD rip failed.** `scale=iw*sar:ih` truncates, and
720*32/27 is 853.33 — libx264 refuses an odd width in yuv420p outright, so
the single most common shape the filter exists for died with "width not
divisible by 2" on every retry. Now rounds both axes to even and sets
`setsar=1`.

**Wrong-movie collision.** The transcode cache no longer reuses
`VideoFrameCache.key_for`, which casefolds the path: on a case-sensitive
filesystem `clip.mp4` and `Clip.mp4` with matching mtimes (routine for
anything unpacked from one archive) collided. For a still that is the wrong
thumbnail; here it played the wrong film and reported it ready without
converting it.

**Conversions outlived what they belonged to.** Deleting an album while one
was in flight let it finish and `mkdir` the just-removed directory back,
orphaning a whole movie under a key nothing would ever clear again;
deleting a *video* left a full decodable copy of it in `~/.cache`, which is
the retention the still-cache was written to avoid. Added `forget_album`
(cancel, then clear), `TranscodeCache.discard`/`prune` wired into the
per-image delete and the index-save sweep, and a pre-publish cancellation
check so a conversion that succeeds after cancellation is still discarded.

**Lifecycle and accounting:**

* Everything between `Popen` and the read loop's `try/finally` was
  unprotected — thread exhaustion at `watcher.start()` left ffmpeg running
  unwatched and unreaped with a multi-gigabyte `.tmp` the sweeper could not
  even see (it counts `.mp4`). Now unwound, and the sweeper reclaims aged
  temps.
* The pool's threads are non-daemon and `concurrent.futures` joins them at
  exit, so stopping the server hung for up to ~47s. An `atexit` hook now
  cancels and kills first.
* `_forget_stale_jobs` could not reach queued or running entries and was
  skipped entirely on a cache hit, so a source whose mtime kept moving (a
  file still being copied in) queued a job per second forever.
* Unknown duration took the *tightest* deadline, killing every `.m2v`
  longer than ten minutes of encoding; it now takes the loosest, with the
  stall detector still covering wedged jobs.
* An audio-only `.mkv`/`.mov`/`.ogg` failed with ffmpeg's "Error opening
  output files: Invalid argument"; it now says it has no video track.
* `_stderr_tail` reported ffmpeg's last line, which is always the generic
  muxer post-mortem, hiding the actual cause on the line above.
* `TranscodeCache.get` stamps the entry, and the sweeper spares anything
  touched recently — the film being watched was otherwise the *first*
  thing evicted, 404ing its own next Range request.

**Player:** the video's `94vw` cap exceeded the frame's own max-width below
800px of viewport, so `overflow: hidden` sliced ~24px off the right of every
video on a phone; and the frame's unconditional `min-width` put up to 160px
of black on one side of a portrait clip on a landscape screen. Caps now
agree, the mins are gone (a `<video>` brings its own 300x150 floor) and the
frame centres. Also: a transient network error no longer tears the clip down
and starts a pointless re-encode (the `MediaError` code is consulted); one
dropped poll no longer abandons a conversion, since the poll is also the
liveness signal; focus no longer lands on `<body>` when no conversion is
available; reopening with no URL releases the previous clip instead of
leaving it playing unstoppably behind the panel; and an indeterminate
progress bar no longer publishes `aria-valuenow="0"`.

Dismissed after checking in a real browser: a report that a stale `error`
from a dismissed clip could destroy the next one. The HTML media element
load algorithm removes pending media element tasks, so replacing `src`
discards the queued event — verified in Chromium (no event fired, while the
control case fired code 4). The proof had manually dispatched the event in
jsdom, which bypasses that.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwzC2csXxZkd9HsD8A2t7h
@lstein @claude
fix: one play badge for every video, now that any of them can be played
b3195ab 
The amber, dashed, slashed badge variant for containers no browser decodes
described a real limitation right up until the previous commit removed it:
clicking one used to reach a dead player offering a download. It now opens
the player, converts the file and plays it, exactly like any other video —
so the variant was advertising a restriction that no longer exists, and
doing it most loudly on the files that now work hardest.

`playable` stays in the badge's payload. The player still reads it to skip
a direct attempt it knows will fail and go straight to converting; it just
no longer changes how the badge looks or what it says.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwzC2csXxZkd9HsD8A2t7h
@lstein @claude
feat: download the current image or video from the control panel
e8900fe 
There was no way to save the item you are looking at without bookmarking it
first, opening the bookmark menu, downloading, and unbookmarking again.

The download itself already existed and already handled video correctly —
`downloadSingleImage` picks `video_url` over the still frame, derives the
extension from the real path, and streams through an `<a download>` rather
than buffering a 200 MB clip into browser memory. Only a way to reach it was
missing, so this adds a trigger rather than an implementation: the logic moves
to a shared `download.js` that the bookmark menu and the new button both call.

The button sits next to Delete because it is the same shape of action, and it
resolves its target through `getCurrentSlideIndex()` — the same view-agnostic
primitive Delete uses — so one button serves both the swiper and the grid with
no per-view code. Nothing is drawn over the picture.

It is deliberately *outside* the `album_locked` guard that wraps Delete: a
lock protects an album's files from being changed, and saving a copy changes
nothing.

Videos always download as the **original** file, never a converted copy. The
conversion exists so the browser can decode a container it otherwise cannot;
it is a lossy re-encode in the general case, and it is not what is in the
user's library.

Adding the button did not widen the panel — it is a fixed 416px with seven
buttons or eight, measured against master. (That width already overflows a
390px viewport on master; unchanged here, and left alone.)

Also fixes two Windows-only failures in the previous commit's tests, both
test bugs rather than product ones: one compared an ffmpeg argument against a
hardcoded POSIX path, and the case-collision test asserted a premise that only
holds on a case-sensitive filesystem — on Windows the two names are one file,
so sharing a cache entry is the correct answer there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwzC2csXxZkd9HsD8A2t7h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lstein