mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
fix(player): one owner for document reassignment, stop the clock on runtime pause
Review follow-up on two holes in the previous commit. The `_onIframeLoad` early return rests on "a new document is always preceded by assigning src/srcdoc, which clears readiness first". That was true of the two attributeChangedCallback branches and false of `_reloadShaderOptions`, which reassigns the document on a shader-option change without touching readiness. Flipping `shader-loading` or `shader-capture-scale` on a ready player therefore loaded a genuinely new document that the load handler then skipped the teardown for, leaving a stale bridge flag and no probe running, so it had no path to ready at all. All six reassignment sites now route through `_setIframeSrc` / `_setIframeSrcdoc`, which own clearing readiness, so the load handler's test holds by construction rather than by convention. Collapsing the tick loop to a single owner also removed an accidental backstop. `_paused` was gating the loop, so a runtime that stopped itself short of the end used to end it; the replacement only covered end-of-timeline, which needs `currentTime >= duration`. A composition calling `pause()` on `window.__player` left the loop scheduled forever. That is a leaked rAF and not a divergence: `createRuntimePlayer` returns its transport-backed object whenever a transport is passed, and the runtime always passes one, so the `setIsPlaying` seam that writes `state.isPlaying` without touching the clock is unreachable there. Every `state.isPlaying = false` in the runtime sits in the transport beside a `clock.pause()`, so the composition cannot advance while the player reports paused. The fix covers the whole class instead of the one instance: the raw `isPlaying` from the wire is forwarded to the player, which stops the clock on any runtime-reported stop. A report seen before the runtime has echoed our own play is ignored, since postMessage delivery between two windows is ordered and such a report was posted before the play arrived — that is what keeps the original stale-message immunity intact. All four playback configurations re-measured and unchanged: 1x throughout, ended at ~14.99s of a 15s composition, clock started once and stopped once.
This commit is contained in:
@@ -2514,7 +2514,9 @@ describe("HyperframesPlayer parent tick clock lifetime", () => {
|
||||
_duration: number;
|
||||
_paused: boolean;
|
||||
_parentTickRaf: number | null;
|
||||
_runtimeBridgeReady: boolean;
|
||||
_onMessage: (event: MessageEvent) => void;
|
||||
probe: { start: () => void };
|
||||
};
|
||||
|
||||
let player: PlayerInternal;
|
||||
@@ -2596,10 +2598,49 @@ describe("HyperframesPlayer parent tick clock lifetime", () => {
|
||||
expect(tickCount()).toBe(1);
|
||||
});
|
||||
|
||||
it("stops ticking when the composition pauses itself short of the end", () => {
|
||||
player.play();
|
||||
advanceFrame();
|
||||
expect(tickCount()).toBe(1);
|
||||
// The runtime confirms the play, so the next not-playing report is a real
|
||||
// stop rather than a message that crossed our own play command.
|
||||
player._onMessage(stateMessage(30, true));
|
||||
|
||||
// Composition code calling pause() on `window.__player`: mid-timeline, so
|
||||
// the completed-playback path never runs.
|
||||
player._onMessage(stateMessage(60, false));
|
||||
advanceFrame();
|
||||
advanceFrame();
|
||||
|
||||
expect(player._parentTickRaf).toBeNull();
|
||||
expect(tickCount()).toBe(1);
|
||||
});
|
||||
|
||||
it("keeps ticking after a shader-option reload starts a new document", () => {
|
||||
// _reloadShaderOptions reassigns the iframe document. If that path does not
|
||||
// clear readiness, the load handler skips the teardown and the fresh
|
||||
// document is left with no probe and a stale bridge flag.
|
||||
player.setAttribute("src", "https://composition.example/comp.html");
|
||||
player._ready = true;
|
||||
|
||||
const probeStart = vi.spyOn(player.probe, "start");
|
||||
|
||||
player.setAttribute("shader-loading", "eager");
|
||||
expect(player._ready).toBe(false);
|
||||
|
||||
player.iframe.dispatchEvent(new Event("load"));
|
||||
|
||||
expect(probeStart).toHaveBeenCalled();
|
||||
expect(player._runtimeBridgeReady).toBe(false);
|
||||
});
|
||||
|
||||
it("stops ticking once the composition reports the end of the timeline", () => {
|
||||
player.play();
|
||||
advanceFrame();
|
||||
expect(tickCount()).toBe(1);
|
||||
// Playback reports itself as it runs; the runtime cannot reach the end of a
|
||||
// timeline without having said it was playing on the way there.
|
||||
player._onMessage(stateMessage(30, true));
|
||||
|
||||
// frame 450 at the default 30fps protocol rate = 15s = the full duration.
|
||||
player._onMessage(stateMessage(450, false));
|
||||
|
||||
@@ -99,6 +99,11 @@ class HyperframesPlayer extends HTMLElement {
|
||||
private _directTimelineAdapter: DirectTimelineAdapter | null = null;
|
||||
private _directTimelineClock: DirectTimelineClock;
|
||||
private _parentTickRaf: number | null = null;
|
||||
/** True between sending "play" and the runtime echoing that it is playing.
|
||||
* postMessage delivery between two windows is ordered, so an
|
||||
* `isPlaying: false` seen inside that window was posted before our play
|
||||
* command arrived and describes the state we just left, not a stop. */
|
||||
private _awaitingPlayEcho = false;
|
||||
private _media: ParentMediaManager;
|
||||
private _scenes: { id: string; start: number; duration: number }[] = [];
|
||||
private _runtimeFps = 30;
|
||||
@@ -170,10 +175,8 @@ class HyperframesPlayer extends HTMLElement {
|
||||
if (this.hasAttribute("poster"))
|
||||
this.posterEl = setupPoster(this.shadow, this.getAttribute("poster"), this.posterEl);
|
||||
if (this.hasAttribute("audio-src")) this._media.setupFromUrl(this.getAttribute("audio-src")!);
|
||||
if (this.hasAttribute("srcdoc"))
|
||||
this.iframe.srcdoc = prepareSrcdocForElement(this, this.getAttribute("srcdoc")!);
|
||||
if (this.hasAttribute("src"))
|
||||
this.iframe.src = prepareSrcForElement(this, this.getAttribute("src")!);
|
||||
if (this.hasAttribute("srcdoc")) this._setIframeSrcdoc(this.getAttribute("srcdoc")!);
|
||||
if (this.hasAttribute("src")) this._setIframeSrc(this.getAttribute("src")!);
|
||||
|
||||
// Host-environment audio lock: when the embedding host (e.g. Claude
|
||||
// desktop) drops the `audio-locked` attribute, attributeChangedCallback
|
||||
@@ -206,17 +209,10 @@ class HyperframesPlayer extends HTMLElement {
|
||||
attributeChangedCallback(name: string, _old: string | null, val: string | null) {
|
||||
switch (name) {
|
||||
case "src":
|
||||
if (val) {
|
||||
this._ready = false;
|
||||
this._runtimeBridgeReady = false;
|
||||
this.iframe.src = prepareSrcForElement(this, val);
|
||||
}
|
||||
if (val) this._setIframeSrc(val);
|
||||
break;
|
||||
case "srcdoc":
|
||||
this._ready = false;
|
||||
this._runtimeBridgeReady = false;
|
||||
if (val !== null) this.iframe.srcdoc = prepareSrcdocForElement(this, val);
|
||||
else this.iframe.removeAttribute("srcdoc");
|
||||
this._setIframeSrcdoc(val);
|
||||
break;
|
||||
// Reject NaN/zero/negative dimensions the same way the composition
|
||||
// probe does (a typo like width="abc" or width="0" would otherwise
|
||||
@@ -299,6 +295,7 @@ class HyperframesPlayer extends HTMLElement {
|
||||
this._paused = false;
|
||||
const directTimelineStarted = this._tryDirectTimelinePlay();
|
||||
if (!directTimelineStarted) {
|
||||
this._awaitingPlayEcho = true;
|
||||
this._sendControl("play");
|
||||
// Only start the parent tick clock once the composition is ready and
|
||||
// confirmed on the runtime bridge path (not the direct-timeline path).
|
||||
@@ -643,14 +640,40 @@ class HyperframesPlayer extends HTMLElement {
|
||||
private _reloadShaderOptions(): void {
|
||||
if (getShaderModeFromElement(this) !== "player") this.shaderLoader.reset();
|
||||
if (this.hasAttribute("srcdoc")) {
|
||||
this.iframe.srcdoc = prepareSrcdocForElement(this, this.getAttribute("srcdoc") || "");
|
||||
this._setIframeSrcdoc(this.getAttribute("srcdoc") || "");
|
||||
return;
|
||||
}
|
||||
if (this.hasAttribute("src")) {
|
||||
this.iframe.src = prepareSrcForElement(this, this.getAttribute("src") || "");
|
||||
this._setIframeSrc(this.getAttribute("src") || "");
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Point the iframe at a new `src` document.
|
||||
*
|
||||
* Every reassignment of the iframe's document goes through here or
|
||||
* `_setIframeSrcdoc`, and clearing readiness is theirs alone. That is what
|
||||
* makes `_onIframeLoad`'s test sound: a `load` seen while still ready can
|
||||
* only be the late load of the document already playing, because starting a
|
||||
* new one clears readiness first. The shader-option reload is why this is a
|
||||
* method rather than a convention — it reassigns the document too, and when
|
||||
* it did so without clearing readiness the load handler skipped the teardown
|
||||
* and left the fresh document with a stale bridge flag and no probe running.
|
||||
*/
|
||||
private _setIframeSrc(src: string): void {
|
||||
this._ready = false;
|
||||
this._runtimeBridgeReady = false;
|
||||
this.iframe.src = prepareSrcForElement(this, src);
|
||||
}
|
||||
|
||||
/** Point the iframe at a new `srcdoc` document, or clear it. See `_setIframeSrc`. */
|
||||
private _setIframeSrcdoc(html: string | null): void {
|
||||
this._ready = false;
|
||||
this._runtimeBridgeReady = false;
|
||||
if (html === null) this.iframe.removeAttribute("srcdoc");
|
||||
else this.iframe.srcdoc = prepareSrcdocForElement(this, html);
|
||||
}
|
||||
|
||||
private _trySyncSeek(timeInSeconds: number): boolean {
|
||||
try {
|
||||
const win = this.iframe.contentWindow as
|
||||
@@ -711,9 +734,13 @@ class HyperframesPlayer extends HTMLElement {
|
||||
* (it has no restart path) while the player still reports `paused === false`.
|
||||
* Cross-origin that leaves nothing driving the composition at all, because
|
||||
* the throttled iframe rAF this clock exists to replace is not running
|
||||
* either. Every transition out of playback — pause(), seek(), a new document,
|
||||
* disconnect, and the end of the timeline — calls `_stopParentTickClock`
|
||||
* directly instead.
|
||||
* either. Instead the two ways playback can end each stop it explicitly: the
|
||||
* player's own pause(), seek(), new-document and disconnect paths call
|
||||
* `_stopParentTickClock` directly, and a stop the runtime initiates for
|
||||
* itself — the end of the timeline, or composition code calling pause() on
|
||||
* `window.__player` — arrives as a playback report and is relayed through
|
||||
* `onRuntimePlaybackReport`. Enumerating only the first group is what left
|
||||
* the loop running after a runtime-side pause.
|
||||
*/
|
||||
private _startParentTickClock(): void {
|
||||
this._stopParentTickClock();
|
||||
@@ -774,7 +801,10 @@ class HyperframesPlayer extends HTMLElement {
|
||||
play: () => this.play(),
|
||||
getLoop: () => this.loop,
|
||||
media: this._media,
|
||||
stopPlaybackClock: () => this._stopParentTickClock(),
|
||||
onRuntimePlaybackReport: (isPlaying) => {
|
||||
if (isPlaying) this._awaitingPlayEcho = false;
|
||||
else if (!this._awaitingPlayEcho) this._stopParentTickClock();
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -25,11 +25,12 @@ export interface PlaybackStateCallbacks {
|
||||
play: () => void;
|
||||
getLoop: () => boolean;
|
||||
media: ParentMediaManager;
|
||||
/** End the parent-driven tick clock. Reaching the end of the timeline is the
|
||||
* only playback stop the runtime initiates by itself, so it is the only one
|
||||
* that has to be relayed here; every other stop already goes through the
|
||||
* player's own pause() / seek() / teardown paths. */
|
||||
stopPlaybackClock: () => void;
|
||||
/** The runtime's own view of whether it is playing, forwarded verbatim from
|
||||
* the wire before any of this function's interpretation of it. The player
|
||||
* uses it to decide the fate of the parent-driven tick clock, which is the
|
||||
* one piece of state a runtime-initiated stop must reach: a stop the player
|
||||
* itself performs already goes through pause() / seek() / teardown. */
|
||||
onRuntimePlaybackReport: (isPlaying: boolean) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -43,6 +44,12 @@ export function applyRuntimeStateMessage(
|
||||
current: PlaybackState,
|
||||
callbacks: PlaybackStateCallbacks,
|
||||
): PlaybackState {
|
||||
// Before any interpretation: a runtime that reports itself stopped is the
|
||||
// only stop the player cannot see coming, and end-of-timeline is just one
|
||||
// instance of it. Forwarding the raw flag here covers the whole class,
|
||||
// including a composition calling pause() on `window.__player` itself.
|
||||
callbacks.onRuntimePlaybackReport(data.isPlaying);
|
||||
|
||||
const rawTime = (data.frame ?? 0) / fps;
|
||||
const currentTime = current.duration > 0 ? Math.min(rawTime, current.duration) : rawTime;
|
||||
const wasPlaying = !current.paused;
|
||||
@@ -81,7 +88,6 @@ export function applyRuntimeStateMessage(
|
||||
|
||||
if (completedPlayback) {
|
||||
if (callbacks.media.audioOwner === "parent") callbacks.media.pauseAll();
|
||||
callbacks.stopPlaybackClock();
|
||||
next.paused = true;
|
||||
callbacks.updateControlsPlaying(false);
|
||||
callbacks.dispatchEvent(new Event("ended"));
|
||||
|
||||
@@ -27,7 +27,7 @@ const makeCallbacks = (): MessageHandlerCallbacks => ({
|
||||
setCompositionSize: vi.fn(),
|
||||
sendControl: vi.fn(),
|
||||
getIframeDoc: vi.fn(() => null),
|
||||
stopPlaybackClock: vi.fn(),
|
||||
onRuntimePlaybackReport: vi.fn(),
|
||||
});
|
||||
|
||||
const stageSizeEvent = (width: unknown, height: unknown, source: object): MessageEvent =>
|
||||
|
||||
@@ -509,7 +509,7 @@ describe("handleRuntimeMessage scenes seam", () => {
|
||||
seek: () => {},
|
||||
play: () => {},
|
||||
getLoop: () => false,
|
||||
stopPlaybackClock: () => {},
|
||||
onRuntimePlaybackReport: () => {},
|
||||
media: {
|
||||
audioOwner: "iframe",
|
||||
promoteToParentProxy: () => {},
|
||||
|
||||
Reference in New Issue
Block a user