diff --git a/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.test.ts b/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.test.ts new file mode 100644 index 00000000..9f3b5693 --- /dev/null +++ b/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, test, beforeEach, afterEach } from 'bun:test'; +import { GaplessEngine, type EngineTrack } from './gapless-engine'; + +// These tests exist because the failure they cover is inaudible to the code and obvious to the ear: two +// sources playing the same track at once. Nothing throws, no state looks wrong, `cur` points at a real +// node that is really playing — there is simply a second one nobody is holding. The only way to catch it +// is to count the nodes that were started. + +type StartCall = { when: number; offset: number }; + +class FakeSource { + buffer: AudioBuffer | null = null; + onended: (() => void) | null = null; + started: StartCall | null = null; + stopped = false; + disconnected = false; + constructor(private ctx: FakeContext) {} + connect(): void {} + disconnect(): void { + this.disconnected = true; + } + start(when = 0, offset = 0): void { + if (this.started) throw new Error('InvalidStateError: already started'); + this.started = { when, offset }; + this.ctx.started.push(this); + } + stop(): void { + if (!this.started) throw new Error('InvalidStateError: not started'); + this.stopped = true; + } + /** What the browser does at the end of the buffer (or at stop()) — the engine's advance trigger. */ + end(): void { + this.onended?.(); + } +} + +class FakeContext { + currentTime = 0; + state: 'running' | 'suspended' | 'closed' = 'running'; + destination = {}; + started: FakeSource[] = []; + createGain() { + return { gain: { value: 1 }, connect: () => {} }; + } + createBufferSource() { + return new FakeSource(this) as unknown as AudioBufferSourceNode & FakeSource; + } + async resume(): Promise { + this.state = 'running'; + } + async suspend(): Promise { + this.state = 'suspended'; + } + async close(): Promise { + this.state = 'closed'; + } + async decodeAudioData(): Promise { + // Decoding is the slow step the bug hides inside — a macrotask is enough to model "not instant". + await new Promise((r) => setTimeout(r, 0)); + return { duration: 100 } as AudioBuffer; + } +} + +/** + * Sources producing sound RIGHT NOW. The `when <= currentTime` clause is not a detail: a gapless engine + * always has the next track already started, scheduled at the current track's end. Counting those as + * audible would make the healthy state look like the bug. + */ +const audible = (ctx: FakeContext) => + ctx.started.filter((s) => !s.stopped && s.started !== null && s.started.when <= ctx.currentTime); + +let ctx: FakeContext; +const originalFetch = globalThis.fetch; +const originalCtor = (globalThis as Record).AudioContext; + +const QUEUE: EngineTrack[] = [ + { key: 'a', url: '/stream/a' }, + { key: 'b', url: '/stream/b' }, + { key: 'c', url: '/stream/c' }, +]; + +// Let every pending decode + its continuation run. +const settle = async () => { + for (let i = 0; i < 8; i++) await new Promise((r) => setTimeout(r, 0)); +}; + +beforeEach(() => { + ctx = new FakeContext(); + (globalThis as Record).AudioContext = function () { + return ctx; + }; + globalThis.fetch = (async () => ({ + ok: true, + arrayBuffer: async () => new ArrayBuffer(8), + })) as unknown as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + (globalThis as Record).AudioContext = originalCtor; +}); + +describe('GaplessEngine', () => { + test('load(autoplay) followed by play() starts the track exactly once', async () => { + // The host's queue effect and playing effect both fire in the same commit when a queue starts from a + // paused player. This is that commit, and it used to produce two sources of the same track. + const engine = new GaplessEngine({}); + engine.load(QUEUE, 0, true); + engine.play(); + await settle(); + + expect(audible(ctx)).toHaveLength(1); + engine.destroy(); + }); + + test('repeated play() during the decode does not stack sources', async () => { + const engine = new GaplessEngine({}); + engine.load(QUEUE, 0, true); + engine.play(); + engine.play(); + engine.play(); + await settle(); + + expect(audible(ctx)).toHaveLength(1); + engine.destroy(); + }); + + test('pause then play while still decoding does not stack sources', async () => { + // The impatient-user path: nothing is audible yet because the file is still downloading, so the play + // button gets hit again. + const engine = new GaplessEngine({}); + engine.load(QUEUE, 0, true); + engine.pause(); + engine.play(); + await settle(); + + expect(audible(ctx)).toHaveLength(1); + engine.destroy(); + }); + + test('a stale source ending does not advance the queue', async () => { + // Defence in depth: even if a source outlives its bookkeeping, only `cur` may move the index. + const seen: number[] = []; + const engine = new GaplessEngine({ onIndex: (i) => seen.push(i) }); + engine.load(QUEUE, 0, true); + await settle(); + + const first = audible(ctx)[0]!; + engine.skipTo(2); // bumps the generation and stops `first` + await settle(); + first.end(); // the browser still delivers its onended + + expect(seen).toEqual([]); // skipTo is a user action; only a NATURAL boundary reports an index + engine.destroy(); + }); + + test('switching queues leaves nothing from the old one sounding', async () => { + const engine = new GaplessEngine({}); + engine.load(QUEUE, 0, true); + await settle(); + engine.load([{ key: 'z', url: '/stream/z' }], 0, true); + await settle(); + + expect(audible(ctx)).toHaveLength(1); + expect(audible(ctx)[0]!.started).not.toBeNull(); + engine.destroy(); + }); + + test('a natural boundary advances the index exactly once', async () => { + const seen: number[] = []; + const engine = new GaplessEngine({ onIndex: (i) => seen.push(i) }); + engine.load(QUEUE, 0, true); + await settle(); + + const cur = audible(ctx)[0]!; + cur.end(); + await settle(); + + expect(seen).toEqual([1]); + engine.destroy(); + }); + + test('destroy() silences every source it started', async () => { + const engine = new GaplessEngine({}); + engine.load(QUEUE, 0, true); + await settle(); + engine.destroy(); + + expect(audible(ctx)).toHaveLength(0); + }); +}); diff --git a/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.ts b/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.ts index 0112387d..68c1386b 100644 --- a/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.ts +++ b/src/workspaces/officerdev/src/MusicPlayer/gapless-engine.ts @@ -38,6 +38,11 @@ export class GaplessEngine { private cur: AudioBufferSourceNode | null = null; private nxt: AudioBufferSourceNode | null = null; + // EVERY source node this engine has started and that has not yet ended. `cur`/`nxt` are the two it is + // reasoning about; this is the set it is responsible for silencing. They diverged once — a duplicate + // begin() left a source playing that nothing held a reference to, so nothing could ever stop it — and + // an orphan in Web Audio is unstoppable and inaudible to the code. Add here, stop from here. + private live = new Set(); private curBaseTime = 0; // ctx time that corresponds to position 0 of the current track private curDuration = 0; private nextStartAt = 0; // ctx time the scheduled `nxt` will begin (= current track's end) @@ -45,6 +50,9 @@ export class GaplessEngine { private started = false; // has the current track's source actually been started? private playing = false; private gen = 0; // bumped on any disruptive change; stale async/onended callbacks check it and bail + // The generation a begin() is currently in flight for, or -1. `gen` alone cannot express this: it marks + // a change, and two begin() calls for the SAME generation are exactly the case that must be refused. + private beginGen = -1; private raf = 0; private cb: EngineCallbacks; @@ -201,19 +209,41 @@ export class GaplessEngine { void this.preloadNext(gen); } - /** Start the current track at `pendingOffset` (decoding first if needed). */ + /** + * Start the current track at `pendingOffset` (decoding first if needed). + * + * Re-entrant calls for the same generation are REFUSED, and that guard is the whole reason this method + * is not simply idempotent-by-gen. `started` only becomes true at the very end, after a fetch and a + * full decode — seconds, for a long track. Anything that calls begin() in that window sees + * `started === false` and starts a second, parallel decode of the same track, and both finish and both + * call startBuffer(): two sources of the same audio playing at once, only one of them in `cur`. + * + * That was not a rare race. Starting a queue from a paused player fired it every single time: the host + * commits a new queue and `playing: true` together, its queue effect calls load(autoplay) → begin, and + * its playing effect then calls play() → begin again, same generation. The audible result compounds — + * the untracked twin keeps its own onended, so at the track boundary advance() runs twice, the index + * jumps two tracks and a second source is promoted while the first is still sounding. + */ private async begin(gen: number): Promise { - const ctx = this.ensureCtx(); - if (ctx.state === 'suspended') await ctx.resume(); - if (gen !== this.gen) return; - const t = this.queue[this.index]; - if (!t) return; - this.cb.onLoadingChange?.(true); - const buf = await this.decode(t.url); - this.cb.onLoadingChange?.(false); - if (gen !== this.gen || !buf) return; - this.startBuffer(buf, this.pendingOffset, gen); - void this.preloadNext(gen); + if (this.beginGen === gen) return; + this.beginGen = gen; + try { + const ctx = this.ensureCtx(); + if (ctx.state === 'suspended') await ctx.resume(); + if (gen !== this.gen) return; + const t = this.queue[this.index]; + if (!t) return; + this.cb.onLoadingChange?.(true); + const buf = await this.decode(t.url); + this.cb.onLoadingChange?.(false); + if (gen !== this.gen || !buf) return; + this.startBuffer(buf, this.pendingOffset, gen); + void this.preloadNext(gen); + } finally { + // Released even on the bail paths: a decode that fails must not leave this generation permanently + // unable to start, or a failed track would wedge the player until something bumped `gen`. + if (this.beginGen === gen) this.beginGen = -1; + } } private startBuffer(buf: AudioBuffer, offset: number, gen: number): void { @@ -224,9 +254,8 @@ export class GaplessEngine { const startAt = ctx.currentTime; const off = Math.min(Math.max(0, offset), buf.duration); src.start(startAt, off); - src.onended = () => { - if (gen === this.gen) this.advance(); - }; + this.live.add(src); + src.onended = () => this.onSourceEnded(src, gen); this.cur = src; this.curDuration = buf.duration; this.curBaseTime = startAt - off; // position = ctx.currentTime - curBaseTime @@ -237,12 +266,19 @@ export class GaplessEngine { /** Decode index+1 and schedule it to begin exactly when the current track ends. */ private async preloadNext(gen: number): Promise { if (this.nxt) { + const stale = this.nxt; + this.live.delete(stale); try { - this.nxt.onended = null; - this.nxt.stop(); + stale.onended = null; + stale.stop(); } catch { /* not started */ } + try { + stale.disconnect(); + } catch { + /* already disconnected */ + } this.nxt = null; } const forIndex = this.index; @@ -257,13 +293,26 @@ export class GaplessEngine { src.buffer = buf; src.connect(this.gain!); src.start(boundary, 0); - src.onended = () => { - if (gen === this.gen) this.advance(); - }; + this.live.add(src); + src.onended = () => this.onSourceEnded(src, gen); this.nxt = src; this.nextStartAt = boundary; } + /** + * A source finished. Only the one the engine considers CURRENT may drive the queue forward. + * + * Without the identity check, any source that outlives its bookkeeping still advances the queue when it + * ends — so one stray node does not just play unwanted audio, it desynchronises the index for + * everything after it. `cur` is the single source of truth for "what is playing"; ending anything else + * is bookkeeping, not an event. + */ + private onSourceEnded(src: AudioBufferSourceNode, gen: number): void { + this.live.delete(src); + if (gen !== this.gen || this.cur !== src) return; + this.advance(); + } + /** Fired at a track boundary (its source ended): move to the next track. */ private advance(): void { if (this.index + 1 >= this.queue.length) { @@ -293,9 +342,9 @@ export class GaplessEngine { } } + /** Silence everything this engine has started. Iterates `live`, not just cur/nxt — see its declaration. */ private stopSources(): void { - for (const s of [this.cur, this.nxt]) { - if (!s) continue; + for (const s of this.live) { try { s.onended = null; s.stop(); @@ -308,6 +357,7 @@ export class GaplessEngine { /* already disconnected */ } } + this.live.clear(); this.cur = null; this.nxt = null; }