perf(playback): cache embedded text-subtitle extracts - #542
Open
CoffeeKnyte wants to merge 1 commit into
Open
Conversation
Extracting an embedded subtitle track walks the interleaved container:
subtitle packets sit between video and audio across clusters, so
harvesting a few KB of text means demuxing that stretch of a multi-GB
file off CephFS. Over 41.6h the web player saw p95 120,021ms / max
121,470ms on /api/v1/stream/{session_id}/subtitles/{track}, with 45 of
220 fetches over 10s. The 120s ceiling is the server's absolute
WriteTimeout (cmd/silo/main.go:2389) cutting the body mid-flight -- and
because WriteHeader(200) already ran, those truncations are logged
status=200 and are invisible in error metrics.
The server believes a 600s window bounds this. It does not: -t is passed
as an input option and ffmpeg silently ignores it for these extracts, so
every request runs from the seek point to EOF. Measured against the
production binary, `-ss 4000 -t 30` and `-ss 4000 -t 300` are
byte-identical to passing no -t at all (last cue 02:10:04, end of film).
Only -ss works, so cost tracks (duration - seek) x bitrate.
The window cannot simply be turned on. silo-apple and silo-android both
fetch a track once and depend on receiving the whole thing, so bounding
the output would silently kill subtitles ~10min into every film on both
platforms. The accidental whole-track behaviour is the de-facto contract.
So: keep whole-track delivery, make it cheap. SubtitleCache already had
the right shape for PGS; text was excluded only by the assumption that
"VTT is already windowed and fast", which the inert -t makes false.
Windowing a cached 83KB VTT costs 52ms versus 16s against the original
27GB remux.
Routing on AllowWindow would have poisoned the cache: it is set only in
the PGS branch, so it is always false for text, while streamExtractArgs
applies -ss to any non-ASS/non-PGS source regardless. A seeked subrip
request would take the full-track path, emit seek->EOF, exit cleanly and
publish that partial as canonical -- and every later viewer from 0 would
lose all cues before it (118 of 143 production requests carry a non-zero
seek). Canonicality is now derived from the effective argv instead:
streamExtractPlanFor is the single source of truth for both the argv and
the partial() predicate, so only a seek=0/duration=0 extract can fill.
- key: adds a schema version and the resolved output profile (an ASS
source is reachable as both .ass and .vtt, so format must be keyed)
- removeStaleSiblings now groups by profile, so committing .vtt no longer
deletes a valid .ass sibling; cleanup and eviction no longer hardcode
.sup, so text entries are reclaimed and counted
- InputIsExtractedTrack carries the cached input's format
- text hits use a plain copy with no-store, matching cold-path HTTP
semantics; ServeContent stays on the PGS path only, since Media3 uses
range-capable data sources and responses must not vary with cache warmth
- renames SUP-specific identifiers now that the cache carries text
The inert -t is deliberately retained and its comment corrected in place:
removing it or moving it after -i would bound the output and break the
native clients.
Latency-only: for every (source codec, requested format, seek) the bytes
a client receives are unchanged.
Rationale, measurements, four superseded revisions and the dead ends are
recorded in docs/superpowers/plans/2026-07-17-subtitle-extract-cache.md.
AI-use disclosure: investigated and implemented with AI assistance
(Claude Code + Codex gpt-5.6-sol); Codex's review caught the cache-
poisoning bug above.
📝 WalkthroughWalkthroughEmbedded subtitle extraction now uses shared output planning and a format-aware cache. SUP, ASS, and WebVTT artifacts use profile-specific keys, content types, canonical full-track caching, and generalized serving behavior. Stream and proxy handlers use the unified ChangesSubtitle extraction cache
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamHandler
participant SubtitleCache
participant ffmpeg
Client->>StreamHandler: Request embedded subtitle
StreamHandler->>SubtitleCache: ServeExtract
SubtitleCache->>SubtitleCache: Lookup output profile
SubtitleCache->>ffmpeg: Extract missing artifact
ffmpeg-->>SubtitleCache: Return subtitle artifact
SubtitleCache-->>StreamHandler: Serve cached or extracted output
StreamHandler-->>Client: Return subtitle response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-17-subtitle-extract-cache.md`:
- Around line 51-54: Update the documentation around streamExtractArgs to use
repository-relative references for files such as subtitle_stream.go, and remove
the host/container absolute path to ffmpeg. Describe the measured binary as the
container-installed production ffmpeg without including a local filesystem path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3deb63f9-908a-400a-aa8b-09a9326767eb
📒 Files selected for processing (7)
docs/superpowers/plans/2026-07-17-subtitle-extract-cache.mdinternal/api/handlers/stream.gointernal/playback/subtitle_cache.gointernal/playback/subtitle_cache_test.gointernal/playback/subtitle_stream.gointernal/playback/subtitle_stream_test.gointernal/proxy/server.go
Comment on lines
+51
to
+54
| `streamExtractArgs` (`subtitle_stream.go:161-168`) passes `-t` as an **input** | ||
| option, reasoning it "caps how much of the file we read". Measured against the | ||
| production binary (`ffmpeg 7.1.4-Jellyfin` at `/usr/lib/jellyfin-ffmpeg/ffmpeg` | ||
| inside the `silo` container — not on `$PATH`; there is no host ffmpeg): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use repository-relative references.
Replace basename-only file references with repository-relative paths. Remove /usr/lib/jellyfin-ffmpeg/ffmpeg and describe the container-installed binary without a local absolute path.
Proposed correction
-`streamExtractArgs` (`subtitle_stream.go:161-168`) passes `-t` as an **input**
+`streamExtractArgs` (`internal/playback/subtitle_stream.go`) passes `-t` as an **input**
...
-production binary (`ffmpeg 7.1.4-Jellyfin` at `/usr/lib/jellyfin-ffmpeg/ffmpeg`
-inside the `silo` container — not on `$PATH`; there is no host ffmpeg):
+production binary (`ffmpeg 7.1.4-Jellyfin` in the `silo` container's Jellyfin
+installation — not on `$PATH`; there is no host ffmpeg):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `streamExtractArgs` (`subtitle_stream.go:161-168`) passes `-t` as an **input** | |
| option, reasoning it "caps how much of the file we read". Measured against the | |
| production binary (`ffmpeg 7.1.4-Jellyfin` at `/usr/lib/jellyfin-ffmpeg/ffmpeg` | |
| inside the `silo` container — not on `$PATH`; there is no host ffmpeg): | |
| `streamExtractArgs` (`internal/playback/subtitle_stream.go`) passes `-t` as an **input** | |
| option, reasoning it "caps how much of the file we read". Measured against the | |
| production binary (`ffmpeg 7.1.4-Jellyfin` in the `silo` container's Jellyfin | |
| installation — not on `$PATH`; there is no host ffmpeg): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-17-subtitle-extract-cache.md` around lines 51
- 54, Update the documentation around streamExtractArgs to use
repository-relative references for files such as subtitle_stream.go, and remove
the host/container absolute path to ffmpeg. Describe the measured binary as the
container-installed production ffmpeg without including a local filesystem path.
Source: Coding guidelines
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.
Closes #423. Part of #296.
Problem
Turning on a subtitle mid-film could hang for two minutes, and the server never noticed.
On the web player, picking an embedded subtitle track sometimes did nothing for a very long
time before cues appeared — or before subtitles simply never showed up at all. Over a 41.6
hour window the subtitle endpoint measured p95 120,021 ms and max 121,470 ms, with 45
of 220 fetches taking over ten seconds.
The 120-second number is not a coincidence: it is the server's own hard write deadline. The
response was being cut off mid-flight. Because the server had already told the client
"200 OK" before the slow part began, every one of those cut-off responses was recorded in
the logs as a success. Operators looking at error rates saw nothing wrong, while viewers
were watching a film with no subtitles.
The underlying cost is that a subtitle track is not stored in one piece. Its text is
scattered through the whole file, interleaved between video and audio. Pulling out a few
kilobytes of dialogue means reading across that entire stretch of a multi-gigabyte file on
network storage.
The safety limit that was supposed to bound this does nothing. The server passes a
600-second window to ffmpeg, and has since the endpoint was written. It is silently ignored
for these extracts: every request reads from its start point all the way to the end of the
film, no matter what the window says. So the cost of a request tracks how much film is left,
and nobody had noticed because the code reads as though it were bounded.
Solution
perf(playback): cache embedded text-subtitle extracts
Measured against the production binary,
-ss 4000 -t 30and-ss 4000 -t 300producebyte-identical output to passing no
-tat all (last cue 02:10:04, end of film).-tispassed as an input option and ffmpeg discards it for subtitle extraction; only
-sssurvives. Cost is therefore
(duration - seek) × bitrate.The window cannot simply be repaired. silo-apple and silo-android both fetch a track
once and depend on receiving the whole thing. Actually bounding the output would silently
kill subtitles roughly ten minutes into every film on both platforms. The accidental
whole-track behaviour is the de-facto client contract, so this keeps whole-track delivery
and makes it cheap instead.
SubtitleCachealready had the right shape from the PGS work; text was excluded only by theassumption that "VTT is already windowed and fast", which the inert
-tmakes false.Windowing a cached 83 KB VTT costs 52 ms against 16 s for the same window over the original
27 GB remux.
Canonicality is derived from the effective ffmpeg argv, not from a flag.
streamExtractPlanFor(internal/playback/subtitle_stream.go) is the single source oftruth for both the argv and the
partial()predicate, so only aseek=0/duration=0extract can fill the cache. Routing on
AllowWindowinstead would have poisoned it:AllowWindowis set only in the PGS branch, so it is always false for text, whilestreamExtractArgsapplies-ssto any non-ASS/non-PGS source regardless. A seeked subriprequest would have taken the full-track path, emitted seek→EOF, exited cleanly, and
published that partial as canonical — and every later viewer starting from 0 would lose all
cues before that point. 118 of 143 production requests carry a non-zero seek.
Supporting changes in
internal/playback/subtitle_cache.go:reachable as both
.assand.vtt, so format has to be part of the key.removeStaleSiblingsgroups by profile, so committing a.vttno longer deletes a valid.asssibling. Cleanup and eviction no longer hardcode.sup, so text entries areactually reclaimed and counted against the budget.
InputIsExtractedTrackcarries the cached input's format.no-store, matching cold-path HTTP semantics.http.ServeContentstays on the PGS path only: Media3 uses range-capable data sources,and responses must not vary with cache warmth.
The inert
-tis deliberately retained and its comment corrected in place(
internal/playback/subtitle_stream.go:152) — removing it, or moving it after-i, wouldbound the output and break the native clients.
This is latency-only: for every (source codec, requested format, seek) combination, the
bytes a client receives are unchanged.
Risk / follow-ups
silo-apple and silo-android; the comment now says so, but it is still a trap.
defaultSubtitleCacheMaxBytes. Text entries are small (tens of KB) so they will notmeaningfully displace bitmap entries, but exposing this as a config knob following the
download.artifact_max_bytespattern is still outstanding.extract rather than waiting. That is no worse than current behaviour and avoids making one
viewer's first-byte latency depend on another client's connection, but it does mean a
cold-start burst can duplicate work.
latency that was triggering it. Logging truncated bodies distinctly is separate work.
other half of Subtitle enable mid-playback is slow: on-demand ffmpeg extraction with no caching; compat endpoint buffers whole file #296.
Verification
go build ./...— clean.go vet ./internal/playback/... ./internal/api/handlers/... ./internal/proxy/...— clean.go test ./internal/playback/...— pass.subtitle_cache_test.gogains coverage forprofile-keyed entries, sibling retention across formats, partial requests never filling,
and text eviction accounting.
go test ./internal/api/handlers/... ./internal/proxy/...— pass.golangci-lint run --new-from-merge-base=origin/mainover the touched packages — 0 issues.gofmt -l internal/— clean.make verify-local-paths— clean.-tvariantsbyte-identical, documented in the plan doc.
origin/main(8bde6f1); the three touched Go files have had no commits onmainsince 2026-07-15, so the rebase carried no conflicts.Rationale, measurements, four superseded revisions and the dead ends are recorded in
docs/superpowers/plans/2026-07-17-subtitle-extract-cache.md.AI-use disclosure
Investigated and implemented with AI assistance (Claude Code + Codex gpt-5.6-sol). Codex's
review caught the
AllowWindowcache-poisoning bug described above. All measurements weretaken against the real production binary and production request logs.
Summary by CodeRabbit
New Features
Bug Fixes