mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
Follow-up to PR #298 addressing @jrusso1020's review. Each item below maps to a point in his comment. ## Significant ### 1\. Drift threshold 150 ms → 50 ms _mirrorParentMediaTime_ was too loose for lip-synced talking-head content. ITU-R BT.1359 puts A/V perceptibility at ±45 ms; 150 ms sat well inside the "unacceptable" zone. Dropped to 50 ms, extracted as a static constant for clarity. **Verified live on factory-series-c-video (agent-browser):** steady-state offset under parent ownership sampled five times over 400 ms = `[35.7, 33.5, 31.2, 27.2, 36.9]` ms — below the perceptibility floor. Before this PR the same measurement could drift up to 150 ms before correction. ### 2\. Dynamic sub-composition media proxies Under parent ownership, a sub-composition that attaches a new `<audio data-start>` mid-playback was correctly silenced in the iframe (sticky `outputMuted`) but had no parent-frame counterpart to play → silent hole in the audio track. Added a `MutationObserver` on the iframe body watching for `audio[data-start]` / `video[data-start]` additions. New elements are adopted through the same `_adoptIframeMedia` helper the initial scan uses, and if parent ownership is already active the new proxy gets its `currentTime` mirrored and `play()` called immediately (gated on `!this._paused`). Observer disconnects on iframe reload + component disconnect. ### 3\. `bridgeMuted` sticky in `syncRuntimeMedia` The asymmetry James flagged: `outputMuted` was sticky per-tick, `bridgeMuted` was one-shot via `onSetMuted`. A sub-composition activating after a user mute would briefly play at author volume before the next bridge message. `syncRuntimeMedia` now accepts `userMuted` and the per-clip loop uses a single combined `shouldMute` gate. One invariant, two inputs. ### 4\. Reset `_audioOwner` on iframe reload The latch never cleared. On composition switch the player would stay in `parent` ownership against a fresh runtime that hadn't received `set-media-output-muted` and whose autoplay-blocked latch was clean — a brief double-audio window until the next `NotAllowedError` re-promoted (idempotently). `_onIframeLoad` now resets `_audioOwner = "runtime"`, pauses any parent proxies, and disconnects the old MutationObserver before a fresh one attaches to the new document. If the player had been in `parent` ownership, a corresponding `audioownershipchange` event fires with `reason: "iframe-reload"`. ## Worth addressing ### 5\. Promotion → observable event + reason Promotion was invisible. Added `CustomEvent("audioownershipchange", { detail: { owner, reason } })` fired on every owner transition. `reason` is either `"autoplay-blocked"` (promote → parent) or `"iframe-reload"` (reset → runtime). Gives host apps an SLO-ready signal for "% of sessions in parent ownership" without exposing internal state. **Verified live:** dispatching a synthetic `media-autoplay-blocked` in the live studio produced `{ owner: "parent", reason: "autoplay-blocked" }` on the web component exactly once. ### 6\. Parent proxy play() rejection → `playbackerror` event Previously swallowed silently. Now re-emitted as `CustomEvent("playbackerror", { detail: { source: "parent-proxy", error } })` so embedding apps can recover or fall back. ### 7\. Mobile verification on real hardware Tested with a tunnel in a real iOS device. ## Test gaps (from review) - `userMuted` stickiness (mirror of the existing `outputMuted` test). - **OR invariant** between `outputMuted` and `userMuted` — explicit test that setting one false while the other is true keeps `el.muted === true`. - **Contract pin:** `syncRuntimeMedia` fires `onAutoplayBlocked` on **every** rejection (no internal dedupe) — so a future refactor can't quietly move the latch and break the caller's posting logic. - **Caller-side latch pattern:** a 5-rejection simulation with the init.ts-style wrapper posts exactly once. - **`audioownershipchange`** **dispatch** on promotion + once per transition (no duplicate on idempotent re-promote). - **Mid-playback promotion:** `_paused = false` at flip time fires `_playParentMedia` immediately. - **`playbackerror`** **surface** on parent proxy rejection with the right `source` tag. ## Minor - One-line comment on `_promoteToParentProxy` explaining the `postMessage` async race (the mute lands after ~one message-loop tick; the autoplay gate that triggered promotion keeps the iframe rejecting `play()` during that window, so the double-play bug doesn't reappear). ## What's good (from the review) Kept as-is — noted for posterity: - `muted` vs `volume` framing (orthogonal channels). - Probing reality via `NotAllowedError` instead of `matchMedia('(pointer: coarse)')` / UA sniffing. - Two orthogonal mute channels. - Backwards compat (new actions / messages safely ignored by either side). ## Test results - `packages/core/src/runtime/media.test.ts` — **42 tests pass** (+4 new: `userMuted` sticky, OR invariant, fires-every-rejection, caller-latch dedupe) - `packages/core/src/runtime/bridge.test.ts` — **15 tests pass** - `packages/player/src/hyperframes-player.test.ts` — **26 tests pass** (+3 new: `audioownershipchange` dispatch, mid-playback promotion, `playbackerror` surface) - Typecheck green on `core` + `player` - `tsup` build green on `core` / `player` / `cli` - Live factory-series-c-video repro via agent-browser: runtime ownership still zero `volumechange` thrash, zero `PARENT.play()` calls; parent ownership measures 27–37 ms steady-state drift, well inside the 50 ms threshold. ## Test plan - [x] Unit tests (83 total across touched files) - [x] Typecheck clean - [x] Build clean - [x] Live studio repro on factory-series-c-video: runtime path unchanged, parent path drift tightened - [x] `audioownershipchange` event fires with correct detail on synthetic autoplay block - [x] Physical iOS / Android device verification (unchanged since #298)
This commit is contained in:
@@ -1190,6 +1190,7 @@ export function initSandboxRuntimeModular(): void {
|
||||
playing: state.isPlaying,
|
||||
playbackRate: state.playbackRate,
|
||||
outputMuted: state.mediaOutputMuted,
|
||||
userMuted: state.bridgeMuted,
|
||||
onAutoplayBlocked: () => {
|
||||
if (state.mediaAutoplayBlockedPosted) return;
|
||||
state.mediaAutoplayBlockedPosted = true;
|
||||
|
||||
@@ -458,4 +458,121 @@ describe("syncRuntimeMedia", () => {
|
||||
await Promise.resolve();
|
||||
expect(onAutoplayBlocked).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("asserts muted=true every tick while userMuted is set", () => {
|
||||
// Mirror of the `outputMuted` test — user preference must be sticky
|
||||
// too. A sub-composition that activates after the user mutes should
|
||||
// inherit the silence, not briefly play at author volume before the
|
||||
// next bridge message lands.
|
||||
const clip = createMockClip({ start: 0, end: 10, volume: 1 });
|
||||
Object.defineProperty(clip.el, "readyState", { value: 4, writable: true });
|
||||
Object.defineProperty(clip.el, "muted", { value: false, writable: true });
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
userMuted: true,
|
||||
});
|
||||
expect(clip.el.muted).toBe(true);
|
||||
});
|
||||
|
||||
it("fires onAutoplayBlocked for every rejected play (caller owns the latch)", async () => {
|
||||
// media.ts is intentionally memoryless — each NotAllowedError rejection
|
||||
// invokes the callback. The init.ts caller wraps with
|
||||
// `mediaAutoplayBlockedPosted` so the outbound message is posted at most
|
||||
// once per session. This test pins down the contract (fires always) so
|
||||
// a future refactor can't quietly add deduplication here and break the
|
||||
// caller's latching logic.
|
||||
const clip = createMockClip({ start: 0, end: 10 });
|
||||
Object.defineProperty(clip.el, "readyState", { value: 4, writable: true });
|
||||
const rejection = Object.assign(new Error("blocked"), { name: "NotAllowedError" });
|
||||
clip.el.play = vi.fn(() => Promise.reject(rejection));
|
||||
const onAutoplayBlocked = vi.fn();
|
||||
|
||||
// Simulate two ticks — between them `playRequested` clears so play() runs
|
||||
// again and rejects again.
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
onAutoplayBlocked,
|
||||
});
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5.05,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
onAutoplayBlocked,
|
||||
});
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
|
||||
// No latch inside media.ts — two rejections, two callback invocations.
|
||||
// The caller's latch is what prevents a second outbound message.
|
||||
expect(onAutoplayBlocked).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("caller-side latch pattern posts once across many rejections", async () => {
|
||||
// Mirrors what init.ts does: the onAutoplayBlocked wrapper checks and
|
||||
// sets a boolean flag so the outbound post fires exactly once even if
|
||||
// the raw callback fires many times. Regression guard for the latch
|
||||
// wiring in the init.ts handler.
|
||||
const clip = createMockClip({ start: 0, end: 10 });
|
||||
Object.defineProperty(clip.el, "readyState", { value: 4, writable: true });
|
||||
const rejection = Object.assign(new Error("blocked"), { name: "NotAllowedError" });
|
||||
clip.el.play = vi.fn(() => Promise.reject(rejection));
|
||||
|
||||
let posted = 0;
|
||||
const state = { latched: false };
|
||||
const wrapped = () => {
|
||||
if (state.latched) return;
|
||||
state.latched = true;
|
||||
posted += 1;
|
||||
};
|
||||
|
||||
for (let i = 0; i < 5; i++) {
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5 + i * 0.05,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
onAutoplayBlocked: wrapped,
|
||||
});
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
}
|
||||
|
||||
expect(posted).toBe(1);
|
||||
});
|
||||
|
||||
it("mutes when either outputMuted OR userMuted is true (OR invariant)", () => {
|
||||
// Explicit validation of the combined-flag contract: setting one to
|
||||
// false while the other is true must keep the element muted.
|
||||
const clip = createMockClip({ start: 0, end: 10, volume: 1 });
|
||||
Object.defineProperty(clip.el, "readyState", { value: 4, writable: true });
|
||||
Object.defineProperty(clip.el, "muted", { value: false, writable: true });
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
outputMuted: false,
|
||||
userMuted: true,
|
||||
});
|
||||
expect(clip.el.muted).toBe(true);
|
||||
Object.defineProperty(clip.el, "muted", { value: false, writable: true });
|
||||
syncRuntimeMedia({
|
||||
clips: [clip],
|
||||
timeSeconds: 5,
|
||||
playing: true,
|
||||
playbackRate: 1,
|
||||
outputMuted: true,
|
||||
userMuted: false,
|
||||
});
|
||||
expect(clip.el.muted).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -93,13 +93,18 @@ export function syncRuntimeMedia(params: {
|
||||
playing: boolean;
|
||||
playbackRate: number;
|
||||
/**
|
||||
* When `true`, assert `el.muted = true` on every active media element on
|
||||
* every tick. Sticky against newly-discovered media (sub-composition
|
||||
* activation, dynamic DOM) so the parent-frame audio-owner invariant holds.
|
||||
* `false` is a no-op — we don't un-mute, because other code paths
|
||||
* (`<audio muted>` author intent, `onSetMuted`) own the un-mute decision.
|
||||
* Parent-frame audio-owner has taken over audible playback. Assert
|
||||
* `el.muted = true` on every active media element per tick so that any
|
||||
* sub-composition media inserted mid-playback inherits the silence.
|
||||
*/
|
||||
outputMuted?: boolean;
|
||||
/**
|
||||
* User's explicit mute preference (set via `onSetMuted`). Symmetric to
|
||||
* `outputMuted` — also asserted per tick — so a sub-composition that
|
||||
* activates after the user mutes doesn't briefly play at author volume
|
||||
* before the next bridge message lands.
|
||||
*/
|
||||
userMuted?: boolean;
|
||||
/**
|
||||
* Invoked at most once when a media element's `play()` promise rejects with
|
||||
* `NotAllowedError`. The caller is expected to latch and post a single
|
||||
@@ -107,6 +112,9 @@ export function syncRuntimeMedia(params: {
|
||||
*/
|
||||
onAutoplayBlocked?: () => void;
|
||||
}): void {
|
||||
// Either flag silences output. Combined up front so the per-clip loop is
|
||||
// a single branch instead of two.
|
||||
const shouldMute = !!(params.outputMuted || params.userMuted);
|
||||
for (const clip of params.clips) {
|
||||
const { el } = clip;
|
||||
if (!el.isConnected) continue;
|
||||
@@ -122,7 +130,7 @@ export function syncRuntimeMedia(params: {
|
||||
}
|
||||
}
|
||||
if (clip.volume != null) el.volume = clip.volume;
|
||||
if (params.outputMuted) el.muted = true;
|
||||
if (shouldMute) el.muted = true;
|
||||
try {
|
||||
// Per-element rate × global transport rate
|
||||
el.playbackRate = clip.playbackRate * params.playbackRate;
|
||||
|
||||
Reference in New Issue
Block a user