feat: effects api #52

mccarthytyler · 2023-05-10T18:03:00Z

Start of the effects API. Right now it requires BaseEffect from @webex/web-media-effects which brings in quite a few dependencies that we probably don't want.

bbaldino · 2023-05-10T18:32:51Z

src/media/local-stream.ts

@@ -9,12 +10,30 @@ interface LocalStreamEvents {
  [LocalStreamEventNames.ConstraintsChange]: TypedEvent<() => void>;
 }

+export type TrackEffect = BaseEffect;


I think we can just use the name web-media-effects defines (BaseEffect). If anything, maybe web-media-effects should rename BaseEffect to TrackEffect?

Did web-media-effects end up defining a TrackEffect interface? Or it still just has BaseEffect?

bbaldino · 2023-05-10T18:34:03Z

src/media/local-stream.ts

+    const outputTrack = await effect.load(this.outputStream.getTracks()[0]);
+    this.effects.push({ name, effect });
+    replaceTrack(this.outputStream, outputTrack);
+
+    // When the effect's track is updated, update the next effect or output stream.
+    effect.on(EffectEvent.TrackUpdated, (track: MediaStreamTrack) => {
+      const effectIndex = this.effects.findIndex((i) => i.name === name);
+      if (effectIndex === this.effects.length - 1) {
+        replaceTrack(this.outputStream, track);
+      } else {
+        this.effects[effectIndex + 1]?.effect.replaceInputTrack(track);
+      }
+    });


Do we need to do the 'pendingEffect' scheme that was recently added in webrtc-core main?

src/media/local-stream.ts

bbaldino · 2023-05-10T18:34:58Z

src/media/local-stream.ts

