Skip to content

Populate localVideoStreamState state with video effects status#2728

Merged
JamesBurnside merged 24 commits intomainfrom
jaburnsi/populate-video-effects-state
Feb 17, 2023
Merged

Populate localVideoStreamState state with video effects status#2728
JamesBurnside merged 24 commits intomainfrom
jaburnsi/populate-video-effects-state

Conversation

@JamesBurnside
Copy link
Copy Markdown
Member

What

When a new local video stream is created we subscribe to changes (effectsStarted, effectsStopped and effectsError), subscriptions happen in two places:

  1. Inside a call, we subscribe when call.onUpdateLocalVideoStreams is triggered with added
    • we unsubscribe when call.onUpdateLocalVideoStreams is triggered with removed
  2. When an unparented view is created
    • we unsubscribe when that view is destroyed

What's not in this PR

  • Calling currently do not expose an isActive properties for streams with effects already applied. They are adding this in the near future (marked as TODO in code)
  • Errors are not propagated to state (marked as TODO in code)

Why

Getting video effects status of the local video stream into state

How Tested

Manually trigger local video streams and verified subscriptions where fired when in config screen and in a cal, and verified localVideoStreamState correctly had the new state properties

image

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 9, 2023

Chat bundle size is not changed.

  • Current size: 5656107
  • Base size: 5656107
  • Diff size: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 9, 2023

CallWithChat bundle size is increased❗.

  • Current size: 5962786
  • Base size: 5959444
  • Diff size: 3342

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 9, 2023

Calling bundle size is increased❗.

  • Current size: 5576139
  • Base size: 5572796
  • Diff size: 3343

Comment thread packages/calling-stateful-client/src/CallContext.ts Outdated
Base automatically changed from jaburnsi/add-video-effects-state-definitions to main February 15, 2023 17:52
Comment thread packages/calling-stateful-client/src/CallContext.ts Outdated
private _recordingSubscriber: RecordingSubscriber;
private _transcriptionSubscriber: TranscriptionSubscriber;
/* @conditional-compile-remove(video-background-effects) */
private _localVideoStreamVideoEffectsSubscribers: Map<string, LocalVideoStreamVideoEffectsSubscriber>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have any other events to be subscribed from stream in near future? If yes then it would be nice to name it LocalStreamSubScriber and put it all there

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh good point, I'll look at making this more generic so new localVideoStream subscriptions could be easily added

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Going to leave this as is. If someone were to create a local video stream subcriber for different events (or a declarativelocalvideostream) they would use this subscriber themselves.

Which from playing around with how this feature should be implemented, could look something like this!~

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/* @conditional-compile-remove(video-background-effects) */
import { Features, LocalVideoStream } from '@azure/communication-calling';
/* @conditional-compile-remove(video-background-effects) */
import { CallContext } from './CallContext';
/* @conditional-compile-remove(video-background-effects) */
import { CallIdRef } from './CallIdRef';
/* @conditional-compile-remove(video-background-effects) */
import { convertSdkLocalStreamToDeclarativeLocalStream } from './Converter';
/* @conditional-compile-remove(video-background-effects) */
import { LocalVideoStreamVideoEffectsSubscriber } from './LocalVideoStreamVideoEffectsSubscriber';

/* @conditional-compile-remove(video-background-effects) */
/**
 * @private
 */
export class ProxyLocalVideoStream implements ProxyHandler<LocalVideoStream> {
  private _context: CallContext;
  private _callIdRef: CallIdRef | 'unparented';
  private _localVideoStreamVideoEffectsSubscriber?: LocalVideoStreamVideoEffectsSubscriber;

  constructor(context: CallContext, callId: CallIdRef | 'unparented') {
    this._context = context;
    this._callIdRef = callId;
  }

  public construct(target: LocalVideoStream): LocalVideoStream {
    const statefulLocalVideoStream = convertSdkLocalStreamToDeclarativeLocalStream(target);
    const effectsApi = target.feature(Features.VideoEffects);
    this._localVideoStreamVideoEffectsSubscriber = new LocalVideoStreamVideoEffectsSubscriber({
      parent: this._callIdRef,
      context: this._context,
      localVideoStream: statefulLocalVideoStream,
      localVideoStreamEffectsAPI: effectsApi
    });
    return new Proxy(target, this);
  }

  public unsubscribe(): void {
    if (this._localVideoStreamVideoEffectsSubscriber) {
      this._localVideoStreamVideoEffectsSubscriber.unsubscribe();
    }
  }

  public get<P extends keyof LocalVideoStream>(target: LocalVideoStream, prop: P): any {
    switch (prop) {
      case 'feature': {
        return async (...args: Parameters<LocalVideoStream['feature']>) => {
          if (args[0] === Features.VideoEffects) {
            const feature = target.feature(Features.VideoEffects);
            return {
              ...feature,
              dispose: (...args: Parameters<typeof feature.dispose>) => {
                this.unsubscribe();
                return feature.dispose(...args);
              }
            };
          } else {
            return target.feature(...args);
          }
        };
      }
      default:
        return Reflect.get(target, prop);
    }
  }
}

/* @conditional-compile-remove(video-background-effects) */
/**
 * Creates a declarative LocalVideoStream that is backed by a LocalVideoStream from the SDK.
 * Calling methods on this declarative object triggers state updates in the stateful client.
 */
export const localVideoStreamDeclaratify = (
  view: LocalVideoStream,
  context: CallContext,
  callId: CallIdRef | 'unparented'
): LocalVideoStream => {
  const proxyLocalVideoStream = new ProxyLocalVideoStream(context, callId);
  return new Proxy(view, proxyLocalVideoStream) as LocalVideoStream;
};

export {};

/* @conditional-compile-remove(video-background-effects) */
{
this._unparentedViewVideoEffectsSubscriber?.unsubscribe();
this._unparentedViewVideoEffectsSubscriber = new LocalVideoStreamVideoEffectsSubscriber({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can see why we put a subscriber here - cause we only create localStream in StreamUtil function then it's hard to get the stream and subscribe to it in other functions

But this adds complexity to the overall dependency graph, previously call context is only responsible for getting and setting data and all the other logics are in subscriber/client
But now it's like subscriber->context->subscriber->context, so it could be easily a circular ref when someone doesn't understand this well

I guess either finding a way to create stream outside of context or getting stream from internal context after the creation or even creating subscriber in internal context would be nice

Lets keep CallContext only the final stop for data and logic to flow in ;D

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

great point, will look into where else it could go...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved the subscription logic into internalContext, not super ideal as then the internalcontext kinda has a handle to the regular context but changes
subscriber->context->subscriber->context
to
subscriber->internalcontext->subscriber->context
which is a little nicer!

@github-actions
Copy link
Copy Markdown
Contributor

Failed to pass the Static HTML UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

@JamesBurnside JamesBurnside enabled auto-merge (squash) February 17, 2023 22:45
@JamesBurnside JamesBurnside merged commit 4b7486a into main Feb 17, 2023
@JamesBurnside JamesBurnside deleted the jaburnsi/populate-video-effects-state branch February 17, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants