diff --git a/internal/design/spf/features/engine-adapter-integration.md b/internal/design/spf/features/engine-adapter-integration.md index 1875807a..6f19611a 100644 --- a/internal/design/spf/features/engine-adapter-integration.md +++ b/internal/design/spf/features/engine-adapter-integration.md @@ -35,7 +35,7 @@ the mixin. | Writable signal refs via `onSignalsReady` | `shareSignals` captures `Signal` / `ReadonlySignal` refs into a consumer-supplied callback at setup time. Generic over composition shape (`makeShareSignals()`) | Per-slot read/write intent is expressed at the use site (callers type captured refs as `Signal` or `ReadonlySignal`). Composed last in the engine so initial state writes are visible to the consumer | | Mixin adapter pattern | `SimpleHlsMediaMixin` is the canonical consumer: function-of-base-class structure (mix into any base), captures refs once in `onSignalsReady`, exposes a WHATWG HTMLMediaElement-shaped API mapping each setter/method to engine writes | Downstream use: `class SimpleHlsMedia extends SimpleHlsMediaMixin(HTMLVideoElementHost) {}` in `packages/core/src/dom/media/simple-hls/` | | Media element binding | `attach(el)` writes `context.mediaElement`; `detach()` clears it. **Engine persists across attach/detach cycles** — only `src` reassignment or explicit `destroy()` tears it down | Re-attach to a different element is supported. The engine is the durable state holder; `mediaElement` is a context slot | -| Source assignment via destroy + recreate | Adapter's `set src` destroys the current engine and creates a fresh one, re-applies any explicit preload, re-attaches `mediaElement` to the new engine, and writes the new `{ url }` | Bypasses the in-place source-replacement path. Rationale not documented in code — see Open questions and [source-replacement.md](./source-replacement.md) | +| Source assignment via in-place recycling | Adapter's `set src` overwrites `state.presentation` on its single recycled engine (`{ url }`, or `undefined` for empty src). Media element + engine-wide preload persist; no engine recreation, no signal re-capture | Drives the engine's in-place source-replacement cascade — see [source-replacement.md](./source-replacement.md). (The adapter previously destroyed + recreated the engine per assignment.) | | Preload reflection | `set preload(value)` writes W3C values to `state.preload`; clearing (`preload = ''`) doesn't patch the current engine but is re-applied on the next src change. Pre-attach src + preload combinations are supported | Extended preload values flow through state but don't reach the DOM (per [`preload-modes`](./preload-modes.md)'s sticky-extended-values semantics) | | Programmatic `play()` with retry | `play()` writes `state.loadActivated = true` (co-writer with `trackLoadTriggers`'s DOM listener path) before invoking native play. **Defensive retry:** if native play rejects with "no supported sources" while src is pending, wait for `loadstart` (MSE attaches blob URL) and retry once | The retry handles MSE pipeline timing — adapter doesn't know exactly when MSE setup attaches the blob URL. Listener canceled on src change | @@ -101,7 +101,7 @@ return createComposition( | `attach(el)` | `context.mediaElement.set(el)` | | `detach()` | `context.mediaElement.set(undefined)` | | `destroy()` | `engine.destroy()` | -| `set src(value)` | `engine.destroy()` → new engine → `state.presentation.set({ url: value })` | +| `set src(value)` | `state.presentation.set({ url: value })` on the recycled engine (`undefined` for empty src) | | `set preload(value)` | `state.preload.set(value)` (W3C values only; pre-empties stay engine-local) | | `play()` | `state.loadActivated.set(true)` → native `play()` with `loadstart` retry on "no supported sources" | @@ -156,15 +156,15 @@ each `set src`). ## Open questions -- **Destroy-recreate vs in-place source replacement.** The canonical - adapter destroys + recreates the engine on every `src` change, even - though the engine's behaviors support in-place - `state.presentation` overwrite (validated by - [`source-replacement`](./source-replacement.md)'s test). The - rationale isn't documented in the code or commit history. Possible - motivations: stricter isolation between sources; simpler reasoning - per-engine; guarding against latent cleanup-cascade bugs. Worth - resolving when the cost of either choice surfaces. +- **Destroy-recreate vs in-place source replacement.** Resolved: the + canonical adapter now recycles a single engine and overwrites + `state.presentation` in place on every `src` change, driving the same + cascade validated by + [`source-replacement`](./source-replacement.md)'s test. This unifies + per-source teardown on one path and lets adapter-side projections + wire once at construction rather than re-wiring on every src change. + It also makes source-change behavior stable enough to build the + media-tracks mixin integration on top of. - **Callback timing semantics.** `shareSignals`'s JSDoc explicitly notes the callback fires while other behaviors are still in setup; reads inside the callback may yield only initial-seed values. The diff --git a/internal/design/spf/features/source-replacement.md b/internal/design/spf/features/source-replacement.md index d1940e06..cd8cc75c 100644 --- a/internal/design/spf/features/source-replacement.md +++ b/internal/design/spf/features/source-replacement.md @@ -15,12 +15,6 @@ load-bearing — every behavior that gates on `isResolvedPresentation` must honor the state-exit cleanup contract, or in-place replacement breaks silently. -The canonical adapter (`SimpleHlsMediaMixin.src`) takes a different -path: destroy the engine and create a fresh one on every assignment. -Both paths work; the in-place path is the load-bearing one for -*engine-internal* reasoning, since the adapter's destroy path bypasses -the cascade entirely. - This doc captures the **capability surface**, the **cleanup contract** new behaviors must honor, and the verification that pins the in-place path against regression. @@ -49,7 +43,7 @@ distinct engine behavior observable from outside. | Initial source load | First source on a fresh engine: external write of `state.presentation = { url }` triggers resolve + full pipeline setup | The unresolved → resolved transition that bootstraps everything | | In-place source replacement | Overwrite `state.presentation` with a new `{ url }` while a previous source is resolved / playing. `resolvePresentation` routes back through `'resolving'`; downstream behaviors tear down via reactor state-exit; new source resolves and plays — *same engine instance* | Validated end-to-end. MediaSource + buffer actors are fresh instances; in-flight fetches aborted via state-bound `AbortController`s | | Source unset | Set `state.presentation` to `undefined`. All presentation-gated behaviors transition to `'preconditions-unmet'` and tear down. Engine is fresh-but-attached, ready for the next source | The "no source" steady state; reachable from any resolved state | -| Destroy + recreate (canonical adapter path) | `SimpleHlsMediaMixin.src` destroys the current engine and creates a fresh one on every assignment. Re-attaches the media element to the new engine | Canonical *consumer-side* mechanism. Bypasses the in-place cascade entirely. Tested via `adapter.test.ts:115–150` | +| Adapter-driven in-place replacement (canonical consumer path) | `SimpleHlsMediaMixin.src` overwrites `state.presentation` on its recycled engine (empty `src` → `undefined`, unsetting the source). Media element + engine-wide preload persist across the change | Canonical *consumer-side* mechanism. Rides the same in-place cascade as engine-internal replacement — no engine recreation. Tested via the recycling assertions in `adapter.test.ts` | | Per-source-identity slot lifecycle | `loadActivated` resets to `false` when source identity changes (URL or `mediaElement`); selected*TrackIds clear naturally on un-resolve (their pickers re-run against the new presentation); **`bandwidthState` is intentionally preserved** across source resets — sampling accumulates via the once-per-behavior `createTrackedFetch` | ABR resume: bandwidth estimate carries over so the first segment of a new source picks an appropriate quality based on observed throughput | ## What's not implemented @@ -153,17 +147,17 @@ semantics — every replaced source runs through the same parser. captured identities differ from the new ones (proving teardown cascade ran) - `packages/spf/src/playback/engines/hls/tests/adapter.test.ts` → - `"creates a new engine when src is set"` / - `"destroys the old engine when src changes"` / - `"re-attaches the media element to the new engine when src changes"` - — validates the canonical adapter destroy-recreate path + `"reuses the same engine instance when src changes"` / + `"does not destroy the engine when src changes"` / + `"keeps the attached media element across src changes"` + — validates the canonical adapter's in-place recycling path - `packages/spf/src/playback/behaviors/dom/tests/track-load-triggers.test.ts` — `loadActivated` per-source-identity reset coverage - **Sandbox:** - `apps/sandbox/src/spf-segment-loading/` — exercises initial source load + manual rendition switching (in-track, not source change) - `apps/sandbox/src/simple-hls-html/` / `simple-hls-react/` — adapter - integration; src reassignment hits the destroy-recreate path + integration; src reassignment recycles the engine via the in-place path ## Open questions @@ -172,12 +166,12 @@ semantics — every replaced source runs through the same parser. management)` for a state-error slot. The shape of this slot — single error vs per-source — affects how consumers respond to "source failed to load." -- **Adapter rationale.** The canonical adapter destroys + recreates the - engine on every src change instead of using in-place replacement. The - reasoning (stricter isolation? simpler reasoning? guarding against - cleanup-cascade bugs?) isn't documented in the code or commit history. - If the in-place path is the engine's load-bearing capability for - internal reasoning, why doesn't the adapter use it? +- **Adapter rationale.** Resolved: the canonical adapter + now recycles a single engine and drives source changes through in-place + `state.presentation` replacement — the same load-bearing cascade the + engine uses internally. Recycling was adopted so per-source teardown + routes through one path, and so adapter-side projections wire once at + construction instead of re-wiring on every src change. - **Per-source `bandwidthState` reset opt-in.** Preserving across sources is the right default for ABR resume, but a test / fresh- session escape hatch may earn its place when consumers start needing @@ -200,10 +194,9 @@ semantics — every replaced source runs through the same parser. re-bootstrapping from `initialBandwidth`. - **subtitles** — text-track actors and selection clear on source un-resolve via the cleanup cascade. -- **engine-adapter-integration** *(not yet documented, candidate)* — - `SimpleHlsMediaMixin`'s destroy-recreate path lives here. The - adapter's choice to bypass in-place replacement is the - feature-design decision to capture in that doc. +- **engine-adapter-integration** — `SimpleHlsMediaMixin`'s source- + assignment path lives here. The adapter recycles a single engine and + drives source changes through this feature's in-place cascade. ## See also diff --git a/packages/spf/src/playback/engines/hls/adapter.ts b/packages/spf/src/playback/engines/hls/adapter.ts index 49e5214f..0143efbe 100644 --- a/packages/spf/src/playback/engines/hls/adapter.ts +++ b/packages/spf/src/playback/engines/hls/adapter.ts @@ -32,10 +32,8 @@ export interface SimpleHlsMediaAPI extends SimpleHlsMediaProps { * Implements the src/play() contract per the WHATWG HTML spec so that SPF can * be used anywhere a media element API is expected. * - * A new engine is created on every src assignment — this fully tears down all - * state, SourceBuffers, and in-flight requests from the previous source before - * the next one begins. The media element reference is preserved across src - * changes and re-applied to the new engine automatically. + * A single engine instance is created at construction and recycled across src + * changes. * * @example * class SimpleHlsMedia extends SimpleHlsMediaMixin(HTMLVideoElementHost) {} @@ -46,7 +44,7 @@ export interface SimpleHlsMediaAPI extends SimpleHlsMediaProps { */ export function SimpleHlsMediaMixin>(BaseClass: Base) { class SimpleHlsMediaImpl extends BaseClass { - #engine: Composition; + readonly #engine: Composition; #config: SimpleHlsEngineConfig; #signals!: SimpleHlsEngineSignals; #preload: '' | 'none' | 'metadata' | 'auto' = simpleHlsMediaDefaultProps.preload; @@ -106,15 +104,19 @@ export function SimpleHlsMediaMixin>(BaseClass: Ba if (value) { this.#signals.state.preload.set(value); } - // value = '' clears #preload (so the next engine recreation won't re-apply - // an explicit value) but does not patch current state — the existing preload - // stays in effect until the next src change creates a fresh engine. + // value = '' resets the IDL mirror (so `get preload` reflects '') but does + // not patch state — the engine keeps its current preload until an explicit + // W3C value replaces it. } // ------------------------------------------------------------------------- // src — synchronous IDL attribute (WHATWG §4.8.11.2) - // Each assignment destroys the current engine and starts a fresh one, exactly - // as the browser's load algorithm resets all media element state on src change. + // Each assignment overwrites the engine's presentation state in place. The + // resolver FSM routes back through teardown → rebuild on the same engine, + // mirroring how the browser's load algorithm resets media state on src change + // — without recreating the engine or re-capturing its signals. Setting an + // empty src un-resolves the presentation, tearing the current source down to + // the engine's fresh-but-attached "no source" state. // ------------------------------------------------------------------------- get src(): string { @@ -122,27 +124,8 @@ export function SimpleHlsMediaMixin>(BaseClass: Ba } set src(value: string) { - const prevMediaElement = this.#signals.context.mediaElement.get(); - this.#cancelPendingPlay(); - this.#engine.destroy(); - this.#engine = this.#createEngine(); - - // Apply explicit preload before setting context so it's already in - // state.preload when syncPreload's read effect runs on the attach — - // the read effect only overwrites when the element's `preload` is a - // W3C value (which a freshly-created