fix: refactor background filters #1415
Conversation
| }, [canvasRef]); | ||
| const start = useCallback( | ||
| (ms: MediaStream) => { | ||
| return new Promise<[MediaStream, () => void]>((resolve, reject) => { |
There was a problem hiding this comment.
That's the whole point of this PR: to rewrite background filter pipeline as a single linear function, instead of an effects-based logic.
| resolve([ | ||
| outputStream, | ||
| () => { | ||
| renderer.dispose(); | ||
| videoEl.srcObject = null; | ||
| disposeOfMediaStream(outputStream); | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
With the new filter API, filter returns a stream and a cleanup function. The cleanup function will be called when the filter is unregistered or when the source stream is muted.
| const ready = withoutConcurrency( | ||
| this.filterRegistrationConcurrencyTag, | ||
| async () => { | ||
| this.filters.push(entry); | ||
| await this.applySettingsToStream(); | ||
| }, | ||
| ); | ||
|
|
||
| return { | ||
| ready, | ||
| unregister: () => | ||
| withoutConcurrency(this.filterRegistrationConcurrencyTag, async () => { | ||
| const cleanup = entry[1]; | ||
| cleanup?.(); | ||
| this.filters = this.filters.filter((f) => f !== entry); | ||
| await this.applySettingsToStream(); | ||
| }), | ||
| }; |
There was a problem hiding this comment.
We move the logic preventing unregistering before finishing registering into the client. Note how both registering and unregistering logic is wrapped in withoutConcurrency.
It's convenient to return the unregister function immediately, so the method is no longer async. The ready promise is there if you want to await for registration.
This can be further optimized. Most of the times we unregister one filter to immediately register another. We can introduce a method called replaceFilter that will only call applySettingsToStream once. That way we won't get flickering when switching blur levels or background images.
There was a problem hiding this comment.
I don't like the API with filters returning [stream, cleanup] tuple, leaving it in this draft, but will probably change before merging.
| const { ready: noiseCancellationRegistration, unregister } = | ||
| this.registerFilter(noiseCancellation.toFilter()); | ||
| this.noiseCancellationRegistration = noiseCancellationRegistration.then( | ||
| () => unregister, |
There was a problem hiding this comment.
I had to change things around the noise cancellation filter a bit, but I haven't looked to deep into it. Probably the code can be simplified in this file.
packages/react-sdk/src/components/BackgroundFilters/BackgroundFilters.tsx
Outdated
Show resolved
Hide resolved
packages/react-sdk/src/components/BackgroundFilters/BackgroundFilters.tsx
Show resolved
Hide resolved
No description provided.