From 2c021c48dd002510d6dd5eb6c3ef72db81d7a139 Mon Sep 17 00:00:00 2001 From: Carlos Alcaraz <193642530+calcarazgre646@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:16:24 -0300 Subject: [PATCH] fix(player): replace or clear the audio-src proxy instead of stacking setupFromUrl (the audio-src attribute path) blindly created a new proxy on every call, and the attribute had no removal handling. Changing audio-src left the previous proxy in the pool, where it kept preloading and played in parallel with the new one; removing audio-src left it playing entirely (srcdoc already handled removal, audio-src did not). Track the attribute's proxy in _urlAudioEntry/_urlAudioSrc: re-setting a different URL tears the previous one down first, re-setting the same URL is a no-op, and removing the attribute routes to a new teardownUrlAudio(). If the URL is already adopted by the composition, dedup wins and no separate proxy is tracked, so teardown targets the tracked proxy by reference and never removes the composition's own clip. A mid-playback swap under parent ownership brings the fresh proxy online so it is not silent until the next play tick. The proxy still persists across iframe reloads, since audio-src is a player-level attribute. Also extracts a shared _resolveIframeMediaSrc helper (the adopt/detach paths duplicated the URL resolution) and drops an unused getter. Adds parent-media tests: replace-not-stack, same-URL no-op, teardown clears, teardown removes only the tracked proxy, safe teardown with nothing set, and no duplication/hijack when the composition already owns the same clip. --- packages/player/src/hyperframes-player.ts | 2 + packages/player/src/parent-media.test.ts | 99 +++++++++++++++++++++++ packages/player/src/parent-media.ts | 67 +++++++++++---- 3 files changed, 154 insertions(+), 14 deletions(-) create mode 100644 packages/player/src/parent-media.test.ts diff --git a/packages/player/src/hyperframes-player.ts b/packages/player/src/hyperframes-player.ts index 29848bb5e..70192a2c2 100644 --- a/packages/player/src/hyperframes-player.ts +++ b/packages/player/src/hyperframes-player.ts @@ -165,6 +165,7 @@ class HyperframesPlayer extends HTMLElement { this.controlsApi?.destroy(); } + // fallow-ignore-next-line complexity attributeChangedCallback(name: string, _old: string | null, val: string | null) { switch (name) { case "src": @@ -222,6 +223,7 @@ class HyperframesPlayer extends HTMLElement { } case "audio-src": if (val) this._media.setupFromUrl(val); + else this._media.teardownUrlAudio(); break; case SHADER_CAPTURE_SCALE_ATTR: case SHADER_LOADING_ATTR: diff --git a/packages/player/src/parent-media.test.ts b/packages/player/src/parent-media.test.ts new file mode 100644 index 000000000..347f3cf45 --- /dev/null +++ b/packages/player/src/parent-media.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect } from "vitest"; +import { ParentMediaManager, type ProxyEntry } from "./parent-media"; + +function makeManager(overrides: Partial<{ isPaused: boolean; owner: "runtime" | "parent" }> = {}) { + const mgr = new ParentMediaManager({ + dispatchEvent: () => {}, + getMuted: () => false, + getVolume: () => 1, + getPlaybackRate: () => 1, + getCurrentTime: () => 0, + isPaused: () => overrides.isPaused ?? true, + }); + return mgr; +} + +describe("ParentMediaManager audio-src proxy lifecycle", () => { + it("replaces the audio-src proxy instead of stacking a second one", () => { + const mgr = makeManager(); + mgr.setupFromUrl("https://example.test/a.mp3"); + expect(mgr.entries).toHaveLength(1); + + mgr.setupFromUrl("https://example.test/b.mp3"); + // The old proxy must be gone, not accumulated alongside the new one. + expect(mgr.entries).toHaveLength(1); + expect(mgr.entries[0].el.src).toBe("https://example.test/b.mp3"); + }); + + it("is a no-op when the same audio-src URL is set again", () => { + const mgr = makeManager(); + mgr.setupFromUrl("https://example.test/a.mp3"); + const first = mgr.entries[0]; + + mgr.setupFromUrl("https://example.test/a.mp3"); + expect(mgr.entries).toHaveLength(1); + // Same element reference — not torn down and rebuilt. + expect(mgr.entries[0]).toBe(first); + }); + + it("clears the audio-src proxy on teardownUrlAudio", () => { + const mgr = makeManager(); + mgr.setupFromUrl("https://example.test/a.mp3"); + const el = mgr.entries[0].el; + + mgr.teardownUrlAudio(); + expect(mgr.entries).toHaveLength(0); + // The proxy's source is reset so it stops preloading. + expect(el.src).not.toBe("https://example.test/a.mp3"); + }); + + it("teardownUrlAudio removes only the url proxy, leaving other entries", () => { + const mgr = makeManager(); + // Simulate an iframe-adopted entry already in the pool. + const adopted: ProxyEntry = { + el: new Audio(), + start: 0, + duration: Infinity, + driftSamples: 0, + }; + adopted.el.src = "https://example.test/iframe-clip.mp4"; + mgr.entries.push(adopted); + + mgr.setupFromUrl("https://example.test/a.mp3"); + expect(mgr.entries).toHaveLength(2); + + mgr.teardownUrlAudio(); + expect(mgr.entries).toHaveLength(1); + expect(mgr.entries[0]).toBe(adopted); + }); + + it("teardownUrlAudio is safe to call with no audio-src set", () => { + const mgr = makeManager(); + expect(() => mgr.teardownUrlAudio()).not.toThrow(); + expect(mgr.entries).toHaveLength(0); + }); + + it("does not duplicate or hijack a clip the composition already owns", () => { + const mgr = makeManager(); + // The composition already adopted a clip with this URL. + const adopted: ProxyEntry = { + el: new Audio(), + start: 0, + duration: Infinity, + driftSamples: 0, + }; + adopted.el.src = "https://example.test/shared.mp3"; + mgr.entries.push(adopted); + + // Pointing audio-src at the same URL must not create a second proxy... + mgr.setupFromUrl("https://example.test/shared.mp3"); + expect(mgr.entries).toHaveLength(1); + expect(mgr.entries[0]).toBe(adopted); + + // ...and removing audio-src must not tear down the composition's own clip + // (teardown targets the tracked proxy by reference, not by URL match). + mgr.teardownUrlAudio(); + expect(mgr.entries).toHaveLength(1); + expect(mgr.entries[0]).toBe(adopted); + }); +}); diff --git a/packages/player/src/parent-media.ts b/packages/player/src/parent-media.ts index 84fccf3dd..e4283b3ea 100644 --- a/packages/player/src/parent-media.ts +++ b/packages/player/src/parent-media.ts @@ -43,6 +43,10 @@ export class ParentMediaManager { private _mediaObserver?: MutationObserver; private _playbackErrorPosted = false; private _audioOwner: "runtime" | "parent" = "runtime"; + /** The proxy created from the `audio-src` attribute, tracked so it can be + * replaced or cleared instead of accumulating on every attribute change. */ + private _urlAudioEntry: ProxyEntry | null = null; + private _urlAudioSrc: string | null = null; private readonly _dispatchEvent: (event: Event) => void; private readonly _getMuted: () => boolean; @@ -76,10 +80,6 @@ export class ParentMediaManager { return this._entries; } - get playbackErrorPosted(): boolean { - return this._playbackErrorPosted; - } - resetForIframeLoad(): void { this._playbackErrorPosted = false; const wasPromoted = this._audioOwner === "parent"; @@ -102,6 +102,8 @@ export class ParentMediaManager { m.el.src = ""; } this._entries = []; + this._urlAudioEntry = null; + this._urlAudioSrc = null; } updateMuted(muted: boolean): void { @@ -212,9 +214,41 @@ export class ParentMediaManager { this._observeDynamicMedia(iframeDoc); } - /** Set up a single proxy from an explicit URL (the `audio-src` attribute path). */ + /** + * Set (or replace) the parent-frame audio proxy driven by the `audio-src` + * attribute. Re-setting with a different URL tears down the previous proxy + * first, so changing `audio-src` swaps the track instead of stacking a + * second one that keeps preloading and plays in parallel. + */ setupFromUrl(audioSrc: string): void { - this._createEntry(audioSrc, "audio", 0, Infinity); + if (this._urlAudioSrc === audioSrc && this._urlAudioEntry) return; + this.teardownUrlAudio(); + const entry = this._createEntry(audioSrc, "audio", 0, Infinity); + // `_createEntry` returns null when a proxy for this URL already exists + // (e.g. the composition already adopted the same media). In that case we do + // not own a proxy, so leave the tracking cleared rather than recording a + // src with no entry — otherwise teardown would target nothing and the + // no-op guard would never engage. + this._urlAudioEntry = entry; + this._urlAudioSrc = entry ? audioSrc : null; + // If the parent already owns playback, bring the fresh proxy online so a + // mid-playback swap is not silent until the next play tick. + if (entry && this._audioOwner === "parent" && !this._isPaused()) { + this.mirrorTime(this._getCurrentTime(), { force: true }); + this.playAll(); + } + } + + /** Tear down the `audio-src` proxy (used when the attribute is removed). */ + teardownUrlAudio(): void { + const entry = this._urlAudioEntry; + this._urlAudioEntry = null; + this._urlAudioSrc = null; + if (!entry) return; + entry.el.pause(); + entry.el.src = ""; + const idx = this._entries.indexOf(entry); + if (idx !== -1) this._entries.splice(idx, 1); } teardownObserver(): void { @@ -258,16 +292,22 @@ export class ParentMediaManager { return entry; } + /** Resolve an iframe media element's source to an absolute URL, or null. */ + private _resolveIframeMediaSrc(iframeEl: HTMLMediaElement): string | null { + const rawSrc = + iframeEl.getAttribute("src") || iframeEl.querySelector("source")?.getAttribute("src"); + return rawSrc ? new URL(rawSrc, iframeEl.ownerDocument.baseURI).href : null; + } + + // fallow-ignore-next-line complexity private _adoptIframeMedia(iframeEl: HTMLMediaElement): void { // Skip elements the preloader has demoted — the observer will re-trigger // when the preload attribute is promoted to "auto". if (iframeEl.preload === "metadata" || iframeEl.preload === "none") return; - const rawSrc = - iframeEl.getAttribute("src") || iframeEl.querySelector("source")?.getAttribute("src"); - if (!rawSrc) return; + const src = this._resolveIframeMediaSrc(iframeEl); + if (!src) return; - const src = new URL(rawSrc, iframeEl.ownerDocument.baseURI).href; const start = parseFloat(iframeEl.getAttribute("data-start") || "0"); const duration = parseFloat(iframeEl.getAttribute("data-duration") || "Infinity"); const tag = iframeEl.tagName === "VIDEO" ? ("video" as const) : ("audio" as const); @@ -285,10 +325,8 @@ export class ParentMediaManager { } private _detachIframeMedia(iframeEl: HTMLMediaElement): void { - const rawSrc = - iframeEl.getAttribute("src") || iframeEl.querySelector("source")?.getAttribute("src"); - if (!rawSrc) return; - const src = new URL(rawSrc, iframeEl.ownerDocument.baseURI).href; + const src = this._resolveIframeMediaSrc(iframeEl); + if (!src) return; const idx = this._entries.findIndex((m) => m.el.src === src); if (idx === -1) return; const entry = this._entries[idx]; @@ -301,6 +339,7 @@ export class ParentMediaManager { this.teardownObserver(); if (typeof MutationObserver === "undefined" || !doc.body) return; + // fallow-ignore-next-line complexity const obs = new MutationObserver((mutations) => { for (const m of mutations) { if (m.type === "attributes" && m.attributeName === "preload") {