Add new BackgroundProcessorManager to provide a better interface for changing between background blur / virtual background#95
Conversation
…witchToBackgroundBlur / switchToVirtualBackground / etc
🦋 Changeset detectedLatest commit: 502f41e 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 |
| /** | ||
| * A preconfigured background processor that supports blurring the background of a user's local | ||
| * video, or replacing the user's background with a virtual background image, and switching the | ||
| * active mode later on the fly to avoid visual artifacts. | ||
| * | ||
| * @example | ||
| * const camTrack = currentRoom.localParticipant.getTrackPublication(Track.Source.Camera)!.track as LocalVideoTrack; | ||
| * camTrack.setProcessor(BackgroundProcessorManager.withBackgroundBlur()); | ||
| */ | ||
| export class BackgroundProcessorManager extends ProcessorWrapper<BackgroundOptions> { |
There was a problem hiding this comment.
I really struggled with what to call this new abstraction - really it's just an "enhanced processorwrapper". If this could be a breaking change, I'd call it BackgroundProcessor and get rid of the old implementation. However, that seemed like maybe not the best idea in the near term.
Any ideas are welcome!
There was a problem hiding this comment.
Also not a fan of the current name.
What do you think about BackgroundEffect ?
There was a problem hiding this comment.
You've defined it as a class now, while other processors were exposed as functions you'd call.
What's the expected behaviour right now when calling new BackgroundProcessorManager()
There was a problem hiding this comment.
What do you think about BackgroundEffect ?
I'm stuck sort of on the Processor part because it is a subclass of a ProcessorWrapper. Maybe BackgroundProcessorWrapper?
You've defined it as a class now, while other processors were exposed as functions you'd call. What's the expected behaviour right now when calling new BackgroundProcessorManager()
It's a good point, and kinda of a wider thing I'm proposing as part of this change - getting rid of the explicit function abstraction layer at the top of the stack and moving that functionality into a number of ProcessorWrapper subclasses. Right now that bare constructor isn't really meant to be that meaningful since there are two starting modes.
Assuming this broad strokes direction makes sense, maybe I could do something like make it so if the bare constructor was called, it was effectively an alias for one of the modes, either blur or virtual background? Or maybe if I introduced that third "disabled" state we had discussed earlier this week, that could be the default generated by the bare constructor? (though there'd probably still be a staticmethod constructor for that case too).
There was a problem hiding this comment.
I'm stuck sort of on the Processor part because it is a subclass of a ProcessorWrapper. Maybe BackgroundProcessorWrapper?
currently they're simply called VirtualBackground and BackgroundBlur, so I'm not too worried about the processor part. Understandable though that BackgroundProcessor would be the preferred name.
Given that (and the fact that a method is already exposed with the same name), could we simply extend the return type of the existing BackgroundProcessor() with dedicated switching between blur + background.
I understand that this isn't exactly what you had envisioned, but it has some low friction advantages:
- Usage patterns wouldn't have to change
- Existing Blur and VirtualBg would continue to work and get the enhanced switching API as an intended side effect
- no naming woes
- we don't have to worry about the bare constructor thing
|
|
||
| /** | ||
| * Instantiates a background processor that is configured in virtual background mode. | ||
| * @deprecated Use `BackgroundProcessorManager.withVirtualBackground` instead. | ||
| */ | ||
| export const VirtualBackground = ( | ||
| imagePath: string, | ||
| segmenterOptions?: SegmenterOptions, | ||
| onFrameProcessed?: (stats: FrameProcessingStats) => void, |
There was a problem hiding this comment.
The old VirtualBackground / BackgroundBlur functions still exist but are marked as @deprecated now. I'm thinking they could be removed in the next major version.
There was a problem hiding this comment.
Eventhough it's technically not necessary for pre 1.0 releases, I'd still argue we should make this PR a minor version update
…er interface for changing between background blur / virtual background
|
Closing in favor of #107 |
2 participants
Previously, switching between these two modes was relatively unintuitive, and not well documented. #94 was a good first pass at this, but I thought there was a potential for some sort of better interface so I gave that a shot here. See the
READMEfor a usage example!Also, while I was in here, I added a fair number of documentation comments to try to point folks towards the new interfaces.