Add logic to BackgroundProcessor to allow dynamically switching modes elegantly, and deprecate pre-existing interfaces

[1egoman]

In a past pull request, I had attempted to come up with a more elegant way to dynamically switch between track processor modes, but got the feedback what I was proposing was a bit too large of a leap and not backwards compatible enough.

Summary

So, I've attempted to do this again in this change, only via a bit of different mechanism:

1. I've updated BackgroundProcessor to now optionally take a mode key when being initially set up to define an initial starting mode. For example: BackgroundProcessor({ mode: 'background-blur', blurRadius: BLUR_RADIUS }) or BackgroundProcessor({ mode: 'virtual-background', imagePath: "..." }).
2. I've also updated BackgroundProcessor's return value - it now returns a new superclass of ProcessorWrapper called BackgroundProcessorWrapper which exposes switchToBackgroundBlur and switchToVirtualBackground functions, which allows for more convenient switching as opposed to calling updateTransformerOptions manually.

In addition, per some other discussions I've had in other issues / pull requests, I wanted to put a demo together that would showcase an experience which wouldn't have any flicker due to media pipeline delay (technical info about this can be found here). In order to accomplish this, I've added a "disabled" mode to the BackgroundProcessor - so there now is a new BackgroundProcessor({ mode: 'disabled' }) constructor signature, and also a new switchToDisabled method on the BackgroundProcessorWrapper.

This means that a user can effectively get rid of all "flicker" by enable the background processor immediately after connecting in the disabled mode, and then later on since the media pipeline is all set up, there will be no artifacts when enabling either effect.

Demo

I also updated the demo - previously, the "toggle buttons" provided for a quite weird interface for both controlling whether the processor was enabled / disabled, and also setting which mode it is in. Now, these are controlled seperately.

Untitled.mov

[changeset-bot]

🦋 Changeset detected

Latest commit: 1b193d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@livekit/track-processors	Patch

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

[xianshijing-lk]

some nits. lgtm, but please wait for Lukas' review if we are not in a rush to land it.

[lukasIO]

The direction makes sense to me!

The sample is currently in a bit of an inbetween state with both the new insert and switch buttons visible, but also the toggleBackground still being available as a separate button (albeit always disabled) and the update background image button also being in a separate section of the UI.

I don't know if this particular PR introduced this, but there's currently a flash of the "real" background if you click flipCam which restarts the track.
I think that's rather unexpected.

[lukasIO]

Have you considered a switchTo(mode, ...opts?) method?

Not saying that would necessarily be preferable, just curious about your thoughts on one vs the other.

[1egoman]

I don't think I have a strong preference. A few unstructured thoughts:

• In an IDE of some sort, if you type backgroundProcessor. and let the autocomplete pop up, in the current state you'd see all the options, versus just switchTo.
• At least initially, switchTo seems like it would be nice because you could parameterize the values being passed in (ie, instead of all the big switch (state.backgroundMode) type stuff in the example app or similar logic in a user's app, you could somehow just flatten that down to one switchTo call), but in practice this doesn't really work out because the overloads would make doing that in a type safe way challenging.
  • I think the better way to do switchTo might be switchTo(opts: BackgroundProcessorOptions) - so that would look like backgroundProcessor.switchTo({ mode: 'background-blur', blurRadius: 10 }).

Typing all that out, there's maybe an argument to have both? Especially since one is more discoverable and one is more flexible. I think I'll add both for now and see if anybody strongly opposes doing that.

[1egoman]

Added both!

[lukasIO]

Typing all that out, there's maybe an argument to have both?

I'm a fan of a lean public API surface. I'd say let's pick one? Don't have a strong opinion on one vs the other, will try to answer to the points your raised:

if you type backgroundProcessor. and let the autocomplete pop up, in the current state you'd see all the options, versus just switchTo

true, but you'll still get it for mode (which reminds me, maybe mode should be it's own string union type?) which seems also rather obvious.

I think the better way to do switchTo might be switchTo(opts: BackgroundProcessorOptions)

yeah, that looks nicer!

[1egoman]

I'm a fan of a lean public API surface. I'd say let's pick one?

Cool, I went with switchTo({ mode: 'background-blur', blurRadius: 10 }) and got rid of the other methods.

which reminds me, maybe mode should be it's own string union type?

It kinda does, via SwitchBackgroundProcessorOptions['mode'] - it's a little tricky though because if I made an explicit string union type, it wouldn't actually be used since the strings must be embedded into the discriminated union for that typing logic to work properly. I guess another option is I could make an enum too which maybe addresses that indirectly, but idk.

[1egoman]

The sample is currently in a bit of an inbetween state with both the new insert and switch buttons visible but also the toggleBackground still being available as a separate button (albeit always disabled)

Ah that's what that button was for, I hadn't realized it was old. Removed.

and the update background image button also being in a separate section of the UI.

Fair enough, moved it into the group!

I don't know if this particular PR introduced this, but there's currently a flash of the "real" background if you click flipCam > which restarts the track.
I think that's rather unexpected.

That is not something this pull request introduced. That being said, on cursory examination, it's for a very similar reason as this: #96. I bet something similar to that fix could be adapted into the client sdk as part of LocalTrack.restart to make that at least somewhat smoother... 🤔

[lukasIO]

I bet something similar to that fix could be adapted into the client sdk

what in particular are you thinking about?

I think the flash happens due to https://github.com/livekit/track-processors-js/blob/main/src/transformers/BackgroundTransformer.ts#L137-L139 ?

[1egoman]

I think the flash happens due to https://github.com/livekit/track-processors-js/blob/main/src/transformers/BackgroundTransformer.ts#L137-L139 ?

@lukasIO From what I can tell no, it actually isn't being caused by that, but a similar type of problem. Tracing it through, videoPub.videoTrack.restartTrack() is called here, which then ends up here, which then calls this.restart and ends up here, which then gets to here in LocalTrack.restart.

Without poking around too much in depth, I'm guessing that here is the issue - the media track is detached from the element, and then a few different async things occur before the restart operation seems to be fully complete. I think what could maybe be done is something similar to the issue where I linked, where a not yet fully initialized stream is attached to the media element right away and maybe the last frame from the previous stream was fed in, leading to less of a perceived delay because the gray flash is avoided.

Anyway, this is an issue on the current main as well so I don't think trying to fix should be included in this change. But, I can make a separate issue / ticket for this if you think what I've outlined is worth digging into!

[1egoman]

(rebased on top of latest main)