feat: update effects APIs #68
Conversation
src/media/local-stream.ts
Outdated
| const outputTrack = await effect.load(this.outputTrack); | ||
| async addEffect(effect: TrackEffect): Promise<void> { | ||
| // Check if the effect has already been added. | ||
| if (this.effects.includes(effect)) { |
There was a problem hiding this comment.
What does this end up using to compare equality?
There was a problem hiding this comment.
It just checks to see if the instances match, but I do think explicitly comparing id or kind would be better, so I can make that change.
| // After loading, check whether or not we still want to use this effect. If another effect of | ||
| // the same kind was added while this effect was loading, we only want to use the latest effect, | ||
| // so dispose this one. If the effects list was cleared while this effect was loading, also | ||
| // dispose it. |
There was a problem hiding this comment.
It feels a bit strange that we'll allow overriding an affect of the same type that was added previously if it didn't finish loading yet, but we error on adding an effect of the same type after it's been loaded. I think I'd expect that subsequent additions of the same type of effect (without removing the previous one) should either always work or always fail?
There was a problem hiding this comment.
The previous behavior was to override the effect if it didn't finish loading, so I think we should keep that behavior (since the client does depend on it). We didn't handle adding a duplicate effect after it has loaded previously, so I can change it so that it will also override instead of error.
src/media/local-stream.ts
Outdated
| * @param kind - The kind of the effects you want to get. | ||
| * @returns A list of effects. | ||
| */ | ||
| getEffectsByKind(kind: string): TrackEffect[] { |
There was a problem hiding this comment.
This returning an array seems inconsistent with the fact that we only allow one instance of each effect type
There was a problem hiding this comment.
Sure, will change it.
There was a problem hiding this comment.
just curious , if we knew there is only 3 kind of effects why didn't we have some function like addVirtualBackgroundEffect rather have names passed in to the effects ?
There was a problem hiding this comment.
Well there are 3 effects now, but we want to support any kind of effect in the future as opposed to hard-coding the current ones in.
| WebrtcCoreErrorType.ADD_EFFECT_FAILED, | ||
| `Effect ${effect.id} has already been added to this stream.` | ||
| ); | ||
| if (this.effects.some((e) => e.id === effect.id)) { |
There was a problem hiding this comment.
So this now checks if the same effect instance is being added, but I think we need to check for an existing effect of the same kind to replace it with this one?
There was a problem hiding this comment.
Oh, I didn't notice that all the code below is in this same method, I see there's handling there for this.
There was a problem hiding this comment.
There's logic starting at line 205 that handles this case.
# [2.3.0](v2.2.3...v2.3.0) (2024-01-10) ### Features * update effects APIs ([#68](#68)) ([64ac8b6](64ac8b6))
This PR makes API changes to the
LocalStreamclass regarding effects. The main change is eliminating the need to pass anameparam to theaddEffectmethod, since the effect object has its own identifiers as of https://github.com/webex/web-media-effects/pull/59.The following APIs have been added:
getEffectByIdgetEffectByKindThe following API has been removed:
getEffectThe following API has been renamed:
getAllEffects->getEffects(I think this new name makes sense with the new APIs; it matches the built-inMediaStreamAPIs, which havegetTracksandgetTrackById, for example)