+
+    // When the effect's track is updated, update the next effect or output stream.
+    effect.on(EffectEvent.TrackUpdated, (track: MediaStreamTrack) => {
+      const effectIndex = this.effects.findIndex((i) => i.name === name);


nit: use e instead of i here? i makes me think it's an index.

bbaldino · 2023-05-10T21:50:24Z

src/media/local-stream.ts

+      await effect.dispose();
+      throw new Error(`Effect already exists with name "${name}"`);


According to the web client team, there's a use case to replace an earlier version of an effect with a newer one (which may have different params)--I think that's what the current webrtc-core supports. If we want to enforce they dispose the old one first, we should check in with them.

Ah I must have misread. For now I can replicate their implementation.

bbaldino · 2023-05-10T21:51:10Z

src/media/local-stream.ts

 /**
 * A stream which originates on the local device.
 */
 abstract class _LocalStream extends Stream {
  [LocalStreamEventNames.ConstraintsChange] = new TypedEvent<() => void>();

+  private effects: EffectItem[] = [];
+
+  private loadingEffects: string[] = [];


Because order isn't important here, I think this could be a map keyed by the effect name, which would make the lookups a bit easier.

bbaldino · 2023-05-11T21:22:19Z

src/media/local-stream.spec.ts

+    mockStream.getTracks()[0].enabled = true;
+
+    localStream.setMuted(true);
+    expect(privateAccess(localStream).outputStream.getTracks()[0].enabled).toBe(false);


can't we verify these on mockStream instead of using privateAccess?

i think this change may have gotten lost?

I changed the above code to:

mockStream.getTracks()[0].enabled = true; localStream.setMuted(true); expect(mockStream.getTracks()[0].enabled).toBe(false); localStream.setMuted(false); expect(mockStream.getTracks()[0].enabled).toBe(true);

bbaldino · 2023-05-11T21:22:58Z

src/media/local-stream.ts

@@ -130,7 +130,7 @@ abstract class _LocalStream extends Stream {
   */
  async disposeEffects(): Promise<void> {
    // Dispose of any effects currently in use
-    if (this.effects.length > 0) {
+    if (this.loadingEffects.size > 0) {


I think we do want this to be this.effects?

bbaldino

one more nit on the tests, but otherwise lgtm

bbaldino · 2023-05-11T22:11:56Z

src/media/local-stream.spec.ts

+    const addEffectPromise = localStream.addEffect('test-effect', effect as unknown as BaseEffect);
+
+    await expect(addEffectPromise).resolves.toBeUndefined();
+    expect(privateAccess(localStream).outputStream.getTracks()[0]).toBe(effectTrack);


I think these uses of privateAccess can be changed too?

In these cases I think we want to make sure the output stream has the track that the effect is outputting

ok...how about just having TestLocalStream expose it?

Though, to be honest, I can't remember why Stream doesn't expose a getter for outputStream, I'm pretty sure we'll end up with one (even it's just an abstract getter at the Stream level)

I ended up having TestLocalStream expose it, I can go back and update if/when the getter is added. Is that something we want to add on this PR?

Ok cool. Nah we'll do that later

bbaldino

one more nit on the tests, but otherwise lgtm

* feat: new stream classes initial commit (#49) * feat: new stream classes initial commit * refactor: originStream -> inputStream * feat: integrate new stream types (#50) * feat: replace LocalMicrophoneTrack with LocalMicrophoneStream * refactor: replace LocalDisplayTrack with LocalDisplayStream * refactor: replace LocalCameraTrack with LocalCameraStream * refactor: remove LocalTrack * refactor: rename originTrack -> inputTrack * feat: effects api (#52) * feat: add effects api * fix: add loadingeffects protection * test: localstream unit tests * chore: upgrade effects version * refactor: add methods and update events for stream classes (#55) * refactor: add support for wcme refactor * chore: minor updates to LocalStream and unit tests --------- Co-authored-by: Bryce Tham <[email protected]> * feat: limit appliable constraints (#56) * feat: limit appliable constraints * refactor: pick VideoDeviceConstraints from MediaTrackConstraints --------- Co-authored-by: Bryce Tham <[email protected]> * feat: listen to track events and additional updates to stream classes (#57) * feat: listen to track events and additional updates to stream classes * fix: preserve outputStream in changeOutputTrack * chore: change outputStream to readonly * chore: update unit tests and comments --------- Co-authored-by: Bryce Tham <[email protected]> * chore: update docs and errors in device management API * fix: stop outputTrack on LocalStream stop --------- Co-authored-by: bbaldino <[email protected]> Co-authored-by: Tyler McCarthy <[email protected]> Co-authored-by: Bryce Tham <[email protected]>

# [1.8.0](v1.7.1...v1.8.0) (2023-07-19) ### Features * convert track-based classes into stream-based classes ([#58](#58)) ([a543678](a543678)), closes [#49](#49) [#50](#50) [#52](#52) [#55](#55)

* feat: new stream classes initial commit (#49) * feat: new stream classes initial commit * refactor: originStream -> inputStream * feat: integrate new stream types (#50) * feat: replace LocalMicrophoneTrack with LocalMicrophoneStream * refactor: replace LocalDisplayTrack with LocalDisplayStream * refactor: replace LocalCameraTrack with LocalCameraStream * refactor: remove LocalTrack * refactor: rename originTrack -> inputTrack * feat: effects api (#52) * feat: add effects api * fix: add loadingeffects protection * test: localstream unit tests * chore: upgrade effects version * refactor: add methods and update events for stream classes (#55) * refactor: add support for wcme refactor * chore: minor updates to LocalStream and unit tests --------- Co-authored-by: Bryce Tham <[email protected]> * feat: limit appliable constraints (#56) * feat: limit appliable constraints * refactor: pick VideoDeviceConstraints from MediaTrackConstraints --------- Co-authored-by: Bryce Tham <[email protected]> * feat: listen to track events and additional updates to stream classes (#57) * feat: listen to track events and additional updates to stream classes * fix: preserve outputStream in changeOutputTrack * chore: change outputStream to readonly * chore: update unit tests and comments --------- Co-authored-by: Bryce Tham <[email protected]> * chore: update docs and errors in device management API * fix: stop outputTrack on LocalStream stop --------- Co-authored-by: bbaldino <[email protected]> Co-authored-by: Tyler McCarthy <[email protected]> Co-authored-by: Bryce Tham <[email protected]> BREAKING CHANGE: convert track-based classes into stream-based classes

# [1.8.0](v1.7.1...v1.8.0) (2023-07-19) ### Features * convert track-based classes into stream-based classes ([#59](#59)) ([77e525c](77e525c)), closes [#49](#49) [#50](#50) [#52](#52) [#55](#55)

feat: add effects api

1249bd6

mccarthytyler requested review from bbaldino and andiwils May 10, 2023 18:03

bbaldino reviewed May 10, 2023

View reviewed changes

mccarthytyler force-pushed the tymccart/effect-api branch from c68a261 to 363f792 Compare May 10, 2023 19:31

bbaldino reviewed May 10, 2023

View reviewed changes

fix: add loadingeffects protection

791e377

mccarthytyler force-pushed the tymccart/effect-api branch from 363f792 to 791e377 Compare May 11, 2023 18:55

bbaldino reviewed May 11, 2023

View reviewed changes

mccarthytyler force-pushed the tymccart/effect-api branch from e4ab49b to 97e41fb Compare May 11, 2023 22:01

bbaldino reviewed May 11, 2023

View reviewed changes

mccarthytyler requested a review from bbaldino May 24, 2023 17:23

mccarthytyler added 2 commits May 25, 2023 10:40

test: localstream unit tests

720bfdc

chore: upgrade effects version

138529b

mccarthytyler force-pushed the tymccart/effect-api branch from 60b80b3 to 138529b Compare May 25, 2023 15:41

bbaldino approved these changes May 25, 2023

View reviewed changes

mccarthytyler merged commit 3c96f27 into stream_classes May 25, 2023

mccarthytyler deleted the tymccart/effect-api branch May 25, 2023 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: effects api #52

feat: effects api #52

mccarthytyler commented May 10, 2023

bbaldino May 10, 2023

bbaldino May 24, 2023

bbaldino May 10, 2023

mccarthytyler May 11, 2023

bbaldino May 10, 2023

mccarthytyler May 11, 2023

bbaldino May 10, 2023

mccarthytyler May 11, 2023

bbaldino May 10, 2023

mccarthytyler May 11, 2023

bbaldino May 11, 2023

mccarthytyler May 11, 2023

bbaldino May 24, 2023

mccarthytyler May 24, 2023

bbaldino May 11, 2023

mccarthytyler May 11, 2023

bbaldino left a comment

bbaldino May 11, 2023

mccarthytyler May 24, 2023

bbaldino May 25, 2023

bbaldino May 25, 2023

mccarthytyler May 25, 2023

bbaldino May 25, 2023

bbaldino left a comment

		await effect.dispose();
		throw new Error(`Effect already exists with name "${name}"`);

feat: effects api #52

feat: effects api #52

Conversation

mccarthytyler commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbaldino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbaldino left a comment

Choose a reason for hiding this comment