From 63487c57756957c7c1365ba45bd13d3277177abf Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 5 Jul 2026 16:12:41 +0800 Subject: [PATCH] =?UTF-8?q?fix(audio):=20seven=20audit=20findings=20?= =?UTF-8?q?=E2=80=94=20loading,=20retries,=20API=20contract=20honesty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - load() gains the already-loaded guard every other parser has: re-preloading a manifest used to silently replace each Howl and leak the old instance's decoded buffers / HTML5 nodes. Returns 0 (cached). - soundLoadError retries are budgeted PER SOUND (state.retryCounters keyed by name): the single shared counter let parallel loads steal each other's retries — one flaky file pushed another sound's first failure over the give-up threshold, a spurious fatal preload error under the default stopOnAudioError. - new me.audio.setStopOnAudioError(value): the flag was documented as settable, but it's a module export — assigning through the namespace throws a TypeError, so the "disable audio instead of throwing" mode was unreachable. The setter is the supported way to change it. - seek()/rate() get honest overloads (the #1456 stereo/position pattern): the setter form returned Howler's Howl object while typed number. Arity-exact forwarding — Howler's core methods dispatch on argument count, so a trailing explicit undefined would parse as an id. - position()/stereo() getters returned Howler's internal null for sounds never positioned/panned; now neutral defaults ([0,0,0] / 0). - resume() without id resumes EVERY paused instance, as its JSDoc always promised: Howler's bare play() only auto-resumes when exactly one instance is paused — with 2+ it spawned a new instance from 0 and left the paused ones stuck. Reads Howl._sounds (no public list; same justified private access as the Howler _muted read). - unload() clears state.currentTrackId when it referenced the unloaded clip, so getCurrentTrack()/pauseTrack()/resumeTrack() don't act on a ghost. tests/audio-audit.spec.js written failing-first: 7/7 red on the old code, 7/7 green after. Full suite 4604 passed / 0 failed. The resume test asserts on the deterministic instance count (a locked headless audio context doesn't flip _paused synchronously). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A --- packages/melonjs/CHANGELOG.md | 7 + packages/melonjs/src/audio/audio.ts | 7 + packages/melonjs/src/audio/backend.ts | 35 +++- packages/melonjs/src/audio/playback.ts | 108 +++++++++-- packages/melonjs/tests/audio-audit.spec.js | 200 +++++++++++++++++++++ 5 files changed, 339 insertions(+), 18 deletions(-) create mode 100644 packages/melonjs/tests/audio-audit.spec.js diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index a93502e124..6a4a291506 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -25,6 +25,13 @@ - **`moveTowards()` never reached its target — and returned the target instead of `this`** (all four vector classes) — the arrival test compared the remaining distance against `step * step` (wrong units: snapped way too early for steps above 1, dithered around the target forever for fractional steps), and on "arrival" it returned the **caller's target vector** without moving `this` at all — so the vector never landed, and chaining onto the result silently mutated the target. It now lands exactly on the target and always returns `this`, per its own documentation; negative-step flee semantics are unchanged. - **Every preloaded video was fetched with forced anonymous CORS** — the video parser unconditionally stamped the `crossorigin` attribute with the loader's `crossOrigin` setting, which defaults to `undefined` — and per the HTML spec an *invalid* value (the string `"undefined"`) maps to `anonymous`, unlike a *missing* attribute (no CORS). Cross-origin videos served without CORS headers failed to load outright when they would have played fine untainted. The attribute is now only set when `crossOrigin` is actually configured (the empty string remains a valid, honored value). - **Video preloading hung forever on autoplay-restricted browsers (e.g. iOS Safari) unless `autoplay: false` was spelled out** — the parser waited for `canplay`, which those browsers never fire for a video that isn't allowed to play, and only an *explicit* `autoplay: false` opted into the reliable `loadedmetadata` path — so the common manifest shape (no `autoplay` key) stalled the whole preloader and the game never started. Preloading now completes on `loadedmetadata` unless `autoplay: true` explicitly asks to wait for `canplay`. +- **Re-preloading an audio manifest silently replaced each Howl and leaked the old one** — the audio parser was the only asset parser without an already-loaded guard, so returning to a stage that preloads (a common pattern) churned every clip's decoded buffers / HTML5 nodes. An already-loaded clip is now reported as cached (like every other asset type); `me.audio.unload()` first to genuinely reload one. +- **Parallel audio loads shared a single retry counter** — three failures of one flaky file pushed *another* sound's first failure straight over the give-up threshold, throwing a spurious fatal load error (the default `stopOnAudioError`). Retries are now budgeted per sound. +- **`me.audio.stopOnAudioError` was documented as settable but couldn't be set** — it's a module export, and assigning through the namespace throws a TypeError, so the documented "disable audio instead of throwing" mode was unreachable. New **`me.audio.setStopOnAudioError(value)`** is the supported way to change it. +- **`me.audio.seek()` / `rate()` setter forms returned Howler's `Howl` object while typed as `number`** — the same get/set contract lie fixed for `stereo`/`position`/`orientation`/`panner` in 19.5.0; both now return nothing when setting and a number when reading. +- **`me.audio.position()` / `stereo()` getters returned `null` for sounds never positioned/panned** — Howler keeps the group state at `null` until first set, and the getters passed that through while typed as a tuple / number. They now return the neutral defaults (`[0, 0, 0]` / `0`). +- **`me.audio.resume()` without an id could spawn a new instance instead of resuming** — Howler's bare `play()` only auto-resumes when *exactly one* instance is paused; with two or more (e.g. after `pause()` without an id, which pauses the whole group) it started a brand-new playback from 0 and left the paused instances stuck forever. `resume()` now explicitly resumes every paused instance, as its documentation always promised. +- **`me.audio.unload()` left `getCurrentTrack()` pointing at the unloaded track** — the current-track pointer is now cleared when the unloaded clip is the active track, so `pauseTrack()`/`resumeTrack()` don't silently act on a ghost. ## [19.8.0] (melonJS 2) - _2026-06-26_ diff --git a/packages/melonjs/src/audio/audio.ts b/packages/melonjs/src/audio/audio.ts index 1194f976c9..ca8f76443f 100644 --- a/packages/melonjs/src/audio/audio.ts +++ b/packages/melonjs/src/audio/audio.ts @@ -31,6 +31,7 @@ import { play } from "./playback.ts"; export { getAudioContext, getMasterGain, + setStopOnAudioError, stopOnAudioError, } from "./backend.ts"; export { @@ -276,6 +277,12 @@ export function unload(sound_name: string): boolean { return false; } + // forget the current-track pointer if it referenced this sound, so + // getCurrentTrack() / pauseTrack() / resumeTrack() don't act on a ghost + if (state.currentTrackId === sound_name) { + state.currentTrackId = null; + } + // destroy the Howl object sound.unload(); delete state.tracks[sound_name]; diff --git a/packages/melonjs/src/audio/backend.ts b/packages/melonjs/src/audio/backend.ts index 7912a01ffc..91ca1997bf 100644 --- a/packages/melonjs/src/audio/backend.ts +++ b/packages/melonjs/src/audio/backend.ts @@ -17,11 +17,30 @@ import { Howl, Howler } from "howler"; * * When `true`, melonJS throws an exception and aborts loading. * When `false`, melonJS disables sound and logs a warning to the console. + * + * Read-only through the `me.audio` namespace (module namespace properties + * can't be assigned) — change it with {@link setStopOnAudioError}. * @default true + * @see setStopOnAudioError */ -// eslint-disable-next-line prefer-const export let stopOnAudioError: boolean = true; +/** + * Set the {@link stopOnAudioError} flag — whether an audio clip that still + * fails after its retries throws (aborting loading) or just disables sound + * with a console warning. This setter is the supported way to change the + * flag: assigning `me.audio.stopOnAudioError = false` directly throws a + * TypeError, because module namespace properties are read-only. + * @param value - `true` to throw on a failed load, `false` to disable sound instead + * @example + * // don't abort the whole game when audio fails to load + * me.audio.setStopOnAudioError(false); + * @category Audio + */ +export function setStopOnAudioError(value: boolean): void { + stopOnAudioError = value; +} + /** * Cross-module mutable state. A single object so multiple consumers * can read and mutate the same fields without the "ESM `let` exports @@ -33,14 +52,16 @@ export let stopOnAudioError: boolean = true; * even though the type signature wouldn't normally admit it. * - `currentTrackId` — the name of the currently-playing track managed * by the `playTrack` / `stopTrack` helpers. - * - `retryCounter` — retry counter for `soundLoadError`'s back-off. + * - `retryCounters` — per-sound retry counters for `soundLoadError`'s + * back-off, keyed by sound name (a single shared counter let parallel + * loads steal each other's retry budget). * - `audioExts` — the active list of audio formats set by `init`. * @ignore */ export const state = { tracks: {} as Record, currentTrackId: null as string | null, - retryCounter: 0, + retryCounters: {} as Record, audioExts: [] as string[], }; @@ -70,7 +91,12 @@ export const soundLoadError = function ( onerror_cb?: () => void, stopOnError: boolean = true, ): void { - if (state.retryCounter++ >= 3) { + // per-sound retry budget — a single shared counter let parallel loads + // steal each other's retries: three failures of one flaky file pushed + // ANOTHER sound's first failure straight over the give-up threshold + const retries = state.retryCounters[sound_name] ?? 0; + if (retries >= 3) { + delete state.retryCounters[sound_name]; const errmsg = `melonJS: failed loading ${sound_name}`; if (!stopOnError) { // disable audio @@ -82,6 +108,7 @@ export const soundLoadError = function ( throw new Error(errmsg); } } else { + state.retryCounters[sound_name] = retries + 1; state.tracks[sound_name]?.load(); } }; diff --git a/packages/melonjs/src/audio/playback.ts b/packages/melonjs/src/audio/playback.ts index edb94288e9..725395e1a4 100644 --- a/packages/melonjs/src/audio/playback.ts +++ b/packages/melonjs/src/audio/playback.ts @@ -43,6 +43,13 @@ export function load( onerrorcb?: () => void, settings: LoadSettings = {}, ): number { + // already loaded? Return 0 ("cached", like every other asset parser) — + // re-preloading a manifest (e.g. re-entering a stage that preloads) used + // to silently replace the Howl and leak the old instance's decoded + // buffers / HTML5 nodes. Unload first to genuinely reload a clip. + if (typeof state.tracks[sound.name] !== "undefined") { + return 0; + } const urls: string[] = []; if (state.audioExts.length === 0) { throw new Error( @@ -71,7 +78,7 @@ export function load( soundLoadError.call(this, sound.name, onerrorcb, stopOnAudioError); }, onload() { - state.retryCounter = 0; + delete state.retryCounters[sound.name]; if (typeof onloadcb === "function") { onloadcb(); } @@ -143,12 +150,19 @@ export function fade( getSoundOrThrow(sound_name).fade(from, to, duration, id); } +/** @inheritDoc */ +export function seek(sound_name: string): number; +/** @inheritDoc */ +export function seek(sound_name: string, seek: number, id?: number): void; /** * Get or set the playback position of a sound. * @param sound_name - Audio clip name (case-sensitive). - * @param args - Optional seek position in seconds, optionally followed - * by the sound instance ID. - * @returns The current seek position when no extra arguments are given. + * @param pos - Seek position in seconds. Omit to read. + * @param id - Sound instance ID. When omitted, all sounds in the group + * are affected. + * @returns The current seek position when called as a getter; nothing + * when called as a setter (the Howl object Howler returns from the + * setter form is an internal, not part of this API). * @example * // read the current position of the background music * let current_pos = me.audio.seek("dst-gameforest"); @@ -156,16 +170,38 @@ export function fade( * me.audio.seek("dst-gameforest", 0); * @category Audio */ -export function seek(sound_name: string, ...args: number[]): number { - return getSoundOrThrow(sound_name).seek(...args); +export function seek( + sound_name: string, + pos?: number, + id?: number, +): number | void { + const sound = getSoundOrThrow(sound_name); + if (pos === undefined) { + return sound.seek(); + } + // forward the exact arity — Howler's core methods dispatch on argument + // count, so an explicit trailing undefined would be parsed as an id + if (id === undefined) { + sound.seek(pos); + } else { + sound.seek(pos, id); + } } +/** @inheritDoc */ +export function rate(sound_name: string): number; +/** @inheritDoc */ +export function rate(sound_name: string, rate: number, id?: number): void; /** * Get or set the playback rate of a sound. * @param sound_name - Audio clip name (case-sensitive). - * @param args - Optional playback rate (`0.5..4.0`, where `1.0` is - * normal speed), optionally followed by the sound instance ID. - * @returns The current playback rate when no extra arguments are given. + * @param rate - Playback rate (`0.5..4.0`, where `1.0` is normal + * speed). Omit to read. + * @param id - Sound instance ID. When omitted, all sounds in the group + * are affected. + * @returns The current playback rate when called as a getter; nothing + * when called as a setter (the Howl object Howler returns from the + * setter form is an internal, not part of this API). * @example * // read the current playback rate * let rate = me.audio.rate("dst-gameforest"); @@ -173,8 +209,22 @@ export function seek(sound_name: string, ...args: number[]): number { * me.audio.rate("dst-gameforest", 2.0); * @category Audio */ -export function rate(sound_name: string, ...args: number[]): number { - return getSoundOrThrow(sound_name).rate(...args); +export function rate( + sound_name: string, + rate?: number, + id?: number, +): number | void { + const sound = getSoundOrThrow(sound_name); + if (rate === undefined) { + return sound.rate(); + } + // forward the exact arity — Howler's core methods dispatch on argument + // count, so an explicit trailing undefined would be parsed as an id + if (id === undefined) { + sound.rate(rate); + } else { + sound.rate(rate, id); + } } /** @inheritDoc */ @@ -205,7 +255,9 @@ export function stereo( ): number | void { const sound = getSoundOrThrow(sound_name); if (pan === undefined) { - return sound.stereo(); + // Howler keeps the group pan at null until it's first set — the + // documented return type is a number, so map that to centered + return sound.stereo() ?? 0; } sound.stereo(pan, id); } @@ -245,7 +297,9 @@ export function position( ): [number, number, number] | void { const sound = getSoundOrThrow(sound_name); if (x === undefined) { - return sound.pos(); + // Howler keeps the group position at null until it's first set — the + // documented return type is a tuple, so map that to the origin + return sound.pos() ?? [0, 0, 0]; } sound.pos(x, y, z, id); } @@ -383,5 +437,31 @@ export function pause(sound_name: string, id?: number): void { * @category Audio */ export function resume(sound_name: string, id?: number): void { - getSoundOrThrow(sound_name).play(id); + const sound = getSoundOrThrow(sound_name); + if (typeof id !== "undefined") { + sound.play(id); + return; + } + // "all sounds in the group are resumed" (see JSDoc above): Howler's bare + // play() only auto-resumes when EXACTLY ONE instance is paused — with two + // or more (e.g. pause() without id pauses the whole group) it spawns a + // brand-new instance from 0 and leaves the paused ones stuck. Howler has + // no public instance list, so read _sounds directly (same justified + // private access as the Howler `_muted` read in audio.ts). + const sounds = ( + sound as unknown as { + _sounds: { _paused: boolean; _ended: boolean; _id: number }[]; + } + )._sounds; + const paused = sounds.filter((s) => { + return s._paused && !s._ended; + }); + if (paused.length === 0) { + // nothing to resume — keep the legacy start-playback behavior + sound.play(); + return; + } + for (const s of paused) { + sound.play(s._id); + } } diff --git a/packages/melonjs/tests/audio-audit.spec.js b/packages/melonjs/tests/audio-audit.spec.js new file mode 100644 index 0000000000..f81f8a986a --- /dev/null +++ b/packages/melonjs/tests/audio-audit.spec.js @@ -0,0 +1,200 @@ +import { describe, expect, it } from "vitest"; +import { soundLoadError, state } from "../src/audio/backend.ts"; +import { audio } from "../src/index.js"; + +// Build a valid silent WAV in-memory and serve it as a data URL (same +// helper as audio.spec.js — kept local, the spec files don't share code). +const makeSilentWavDataUrl = (durationSec = 0.01) => { + const sampleRate = 8000; + const numSamples = Math.max(1, Math.floor(sampleRate * durationSec)); + const dataSize = numSamples * 2; // 16-bit mono + const buf = new ArrayBuffer(44 + dataSize); + const view = new DataView(buf); + let p = 0; + const writeStr = (s) => { + for (let i = 0; i < s.length; i++) { + view.setUint8(p++, s.charCodeAt(i)); + } + }; + const writeU32 = (v) => { + view.setUint32(p, v, true); + p += 4; + }; + const writeU16 = (v) => { + view.setUint16(p, v, true); + p += 2; + }; + writeStr("RIFF"); + writeU32(36 + dataSize); + writeStr("WAVE"); + writeStr("fmt "); + writeU32(16); // PCM chunk size + writeU16(1); // format = PCM + writeU16(1); // mono + writeU32(sampleRate); + writeU32(sampleRate * 2); // byte rate + writeU16(2); // block align + writeU16(16); // bits per sample + writeStr("data"); + writeU32(dataSize); + // samples already zero-initialised → silence + const bytes = new Uint8Array(buf); + let bin = ""; + for (let i = 0; i < bytes.length; i++) { + bin += String.fromCharCode(bytes[i]); + } + return `data:audio/wav;base64,${btoa(bin)}`; +}; + +const loadClip = (name) => { + audio.init("wav"); + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error(`timeout loading ${name}`)); + }, 2000); + audio.load( + { name, src: makeSilentWavDataUrl() }, + () => { + clearTimeout(timeout); + resolve(); + }, + () => { + clearTimeout(timeout); + reject(new Error(`load failed for ${name}`)); + }, + ); + }); +}; + +/** + * Audit fixes for the audio module, written failing-first — each test below + * documents a confirmed bug from the 2026-07 adversarial code audit. + */ +describe("audio audit fixes", () => { + it("load() returns 0 and keeps the same Howl when the clip is already loaded", async () => { + // pre-fix: the audio parser was the only one without an already-loaded + // guard — re-preloading a manifest silently replaced each Howl and + // leaked the old instance (decoded buffers / HTML5 nodes) + await loadClip("audit-cached"); + const before = state.tracks["audit-cached"]; + const count = audio.load({ + name: "audit-cached", + src: makeSilentWavDataUrl(), + }); + expect(count).toBe(0); + expect(state.tracks["audit-cached"]).toBe(before); + audio.unload("audit-cached"); + }); + + it("a flaky sound's retries don't consume another sound's retry budget", () => { + // pre-fix: a single global retryCounter was shared by every sound — + // three failures of sound A pushed sound B's FIRST failure straight + // over the give-up threshold (a spurious fatal error under the + // default stopOnAudioError) + let aGaveUp = false; + let bGaveUp = false; + for (let i = 0; i < 3; i++) { + soundLoadError( + "audit-flaky-a", + () => { + aGaveUp = true; + }, + false, + ); + } + expect(aGaveUp).toBe(false); // still within its own 3-retry budget + soundLoadError( + "audit-flaky-b", + () => { + bGaveUp = true; + }, + false, + ); + expect(bGaveUp).toBe(false); // b's FIRST failure must only start ITS budget + // a's 4th failure exhausts a's own budget + soundLoadError( + "audit-flaky-a", + () => { + aGaveUp = true; + }, + false, + ); + expect(aGaveUp).toBe(true); + // the give-up path (stopOnError=false) mutes audio globally — restore + audio.unmuteAll(); + }); + + it("setStopOnAudioError() is the supported way to change the flag", () => { + // pre-fix: the flag was DOCUMENTED as settable, but it's a module + // export — assigning through the namespace throws, so the + // "disable audio instead of throwing" mode was unreachable + expect(() => { + audio.stopOnAudioError = false; + }).toThrow(TypeError); + expect(typeof audio.setStopOnAudioError).toBe("function"); + audio.setStopOnAudioError(false); + expect(audio.stopOnAudioError).toBe(false); + audio.setStopOnAudioError(true); + expect(audio.stopOnAudioError).toBe(true); + }); + + it("seek()/rate() setter form returns undefined; getter form returns a number", async () => { + // pre-fix: the setter form returned Howler's Howl object while the + // signature declared `number` — the same get/set contract lie class + // fixed for stereo/position/orientation/panner in #1456 + await loadClip("audit-seekrate"); + expect(audio.seek("audit-seekrate", 0)).toBeUndefined(); + expect(typeof audio.seek("audit-seekrate")).toBe("number"); + expect(audio.rate("audit-seekrate", 1.5)).toBeUndefined(); + expect(typeof audio.rate("audit-seekrate")).toBe("number"); + audio.unload("audit-seekrate"); + }); + + it("position()/stereo() getters return neutral defaults when never set", async () => { + // pre-fix: both getters passed Howler's internal `null` through, + // while typed to return a tuple / number + await loadClip("audit-spatial"); + expect(audio.stereo("audit-spatial")).toBe(0); + expect(audio.position("audit-spatial")).toEqual([0, 0, 0]); + audio.unload("audit-spatial"); + }); + + it("resume() without id targets the paused instances instead of spawning a new one", async () => { + // pre-fix: resume() forwarded to Howler's bare play(), which only + // auto-resumes when EXACTLY ONE instance is paused — with two or + // more (e.g. pause() without id pauses the whole group) it spawned + // a brand-new instance from 0 and left the paused ones stuck forever + await loadClip("audit-resume"); + const id1 = audio.play("audit-resume"); + const id2 = audio.play("audit-resume"); + audio.pause("audit-resume"); // no id → pauses the whole group + const howl = state.tracks["audit-resume"]; + expect(howl._sounds.length).toBe(2); + + audio.resume("audit-resume"); + + // no third instance spawned; the resume targeted the existing two + // (instance count is deterministic even when the headless audio + // context is locked — actual playback state isn't) + expect(howl._sounds.length).toBe(2); + expect( + howl._sounds + .map((s) => { + return s._id; + }) + .sort(), + ).toEqual([id1, id2].sort()); + audio.stop("audit-resume"); + audio.unload("audit-resume"); + }); + + it("unload() clears the current-track pointer when it referenced the unloaded clip", async () => { + // pre-fix: getCurrentTrack() kept reporting a track that no longer + // existed, and pauseTrack()/resumeTrack() silently no-op'd on it + await loadClip("audit-track"); + audio.playTrack("audit-track"); + expect(audio.getCurrentTrack()).toBe("audit-track"); + audio.unload("audit-track"); + expect(audio.getCurrentTrack()).toBeNull(); + }); +});