diff --git a/src/audio/AudioModule.js b/src/audio/AudioModule.js index ad51fd8138..838d9f0f02 100644 --- a/src/audio/AudioModule.js +++ b/src/audio/AudioModule.js @@ -15,6 +15,8 @@ import { ChatbotService } from "../chatbots/ChatbotService.ts"; import OffscreenAudioBridge from "./OffscreenAudioBridge.js"; import { BrowserCompatibilityModule } from "../compat/BrowserCompatibilityModule.ts"; import { createAudioRemovalObserverCallback } from "./audioElementRemoval.ts"; +import { findHostAudioElement, shouldMuteHostAudio } from "./hostAudio.ts"; +import { audioProviders } from "../tts/SpeechModel.ts"; const INITIAL_PLAYBACK_BUFFER_TIMEOUT_MS = 5000; @@ -26,6 +28,8 @@ export default class AudioModule { this.AUDIO_ELEMENT_ID = "saypi-audio-main"; this.audioElement = null; + /** Are WE the voice? Set from audio:changeProvider; decides the host's mute. */ + this.providerIsSayPi = false; this.mutationObserver = null; this.swapObserver = null; @@ -250,6 +254,7 @@ export default class AudioModule { } this.decorateAudioElement(this.audioElement); this.registerRemovalListener(); + this.applyHostAudioMute(); } swapAudioElement(newAudioElement) { @@ -263,7 +268,7 @@ export default class AudioModule { this.registerAudioPlaybackEvents(this.audioElement, this.audioOutputActor); } this.registerAudioPlaybackEvents(this.audioElement, this.voiceConverter); - + // handle slow responses from pi.ai - since 2024-07 (Pi.ai only) if (ChatbotIdentifier.isChatbotType("pi")) { this.initializeSlowResponseHandler(); @@ -274,6 +279,29 @@ export default class AudioModule { } this.registerRemovalListener(); this.registerLifecycleDebug(); + // A fresh element arrives unmuted, and the host may already be playing + // through it, so re-assert the invariant rather than waiting for a + // loadstart we might have just missed (#602). + this.applyHostAudioMute(); + } + + /** + * Hold the host's audio silent while SayPi is the voice — an invariant, not a + * reaction to a `loadstart` we have to be attached in time to see (#602). + * Re-applied whenever the element changes or the provider does. + */ + applyHostAudioMute() { + if (!this.audioElement) return; + const mute = shouldMuteHostAudio({ + providerIsSayPi: this.providerIsSayPi, + playbackIsOffscreen: this.useOffscreenAudio, + }); + if (this.audioElement.muted !== mute) { + logger.debug( + `[AudioModule] ${mute ? "Muting" : "Unmuting"} the host's audio element` + ); + this.audioElement.muted = mute; + } } registerRemovalListener() { @@ -290,7 +318,7 @@ export default class AudioModule { logger.debug("Audio element removed from the document"); this.cleanupAudioElement(this.audioElement); this.audioElement = null; - this.listenForAudioElementSwap(); + this.rebindAudioElement(); } ) ); @@ -348,6 +376,27 @@ export default class AudioModule { logger.debug("Cleaned up audio element"); } + /** + * Take up whatever audio element the host has NOW, and only wait for one if + * there genuinely isn't one. + * + * The old recovery went straight to `listenForAudioElementSwap()`, whose + * observer fires for newly ADDED subtrees. A host that replaces its player + * rather than reusing it (pi.ai) has already inserted the replacement by the + * time we notice ours is gone, so nothing was ever added afterwards to react + * to: the observer waited forever, no `loadstart` reached the output machine, + * and the host's own voice played on top of ours (#602). + */ + rebindAudioElement() { + const replacement = findHostAudioElement(document, this.AUDIO_ELEMENT_ID); + if (replacement) { + logger.debug("[AudioModule] Rebinding to the host's audio element"); + this.swapAudioElement(replacement); + return; + } + this.listenForAudioElementSwap(); + } + listenForAudioElementSwap() { if (this.swapObserver) { this.swapObserver.disconnect(); @@ -510,6 +559,12 @@ export default class AudioModule { if (outputActor) { outputActor.send({ type: "changeProvider", ...detail }); } + // Whether the host may be heard is decided by who is speaking, so it is + // settled here rather than at each playback event (#602). Matched by + // identity against the SayPi provider — `matches(source)` answers a + // different question (does this URL belong to it). + this.providerIsSayPi = detail?.provider === audioProviders.SayPi; + this.applyHostAudioMute(); }); EventBus.on("audio:changeVoice", (detail) => { if (outputActor) { diff --git a/src/audio/hostAudio.ts b/src/audio/hostAudio.ts new file mode 100644 index 0000000000..0b393d3f23 --- /dev/null +++ b/src/audio/hostAudio.ts @@ -0,0 +1,55 @@ +/** + * Two decisions about the HOST's audio element, kept out of AudioModule.js so + * they can be tested (that module's constructor drags in the whole content-script + * bootstrap — the same reason `audioElementRemoval.ts` exists). + * + * Both exist to serve one rule: when a SayPi voice is selected for the active + * host, SayPi is the text-to-speech provider — the host's own audio is silenced + * and ours plays instead (#602). + */ + +/** + * The element to bind to after losing the tracked one. + * + * The old recovery was to re-arm an observer that only fires for NEWLY ADDED + * subtrees. On a host that replaces its player rather than reusing it (pi.ai + * does), the replacement is already in the document by the time we look, so + * that observer waits forever and the binding is lost for the life of the page + * — with it, every `loadstart` the output machine needs in order to skip the + * host's voice. + * + * Prefers an element already carrying our id (we decorated it before), then any + * other. Returns null when there is genuinely nothing yet, which is the only + * case where waiting for an insertion is the right answer. + */ +export function findHostAudioElement( + root: Document | Element | null | undefined, + decoratedId: string +): HTMLAudioElement | null { + if (!root) return null; + // Matched by property rather than by an interpolated `#id` selector: the id + // never needs escaping, and this can't throw on a hostile one. + const elements = [...root.querySelectorAll("audio")]; + return elements.find((el) => el.id === decoratedId) ?? elements[0] ?? null; +} + +/** + * Whether the host's element should be held muted right now. + * + * An invariant rather than a reaction: `skipCurrent` only fires when we catch a + * `loadstart`, so a track the host started before we bound to it stays audible + * for its whole length. Muting is also gentler than pausing — the host's player + * keeps its own state, and its UI doesn't fight us over it. + * + * `playbackIsOffscreen` is the condition that makes this safe. Under the + * offscreen document (Chrome/Edge) our speech never touches the page's element, + * so muting it silences only the host. Where offscreen isn't available (Firefox, + * Safari) SayPi plays through that same element, and muting it would mute US — + * there, skip-on-loadstart remains the whole mechanism. + */ +export function shouldMuteHostAudio(state: { + providerIsSayPi: boolean; + playbackIsOffscreen: boolean; +}): boolean { + return state.providerIsSayPi && state.playbackIsOffscreen; +} diff --git a/test/audio/audioElementRemoval.spec.ts b/test/audio/audioElementRemoval.spec.ts index a9f6bdafaa..a334a20a8d 100644 --- a/test/audio/audioElementRemoval.spec.ts +++ b/test/audio/audioElementRemoval.spec.ts @@ -186,13 +186,45 @@ describe("AudioModule removal-observer wiring", () => { ); }); - it("releases the audio element and arms the swap listener on removal", () => { + it("releases the audio element and recovers the binding on removal", () => { const handler = source.match( /createAudioRemovalObserverCallback\([\s\S]*?\n {6}\);/ )?.[0]; expect(handler).toBeDefined(); expect(handler).toMatch(/this\.cleanupAudioElement\(this\.audioElement\)/); expect(handler).toMatch(/this\.audioElement = null/); - expect(handler).toMatch(/this\.listenForAudioElementSwap\(\)/); + // Recovery goes through rebindAudioElement, which takes up a replacement + // that is ALREADY in the document and only falls back to waiting for an + // insertion when there is none. Going straight to the swap listener was + // #602: on a host that replaces its player, nothing is ever added + // afterwards, so the binding was lost for the life of the page. + expect(handler).toMatch(/this\.rebindAudioElement\(\)/); + }); + + it("rebinds to an existing element before falling back to waiting", () => { + const rebind = source.match(/rebindAudioElement\(\)\s*\{[\s\S]*?\n {2}\}/)?.[0]; + expect(rebind).toBeDefined(); + expect(rebind).toMatch(/findHostAudioElement\(/); + expect(rebind).toMatch(/this\.swapAudioElement\(/); + expect(rebind).toMatch(/this\.listenForAudioElementSwap\(\)/); + // The fallback must come after the take-it-now path, not instead of it. + expect(rebind!.indexOf("swapAudioElement")).toBeLessThan( + rebind!.indexOf("listenForAudioElementSwap") + ); + }); + + it("holds the host's audio muted from the provider, not from playback events", () => { + // The mute is an invariant: re-asserted whenever the element changes and + // whenever the provider does, so a track the host started before we bound + // to it can't stay audible for its whole length (#602). + expect(source).toMatch(/applyHostAudioMute\(\)\s*\{/); + expect(source).toMatch(/shouldMuteHostAudio\(\{/); + const onProviderChange = source.match( + /EventBus\.on\("audio:changeProvider"[\s\S]*?\n {4}\}\);/ + )?.[0]; + expect(onProviderChange).toMatch(/providerIsSayPi = /); + expect(onProviderChange).toMatch(/this\.applyHostAudioMute\(\)/); + const swap = source.match(/swapAudioElement\(newAudioElement\)\s*\{[\s\S]*?\n {2}\}/)?.[0]; + expect(swap).toMatch(/this\.applyHostAudioMute\(\)/); }); }); diff --git a/test/audio/hostAudio.spec.ts b/test/audio/hostAudio.spec.ts new file mode 100644 index 0000000000..f6d4f4e862 --- /dev/null +++ b/test/audio/hostAudio.spec.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { + findHostAudioElement, + shouldMuteHostAudio, +} from "../../src/audio/hostAudio"; + +const ID = "saypi-audio-main"; + +describe("finding the host's audio element again after losing it", () => { + beforeEach(() => { + document.body.innerHTML = ""; + }); + + it("finds a replacement that is ALREADY in the document", () => { + // The #602 case: pi.ai replaces its player rather than reusing it, so by + // the time we notice ours is gone the new one has already been inserted — + // and an observer waiting for an insertion waits forever. + const replacement = document.createElement("audio"); + document.body.appendChild(replacement); + expect(findHostAudioElement(document, ID)).toBe(replacement); + }); + + it("prefers the element we had already decorated", () => { + const stranger = document.createElement("audio"); + const ours = document.createElement("audio"); + ours.id = ID; + document.body.append(stranger, ours); + expect(findHostAudioElement(document, ID)).toBe(ours); + }); + + it("reports nothing when the host has no player yet — the one case worth waiting for", () => { + expect(findHostAudioElement(document, ID)).toBeNull(); + }); + + it("never throws on a missing root", () => { + expect(findHostAudioElement(null, ID)).toBeNull(); + expect(findHostAudioElement(undefined, ID)).toBeNull(); + }); +}); + +describe("holding the host's audio muted while SayPi is the provider", () => { + it("mutes the host when we are speaking from the offscreen document", () => { + expect( + shouldMuteHostAudio({ providerIsSayPi: true, playbackIsOffscreen: true }) + ).toBe(true); + }); + + it("leaves the host audible when its own voice is the one selected", () => { + expect( + shouldMuteHostAudio({ providerIsSayPi: false, playbackIsOffscreen: true }) + ).toBe(false); + }); + + it("never mutes the element WE play through (no offscreen document)", () => { + // Firefox/Safari: SayPi shares the page's audio element with the host, so + // muting it would mute us. Skip-on-loadstart stays the whole mechanism. + expect( + shouldMuteHostAudio({ providerIsSayPi: true, playbackIsOffscreen: false }) + ).toBe(false); + }); +});