Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update effects APIs #68

Merged
merged 2 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
"dependencies": {
"@webex/ts-events": "^1.1.0",
"@webex/web-capabilities": "^1.1.0",
"@webex/web-media-effects": "^2.15.2",
"@webex/web-media-effects": "^2.15.6",
"events": "^3.3.0",
"js-logger": "^1.6.1",
"typed-emitter": "^2.1.0",
Expand Down
27 changes: 23 additions & 4 deletions src/media/local-stream.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ describe('LocalStream', () => {
effect = {
id: 'test-id',
kind: 'test-kind',
isEnabled: false,
dispose: jest.fn().mockResolvedValue(undefined),
load: jest.fn().mockResolvedValue(undefined),
on: jest.fn(),
Expand All @@ -76,11 +77,15 @@ describe('LocalStream', () => {
expect(emitSpy).toHaveBeenCalledWith(effect);
});

it('should load and add multiple effects', async () => {
it('should load and add multiple effects with different IDs and kinds', async () => {
expect.hasAssertions();

const firstEffect = effect;
const secondEffect = { ...effect, kind: 'another-kind' } as unknown as TrackEffect;
const secondEffect = {
...effect,
id: 'another-id',
kind: 'another-kind',
} as unknown as TrackEffect;
await localStream.addEffect(firstEffect);
await localStream.addEffect(secondEffect);

Expand All @@ -89,13 +94,13 @@ describe('LocalStream', () => {
expect(emitSpy).toHaveBeenCalledTimes(2);
});

it('should throw an error if the effect is already added', async () => {
it('should not load an effect with the same ID twice', async () => {
expect.hasAssertions();

await localStream.addEffect(effect);
const secondAddEffectPromise = localStream.addEffect(effect);

await expect(secondAddEffectPromise).rejects.toBeInstanceOf(WebrtcCoreError);
await expect(secondAddEffectPromise).resolves.toBeUndefined(); // no-op
expect(loadSpy).toHaveBeenCalledTimes(1);
expect(localStream.getEffects()).toStrictEqual([effect]);
expect(emitSpy).toHaveBeenCalledTimes(1);
Expand All @@ -116,6 +121,20 @@ describe('LocalStream', () => {
expect(emitSpy).toHaveBeenCalledTimes(1);
});

it('should replace the effect if an effect of the same kind is added after loading', async () => {
expect.hasAssertions();

const firstEffect = effect;
const secondEffect = { ...effect, id: 'another-id' } as unknown as TrackEffect; // same kind
await localStream.addEffect(firstEffect);
const secondAddEffectPromise = localStream.addEffect(secondEffect);

await expect(secondAddEffectPromise).resolves.toBeUndefined();
expect(loadSpy).toHaveBeenCalledTimes(2);
expect(localStream.getEffects()).toStrictEqual([secondEffect]);
expect(emitSpy).toHaveBeenCalledTimes(2);
});

it('should throw an error if effects are cleared while loading', async () => {
expect.hasAssertions();

Expand Down
81 changes: 60 additions & 21 deletions src/media/local-stream.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { AddEvents, TypedEvent, WithEventsDummyType } from '@webex/ts-events';
import { BaseEffect, EffectEvent } from '@webex/web-media-effects';
import { WebrtcCoreError, WebrtcCoreErrorType } from '../errors';
import { logger } from '../util/logger';
import { Stream, StreamEventNames } from './stream';

export type TrackEffect = BaseEffect;
Expand Down Expand Up @@ -148,11 +149,8 @@ abstract class _LocalStream extends Stream {
*/
async addEffect(effect: TrackEffect): Promise<void> {
// Check if the effect has already been added.
if (this.effects.includes(effect)) {
throw new WebrtcCoreError(
WebrtcCoreErrorType.ADD_EFFECT_FAILED,
`Effect ${effect.id} has already been added to this stream.`
);
if (this.effects.some((e) => e.id === effect.id)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't notice that all the code below is in this same method, I see there's handling there for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's logic starting at line 205 that handles this case.

return;
}

// Load the effect. Because loading is asynchronous, keep track of the loading effects.
Expand All @@ -172,21 +170,62 @@ abstract class _LocalStream extends Stream {
}
this.loadingEffects.delete(effect.kind);

// Add the effect to the effects list.
this.effects.push(effect);

// When the effect's track is updated, update the next effect or output stream.
// TODO: using EffectEvent.TrackUpdated will cause the entire web-media-effects lib to be built
// and makes the size of the webrtc-core build much larger, so we use type assertion here as a
// temporary workaround.
effect.on('track-updated' as EffectEvent, (track: MediaStreamTrack) => {
const effectIndex = this.effects.indexOf(effect);
/**
* Handle when the effect's output track has been changed. This will update the input of the
* next effect in the effects list of the output of the stream.
*
* @param track - The new output track of the effect.
*/
const handleEffectTrackUpdated = (track: MediaStreamTrack) => {
const effectIndex = this.effects.findIndex((e) => e.id === effect.id);
if (effectIndex === this.effects.length - 1) {
this.changeOutputTrack(track);
} else {
} else if (effectIndex >= 0) {
this.effects[effectIndex + 1]?.replaceInputTrack(track);
} else {
logger.error(`Effect with ID ${effect.id} not found in effects list.`);
}
};

/**
* Handle when the effect has been disposed. This will remove all event listeners from the
* effect.
*/
const handleEffectDisposed = () => {
effect.off('track-updated' as EffectEvent, handleEffectTrackUpdated);
effect.off('disposed' as EffectEvent, handleEffectDisposed);
};

// TODO: using EffectEvent.TrackUpdated or EffectEvent.Disposed will cause the entire
// web-media-effects lib to be rebuilt and inflates the size of the webrtc-core build, so
// we use type assertion here as a temporary workaround.
effect.on('track-updated' as EffectEvent, handleEffectTrackUpdated);
effect.on('disposed' as EffectEvent, handleEffectDisposed);

// Add the effect to the effects list. If an effect of the same kind has already been added,
// dispose the existing effect and replace it with the new effect. If the existing effect was
// enabled, also enable the new effect.
const existingEffectIndex = this.effects.findIndex((e) => e.kind === effect.kind);
if (existingEffectIndex >= 0) {
const [existingEffect] = this.effects.splice(existingEffectIndex, 1, effect);
if (existingEffect.isEnabled) {
// If the existing effect is not the first effect in the effects list, then the input of the
// new effect should be the output of the previous effect in the effects list. We know the
// output track of the previous effect must exist because it must have been loaded (and all
// loaded effects have an output track).
const inputTrack =
existingEffectIndex === 0
? this.inputTrack
: (this.effects[existingEffectIndex - 1].getOutputTrack() as MediaStreamTrack);
await effect.replaceInputTrack(inputTrack);
// Enabling the new effect will trigger the track-updated event, which will handle the new
// effect's updated output track.
await effect.enable();
}
});
await existingEffect.dispose();
} else {
this.effects.push(effect);
}

// Emit an event with the effect so others can listen to the effect events.
this[LocalStreamEventNames.EffectAdded].emit(effect);
Expand All @@ -203,13 +242,13 @@ abstract class _LocalStream extends Stream {
}

/**
* Get all the effects from the effects list with the given kind.
* Get an effect from the effects list by kind.
*
* @param kind - The kind of the effects you want to get.
* @returns A list of effects.
* @param kind - The kind of the effect you want to get.
* @returns The effect or undefined.
*/
getEffectsByKind(kind: string): TrackEffect[] {
return this.effects.filter((effect) => effect.kind === kind);
getEffectByKind(kind: string): TrackEffect | undefined {
return this.effects.find((effect) => effect.kind === kind);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2687,10 +2687,10 @@
dependencies:
bowser "^2.11.0"

"@webex/web-media-effects@^2.15.2":
version "2.15.2"
resolved "https://registry.yarnpkg.com/@webex/web-media-effects/-/web-media-effects-2.15.2.tgz#846efa44d8c88aeadad0856805662807e2f162c0"
integrity sha512-+a5DU45D0Evdl8fi8xx39tlHRhLWEHbsxB//co90q0MHH1Hxc6iNkKuqK1zl56iYIROhdwCrl1gt5h0cKHDEUA==
"@webex/web-media-effects@^2.15.6":
version "2.15.6"
resolved "https://registry.yarnpkg.com/@webex/web-media-effects/-/web-media-effects-2.15.6.tgz#9506963447a6c96435bd1b01acab0f87a6b7d7df"
integrity sha512-uK8sg14FtCwGvpRKi3guJuA6/I/qSI0v8lGblWvpYBqnfgNAki0pzHfJn6H1oiVK03zWqYQ3uzVDvS07hMAghg==
dependencies:
"@webex/ladon-ts" "^4.2.4"
events "^3.3.0"
Expand Down