fix(playback): restore V3 subtitle timing and stability - #598
Conversation
📝 WalkthroughWalkthroughPlayback V3 now resolves remux seeks to preceding keyframes before transport selection. Local, remote, audiobook, video, and subtitle playback use ChangesPlayback timeline and subtitle handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlaybackHandler
participant FFprobe
participant Transport
Client->>PlaybackHandler: request playback at target position
PlaybackHandler->>FFprobe: resolve preceding keyframe
FFprobe-->>PlaybackHandler: return seek anchor and segment
PlaybackHandler->>Transport: start with prepared timeline metadata
Transport-->>Client: return playback plan
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Quick104
changed the title
![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 `@web/src/pages/audiobooks/player/useAudiobookPlayback.ts`:
- Line 329: Update useAudiobookPlayback and the audiobook time-conversion
helpers to rename streamOriginSecondsRef to timelineOffsetSecondsRef, preserve
finite signed timeline_offset_seconds values through audiobookAbsoluteTime and
absoluteBufferedRanges, and clamp only the resulting absolute position and
buffered range boundaries to zero. Add a regression test covering a negative
timeline offset.
🪄 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: 63df1b97-dcb8-41a5-ad5b-d594d858dab0
📒 Files selected for processing (11)
docs/architecture/playback-protocol-v3.mdinternal/api/handlers/playback.gointernal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_test.goweb/src/pages/audiobooks/player/useAudiobookPlayback.test.tsweb/src/pages/audiobooks/player/useAudiobookPlayback.tsweb/src/player/client-context-v3.test.tsweb/src/player/client-context-v3.tsweb/src/player/components/VideoPlayer.test.tsxweb/src/player/components/VideoPlayer.tsxweb/src/player/utils/mediaTimeline.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55b17470e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 271ee33022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9486f1ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Problem
Part of #135
Follow-up to #567
The neutral V3 migration stopped calling the existing copy-seek anchor resolver. FFmpeg copy remuxes begin on the preceding keyframe, but V3 advertised the requested seek as byte-zero. The web player also used
stream_origin_secondsfor source/player conversion instead of the contract'stimeline_offset_seconds.On the isolated real-media reproduction, a request for 1086.2 s emitted its first copied video packet from 1085.501 s. The old V3 response advertised both origin and player start as 1086.2/0, producing a measurable 699 ms subtitle error.
Follow-up runtime testing exposed the reported flashing. The merged web capability incorrectly advertised
embedded_text: false, even though Silo exposes embedded text tracks to the web player through session-scoped VTT/ASS sidecars that it already renders. Selecting an embedded SRT therefore forced an unnecessary burn-in video transcode. Shared dev could not initialize its QSV device, so that replan was refused; rollback then cleared the automatic-selection guard and immediately selected the same rejected track again. One session issued 483 replans in 155 seconds, repeatedly toggling subtitle state and flashing the cues. Any refused automatically selected subtitle replan could trigger the feedback loop.Approach
V:0stream that FFmpeg maps for video remuxes.timeline_offset_secondsfor source/player conversions.Impact
Resumed and reanchored copy remuxes now keep video, text/ASS subtitles, progress, and seeks on the same source clock. Embedded text subtitles remain client-rendered and no longer depend on a video encoder. A failed subtitle selection cannot cause a replan storm or flashing cues. Bitmap subtitles still correctly require burn-in, and direct/encoded route timeline shapes are unchanged.
The shared-dev QSV startup failure remains an operational issue for routes that genuinely require video encoding; this change removes that dependency from embedded text playback rather than masking encoder failures.
Validation
server_remux_progressivetranscode_start_failedafter QSV/VAAPI initialization failedinternal/api/handlersandinternal/playbackpackage tests: passed.make test-web: passed 273 web test files / 1,876 tests.pnpm run build: passed.make test: passed all Go packages and 273 web test files / 1,873 tests.go test ./internal/api/handlers ./internal/playback -count=1: passed.go vet ./...: passed.golangci-lint v2.12.2: 0 issues.pnpm run lint: 0 errors (151 existing warnings).pnpm run format:check: passed.make verify-local-pathsandgit diff --check: passed.Coverage gap
The collaborative browser reached the shared-dev sign-in screen but had no authenticated session, so the post-fix playback path was not visually replayed there and this branch was not deployed to the shared environment. The timing path was exercised through an authenticated isolated sandbox against real scanned media; the flashing cause was independently confirmed from shared-dev session, route-event, replan, and FFmpeg evidence; and both regressions are covered by focused tests plus the full web suite.
AI Disclosure
Checklist
Summary by CodeRabbit