Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 74ed7c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
Serialize audio context teardown behind a mutex so a disconnect that races a mute/unmute cycle cannot leave a context, an analyser or a source node behind. Race `resume()` against a timeout in `detectSilence` so an autoplay-blocked page cannot strand the graph on a pending promise, and guard `startAudio` on `audioContextReleased` so calling it after a disconnect no longer builds an iOS dummy element, a listener and a context that nothing closes. Export `releaseEmptyAudioStreamTrack` so callers can hand back a clone of the shared empty track, and cover the refcount, the teardown order and the disconnected-room guards with unit tests. The volume meter in the demo never released its analyser or cleared its interval, which leaked the same three node types from the page used to measure the fix.
… eslint no-empty-function
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Abort an audio context acquisition that was queued behind a disconnect, so a late `startAudio` cannot clear the released flag and build a context on a room that nothing tears down again. A connect still acquires normally, which is what lets a reconnect recover. Disconnect only the web audio edges the track created, walking the chain recorded when the graph was wired. Reading the plugin array instead left the previous plugins attached once `setWebAudioPlugins` had replaced them, and a bare `disconnect()` on a node the application owns took the routing owned by the application down with it. Reference count the iOS dummy audio element across rooms. It is shared through the DOM, so the first room to disconnect used to remove the element and release the silent track that the other rooms on the page were still playing. Its visibility listener is now per room, because the shared one resumed playback on whichever room happened to create it.
# Conflicts: # src/room/utils.test.ts
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
It's hard for me to review this comprehensively in its current state since all the different fixes are quite mixed together. However, I did a pass through it and saw a few potentially unexpected things.
|
|
||
| Three further behavior changes to be aware of: | ||
|
|
||
| - A track retained across rooms loses its audio processor. `room.disconnect({ stopTracks: false })` detaches the track from the audio context, which stops the processor. Call `setProcessor` again after you republish the track. |
There was a problem hiding this comment.
suggestion: This might be worth calling out in whatever docs surfaces mention TrackProcessor and associated behavior.
| stopLocalVolumeMeter?.(); | ||
| stopLocalVolumeMeter = () => { | ||
| clearInterval(interval); | ||
| cleanup().catch(() => {}); |
There was a problem hiding this comment.
suggestion: Should this log the caught error?
| /** | ||
| * registers the krisp feature listeners on the given processed track, moving them over from | ||
| * a previously observed one. Passing `undefined` just removes the existing listeners. | ||
| */ | ||
| private observeProcessedTrack(processedTrack: MediaStreamTrack | undefined) { | ||
| if (this.observedProcessedTrack === processedTrack) { | ||
| return; | ||
| } | ||
| if (this.observedProcessedTrack) { | ||
| this.observedProcessedTrack.removeEventListener( | ||
| 'enable-lk-krisp-noise-filter', | ||
| this.handleKrispNoiseFilterEnable, | ||
| ); | ||
| this.observedProcessedTrack.removeEventListener( | ||
| 'disable-lk-krisp-noise-filter', | ||
| this.handleKrispNoiseFilterDisable, | ||
| ); |
There was a problem hiding this comment.
question: In practice, is there any situation where this.observedProcessedTrack would ever not equal this.processor.processedTrack? From what I can tell no, since only this.processor.processedTrack or undefined is ever passed in.
Assuming I am not missing something here - would it be less complex for this to call this.processor?.processedTrack.removeEventListener(...) rather than tracking the this.observedProcessedTrack state separately? And then maybe this method could be renamed something like transferNoiseFilterHandlers() (ie, remove processedTrack)?
| // `stop` is synchronous, so we can't await the teardown of the processor here. | ||
| // make sure a failing teardown doesn't surface as an unhandled rejection. | ||
| this.processor?.destroy().catch((error) => { | ||
| this.log.error('failed to destroy processor', { ...this.logContext, error }); | ||
| }); |
There was a problem hiding this comment.
thought: I don't think changing stop to return a promise wouldn't be a breaking change, since track.stop() would still work from a user's perspective, and they currently would have no reason to use the undefined return value.
Given this, would it be worth awaiting this.processor?.destroy() here and bubbling this rejection upwards to the caller?
IMO it still makes sense to add the catch log here, and by keeping this catch as a parallel path that would silence any possible unhandledrejection errors as well.
| // the plugin nodes belong to whoever passed them in and may feed graphs of their own, so only | ||
| // the edges this track created come down. A bare `node.disconnect()` would drop the | ||
| // application's own routing with them | ||
| for (let i = 0; i < this.connectedNodes.length - 1; i += 1) { | ||
| try { | ||
| this.connectedNodes[i].disconnect(this.connectedNodes[i + 1]); | ||
| } catch { | ||
| // a plugin node the application already disconnected itself, the rest of the chain still | ||
| // has to come apart | ||
| } | ||
| } | ||
| this.connectedNodes = []; | ||
| // the source and gain nodes are ours alone, so every remaining edge of theirs is ours to drop |
There was a problem hiding this comment.
question: I might be misreading this, but I think this.connectedNodes[i].disconnect(...); would never run for the final entry in this.connectedNodes, since the loop runs until i < this.connectedNodes.length - 1? That final entry would only get hit by this.connectedNodes[i + 1].
It's hard for me to be 100% sure that this is intended though without deeper context on how the audio node graph is structured.
| /** | ||
| * iOS blocks audio element playback if | ||
| * - user is not publishing audio themselves and | ||
| * - no other audio source is playing | ||
| * | ||
| * as a workaround, we create an audio element with an empty track, so that | ||
| * silent audio is always playing | ||
| */ |
There was a problem hiding this comment.
suggestion: Can you add this comment back in somewhere? IMO this better explains why this is needed in an easier to understand way than the LLM inserted versions do.
This branch has not been deployed
2 participants
Fixes #1969. Linear: CLT-2995.
The leak
Every
AudioContext,AnalyserNode,MediaStreamAudioSourceNodeandMediaStreamAudioDestinationNodethe SDK created during a call stayed alive afterroom.disconnect(). They accumulated over mute/unmute cycles, and a page that joined several rooms kept every graph it had ever built. After this change the heap returns to its pre-call baseline once the room is disconnected and GC runs.The main sources:
detectSilenceclosed its context on the happy path only, so any throw stranded the context, the analyser and the source node.createAudioAnalyserclosed the context without disconnecting its nodes, and it was not idempotent.webAudioMix(an object with noaudioContext) never closed it.visibilitychangelistener were never torn down, so the listener outlived every room on the page.resume()pending forever and strand the whole graph.resume()is now raced against a timeout.startAudio()after a disconnect rebuilt the iOS dummy element, its listener and a context that nothing closed. It is now guarded onaudioContextReleased.Audio context teardown is serialized behind a mutex, so a disconnect racing a mute/unmute cycle cannot leave a half-torn-down graph.
Also fixed: a false
AudioSilenceDetectedSilence detection read a zero-filled buffer from a context that had never reached
running. An autoplay-blocked page therefore reported silence on every track it checked.This is one half of the fix
The krisp side of the same leak shipped in
@livekit/krisp-noise-filter0.4.4. Users who run noise cancellation need both that version and this release. Neither one alone returns the heap to baseline.One note on the original report:
meet.livekit.iodoes enable krisp noise cancellation, so what was measured there was the combined SDK and krisp leak, not an SDK-only one.Why the changeset is
minor, notpatchThis adds a public export,
releaseEmptyAudioStreamTrack, the counterpart togetEmptyAudioStreamTrack. Callers that take a clone of the shared empty track hand it back through this function, and the shared context closes when the count reaches zero.Behavior changes to be aware of
room.disconnect({ stopTracks: false })detaches the track from the audio context, which stops the processor. CallsetProcessoragain after you republish the track.audioContext: undefined. A processor must handle that and tear its nodes down, rather than assume a context is always present.webAudioMix, every attached element is muted, not only the first one, so a second attached element no longer plays the track twice. Those elements are unmuted and their volume restored when the context goes away.Participant.setAudioContextandLocalAudioTrack.setAudioContextnow return a promise. Both are marked@internal, but both appear in the published type declarations.Verification
npx tsc --noEmitclean,pnpm test870 passed / 1 skipped.createAudioAnalyser, and the disconnected-room guards.Two gaps, stated plainly:
startAudio()disconnected-room guard ships without a regression test.