feat(spf): text tracks switching#1687
Open
cjpillsbury wants to merge 10 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cjpillsbury
force-pushed
the
feat/spf-capability-probing
branch
2 times, most recently
from
09b3404 to
b2f53d8
Compare
cjpillsbury
force-pushed
the
feat/spf-text-tracks-switching
branch
from
052dee0 to
b3605de
Compare
cjpillsbury
force-pushed
the
feat/spf-capability-probing
branch
from
b2f53d8 to
6134d8e
Compare
cjpillsbury
force-pushed
the
feat/spf-text-tracks-switching
branch
from
b3605de to
7e53ffd
Compare
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…selection The final pick was hardcoded to the rule-chain head, which assumes a track is always selected. Add an optional resolveSelection(candidates, deps) config hook (defaulting to the chain head) so a variant whose selection is legitimately optional — text: opt-in captions, explicit off — can resolve to undefined and clear the slot. Video and audio don't supply it and are unchanged. Also lands a working-reference plan for the text-track-switching refactor under .claude/plans/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…selection Migrate text selection onto the track-switching rule chain so it gains the failed-CDN constraint and active-CDN scope it previously lacked. Unlike video and audio, text selection is optional (captions are opt-in and off-able), so the variant runs [excludeFailedCdns] + [preferActiveCdn] and supplies a text-specific terminal (pickResolvedTextTrack) via the resolveSelection seam: it resolves the standing userTextTrackSelection intent (a language partial, 'off', or undefined=auto) against the constrained, CDN-scoped candidates and may yield no selection. Extract pickTextTrackFromTracks from pickTextTrack so the auto-default policy (preferred language -> DEFAULT+AUTOSELECT -> none, forced excluded) is shared between the presentation-shaped picker and the candidate-list terminal. Additive only: switchTextTrack is not yet wired into an engine — selectTextTrack still owns text selection until the engine rewire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…the resolved id syncTextTracks no longer writes selectedTextTrackId. Its change-bridge now writes userTextTrackSelection — a language-based partial (sticky across sources) or 'off' — which switchTextTrack resolves into selectedTextTrackId. The behavior reads the resolved id only to mirror DOM modes one-way and as the echo-guard reference, so DOM action and the resolver no longer contend for one slot. The echo guard (showingId === selectedTextTrackId) now also absorbs resolver-driven mode corrections: a correction that disables the user's pick (e.g. its CDN failed) is recognized as an echo and not written back as a spurious 'off'. Not yet wired into an engine — selectTextTrack still owns text selection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tTrack Compose switchTextTrack in place of selectTextTrack so text selection runs the track-switching chain (failed-CDN constraint + active-CDN scope) and resolves the standing userTextTrackSelection intent. Expose userTextTrackSelection via shareSignals as the programmatic + DOM write path; selectedTextTrackId is now a single-writer resolved output. Remove the now-dead selectTextTrack behavior and its SelectTextTrackConfig (pickTextTrack primitive stays). Update the sandbox to drive text selection through userTextTrackSelection instead of writing the resolved id directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xtTrack model Reflect the shipped refactor: selectedTextTrackId is a single-writer output of switchTextTrack; userTextTrackSelection is the sticky intent input (DOM + consumer); selection runs the track-switching chain (failed-CDN constraint + active-CDN scope). Rewrite the bidirectional-sync section as a one-way mode mirror + DOM→intent bridge with the echo guard, and correct the enableDefaultTrack default (false, not true). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… demo A Subtitles/Captions picker (Off + one button per language, with an auto/pinned/off status row and reset) wired to userTextTrackSelection, so text selection is driven through the intent path like the audio picker. Replaces the stage-d auto-select-first-text hack, so the engine's real opt-in default policy runs on load. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Setting the `default` attribute on an SPF-owned <track> made the browser auto-activate that slot on insertion, firing a `change` that syncTextTracks recorded as user intent — auto-enabling captions for a DEFAULT=YES rendition past SPF's opt-in policy (enableDefaultTrack defaults to false). SPF owns selection via switchTextTrack + selectedTextTrackId, so the slots carry no selection hint; DEFAULT=YES is honored (or not) by the engine's default policy, not the browser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ack model Update the design/package docs that still named the removed selectTextTrack behavior or the old multi-writer selectedTextTrackId pattern: architecture, capability-probing (text has no capability constraint), audio-playback, source-replacement, background-looping-video, audio-only-mode-override, multi-language-audio, clusters, conventions/behaviors, and the hls-engine walkthrough. The multi-writer convention + cluster note are reframed around the intent→resolved resolution (route differing inputs to a userTextTrackSelection intent slot; switchTextTrack is the single writer of selectedTextTrackId). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ult attribute Completes a8de404: the full-engine "creates track elements for all text tracks" test still asserted the DEFAULT=YES rendition's <track>.default === true. Assert false — the attribute is intentionally not propagated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Off + language buttons now set mediaElement.textTracks[id].mode (what a captions button / browser UI touches) instead of writing userTextTrackSelection directly, so a click exercises the real syncTextTracks DOM→intent bridge (change event → language partial / 'off' → switchTextTrack → resolved id). "Reset to auto" stays a direct userTextTrackSelection=undefined write — it has no native-mode analog (it means "forget my preference"). Smoke-tested via Playwright against a multi-language source: native selection bridges to intent, Off bridges to 'off' (echo guard holds), reset returns to the opt-in default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cjpillsbury
force-pushed
the
feat/spf-text-tracks-switching
branch
from
7e53ffd to
6b3d26b
Compare
Contributor
📦 Bundle Size Report🎨 @videojs/html — no changesPresets (7)
Media (9)
Players (5)
Skins (30)
UI Components (37)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (8)
Skins (27)
UI Components (31)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (11)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6b3d26b. Configure here.
| if (showingId === state.selectedTextTrackId.get()) return; | ||
| // Genuine user action → write intent (resolved into selectedTextTrackId | ||
| // by switchTextTrack), not the resolved id. | ||
| state.userTextTrackSelection.set(deriveTextTrackIntent(showingId, modelTextTracks)); |
There was a problem hiding this comment.
Stale model tracks in bridge
Low Severity
The DOM change handler captures modelTextTracks once on sync-active entry and passes that snapshot into deriveTextTrackIntent on every user event. If the resolved presentation’s text renditions change while the reactor stays in sync-active, intent can be derived from outdated track metadata even though switchTextTrack reads the live presentation.
Reviewed by Cursor Bugbot for commit 6b3d26b. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Likely fine, but will confirm edge case concerns/considerations.
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
2 participants
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.
TL;DR
Migrates text-track (caption/subtitle) selection onto the track-switching rule chain introduced in #1658, so captions gain the same failed-CDN constraint (drop tracks on a CDN in failover cooldown) and active-CDN scope that video/audio already run. Because captions are opt-in and off-able, it adds an optional-selection seam (a variant may resolve to no pick). The text slot is split into a sticky
userTextTrackSelectionintent input (DOM + programmatic) and a single-writerselectedTextTrackIdresolved output, retiring the oldselectTextTrackbehavior. Internal SPF only (alpha).For reviewers — how to read this PR
Bucket 1 — Runtime (focus here)
Targeted careful read:
track-switching.ts(+204) — theresolveSelectionseam (optional terminal; defaults to the rule-chain head, so video/audio are unchanged);switchTextTrackruns[excludeFailedCdns, preferActiveCdn]thenpickResolvedTextTrack, resolvinguserTextTrackSelection(language partial /'off'/undefined→auto) against the constrained candidates and may select nothing. Verify the optional-none path stays distinct from the empty-candidate clear path added for capability-probing (no-pick-by-intent vs. constraints-emptied)sync-text-tracks.ts(+69) — DOM→intent bridge now writesuserTextTrackSelection, not the resolved id (single-writer invariant:switchTextTrackis the sole writer ofselectedTextTrackId); the echo guard (showingId === selectedTextTrackId) must also absorb resolver-driven mode corrections (e.g. a pick disabled because its CDN failed) without writing back a spurious'off'engine.ts(+31) —switchTextTrackcomposed in place ofselectTextTrack;userTextTrackSelectionexposed viashareSignalsas the programmatic + DOM write pathmedia/primitives/select-tracks.ts(Δ) —pickTextTrackFromTracksextracted frompickTextTrack; shared auto-default policy (preferred language →DEFAULT+AUTOSELECT→ none; forced excluded) across the presentation-picker and the candidate-list terminalSkim structure only:
behaviors/select-tracks.ts(selectTextTrack+SelectTextTrackConfigremoved,pickTextTrackstays — no dangling refs),engine-audio-only.ts/text-track-slots.ts/all.ts(wiring + exports),engine.test.ts(<track>.defaultassertion flipped tofalse)Skim file: Tests —
track-switching.test.ts(+278),sync-text-tracks.test.ts(+65),select-tracks.test.ts(Δ) — assertion shape across intent-resolution, optional-none, and echo-guard mode correctionsSandbox (runtime-adjacent), skim file:
spf-segment-loading/main.ts(+124) — subtitle picker driven via nativetextTracks[id].modeso a click exercises the real DOM→intent bridge, not a direct slot writeBucket 2 — Design docs (skim)
Skim file:
text-track-architecture.md(+72) — the intent-input + single-writer-output model; one-way mode mirror + DOM→intent bridge + echo guard;enableDefaultTrackdefault corrected tofalseSkim or skip:
subtitles.md(+28) and the stale-selectTextTrack-reference sweep across the registry (architecture,conventions/behaviors,capability-probing— text has no capability constraint —,clusters,multi-language-audio,source-replacement,audio-playback,audio-only-mode-override,background-looping-video,hls-engine)Bucket 3 — Working-reference plan, no runtime (skip)
Skim or skip:
.claude/plans/spf-text-track-switching-refactor.mdSmoke test
Sandbox:
/spf-segment-loading/?src=https://stream.mux.com/s41JYeqIpBMBzE4OzxDyGR2yrp2hD1CQ6gJN9SlVGDQ.m3u8?redundant_streams=truepnpm dev:sandbox→ paste the path into the running Vite server) or via the PR's deploy preview. Press Play (the harness video ispreload="none").redundant_streams=truelists each rendition on multiple CDNs, so text selection exercises the active-CDN scope it now runs through.DEFAULT+AUTOSELECTrendition), not auto-enabled.What changed — by surface
Text selection onto the track-switching chain.
switchTextTrackreplacesselectTextTrack, running the sameapplyConstraintspre-pass (failed-CDN constraint) + active-CDN scope as video/audio. A new optionalresolveSelection(candidates, deps)hook onsetupTrackSwitchinglets a variant resolve toundefined(the head-of-chain default assumed a track is always picked); text suppliespickResolvedTextTrack, which resolves the standing intent against the constrained candidates.Intent input vs. resolved output. The single
selectedTextTrackIdslot is split:userTextTrackSelection(a language partial,'off', orundefined=auto) is the sticky intent written by the DOM bridge and programmatic callers;selectedTextTrackIdbecomes a single-writer resolved output ofswitchTextTrack. This removes the multi-writer contention where the DOM action and the resolver both wrote one slot.Engine rewire + cleanup.
engine.tscomposesswitchTextTrack, exposesuserTextTrackSelectionviashareSignals, and the deadselectTextTrackbehavior +SelectTextTrackConfigare removed (thepickTextTrackprimitive stays, refactored to share its auto-default policy).DEFAULT=YES<track>fix. SPF no longer sets thedefaultattribute on its<track>elements — that made the browser auto-activate the slot, firing achangethe bridge recorded as user intent and enabling captions past SPF's opt-in policy.Doc cascade.
text-track-architecture.md+subtitles.mdrewritten to theswitchTextTrackmodel; staleselectTextTrack/ multi-writer references swept across the registry.Notable design decisions
userTextTrackSelection), not the resolved id. Alternative considered: keep the bridge writingselectedTextTrackIddirectly (status quo). Rejected because the DOM action and theswitchTextTrackresolver then contend for one slot; routing the DOM through the intent slot makesselectedTextTrackIdsingle-writer and lets the resolver own corrections (e.g. disabling a pick whose CDN failed).resolveSelectionseam, not a separate text path. Alternative considered: special-case text insidesetupTrackSwitchingor give text its own selection behavior. Rejected because a generic optional-terminal hook (defaulting to the chain head) keeps video/audio untouched while unifying text onto the same chain — that's what gives captions the failed-CDN constraint + active-CDN scope they previously lacked.DEFAULT=YESis honored by engine policy, not the browser'sdefaultattribute. Alternative considered: keep propagatingdefaultso the UA hints the slot. Rejected because SPF owns selection (switchTextTrack), and the attribute auto-activated captions past theenableDefaultTrack=falseopt-in policy.Breaking changes
Internal SPF only (alpha; no external consumers). The
selectTextTrackbehavior andSelectTextTrackConfigare removed; text selection now runs the track-switching chain,selectedTextTrackIdis a resolved output (writeuserTextTrackSelectioninstead), and SPF-owned<track>elements no longer carrydefault.Test plan
pnpm -F @videojs/spf test— 1042 passed / 14 skipped), including newswitchTextTrackintent-resolution, optional-none, and echo-guard tests.'off'(echo guard holds); reset → opt-in default.Note
Medium Risk
Refactors core track-selection and DOM caption sync with a breaking alpha API (
selectedTextTrackIdread-only; writeuserTextTrackSelection), but behavior is heavily unit-tested and scoped to SPF internals.Overview
Moves caption/subtitle selection onto the shared track-switching rule chain so text renditions get failed-CDN pruning and active-CDN scoping like video/audio.
selectTextTrackis removed; the HLS engine composesswitchTextTrackinstead and exposes stickyuserTextTrackSelectionviashareSignals.Intent vs resolved id: DOM and programmatic callers write
userTextTrackSelection(language partial,'off', orundefined= auto);switchTextTrackis the sole writer ofselectedTextTrackId.syncTextTracksmirrors the resolved id into nativeTextTrack.modeone way and bridgeschangeevents to intent, with settling-window + echo guard so resolver-driven disables are not recorded as user “off.”Framework seam:
setupTrackSwitchinggains optionalresolveSelection(default chain head); text usespickResolvedTextTrackso selection can legitimately clear. Text skips MSEexcludeUnplayableTracks; user intent is handled in the terminal, notfilterByUserSelection.Opt-in policy fix: SPF-owned
<track>elements no longer set the browserdefaultattribute (avoids auto-enable bypassingenableDefaultTrack). Sandbox adds a subtitles picker driven through native modes to exercise the DOM→intent path.Reviewed by Cursor Bugbot for commit 6b3d26b. Bugbot is set up for automated code reviews on this repo. Configure here.