Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
daf5506
Add stateful definitions of the video effecst feature
JamesBurnside Feb 2, 2023
07073b5
fix stable build
JamesBurnside Feb 2, 2023
4624aaa
Change files
JamesBurnside Feb 2, 2023
1d7883b
Duplicate change files for beta release
JamesBurnside Feb 2, 2023
4c4ea88
add doc comments
JamesBurnside Feb 3, 2023
5293b45
Create LocalVideoStreamVideoEffectsSubscriber and have it update the …
JamesBurnside Feb 6, 2023
f13ebc5
Change files
JamesBurnside Feb 6, 2023
5c0212c
Duplicate change files for beta release
JamesBurnside Feb 6, 2023
d7e8d1f
Merge remote-tracking branch 'origin/main' into jaburnsi/populate-vid…
JamesBurnside Feb 7, 2023
ff64ae2
stream -> stream
JamesBurnside Feb 9, 2023
40929c6
Merge remote-tracking branch 'origin/main' into jaburnsi/populate-vid…
JamesBurnside Feb 9, 2023
ec85993
Merge remote-tracking branch 'origin/main' into jaburnsi/add-video-ef…
JamesBurnside Feb 9, 2023
23db6a0
Merge remote-tracking branch 'origin/jaburnsi/add-video-effects-state…
JamesBurnside Feb 9, 2023
5d9b31f
remove unused file
JamesBurnside Feb 9, 2023
c2ca240
add calling todo
JamesBurnside Feb 9, 2023
7d9ba9c
improve todos
JamesBurnside Feb 9, 2023
8c1de54
fix tests
JamesBurnside Feb 9, 2023
bf1b5f9
fix test for stable build
JamesBurnside Feb 9, 2023
3369141
Merge branch 'main' into jaburnsi/populate-video-effects-state
JamesBurnside Feb 15, 2023
62e60af
Merge branch 'main' into jaburnsi/populate-video-effects-state
JamesBurnside Feb 15, 2023
b8a20d9
move unparented subscription to context
JamesBurnside Feb 17, 2023
a2c92d9
Merge branch 'main' into jaburnsi/populate-video-effects-state
JamesBurnside Feb 17, 2023
f2bd6b6
Remove unused import
JamesBurnside Feb 17, 2023
fec635f
Merge branch 'main' into jaburnsi/populate-video-effects-state
JamesBurnside Feb 17, 2023
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "In the stateful call client subscribe to video effects changes and populate state when those subscriptions fire'",
"packageName": "@azure/communication-react",
"email": "2684369+JamesBurnside@users.noreply.github.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "In the stateful call client subscribe to video effects changes and populate state when those subscriptions fire'",
"packageName": "@azure/communication-react",
"email": "2684369+JamesBurnside@users.noreply.github.com",
"dependentChangeType": "patch"
}
78 changes: 71 additions & 7 deletions packages/calling-stateful-client/src/CallContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ import {
} from './CallClientState';
import { callingStatefulLogger } from './Logger';
import { CallIdHistory } from './CallIdHistory';
/* @conditional-compile-remove(video-background-effects) */
import { LocalVideoStreamVideoEffectsState } from './CallClientState';
/* @conditional-compile-remove(video-background-effects) */
import { LocalVideoStreamVideoEffectsSubscriber } from './LocalVideoStreamVideoEffectsSubscriber';
/* @conditional-compile-remove(video-background-effects) */
import { VideoEffectsFeature } from '@azure/communication-calling';

enableMapSet();
// Needed to generate state diff for verbose logging.
Expand All @@ -57,6 +63,8 @@ export class CallContext {
private _emitter: EventEmitter;
private _atomicId: number;
private _callIdHistory: CallIdHistory = new CallIdHistory();
/* @conditional-compile-remove(video-background-effects) */
private _unparentedViewVideoEffectsSubscriber: LocalVideoStreamVideoEffectsSubscriber | undefined;

constructor(
userId: CommunicationIdentifierKind,
Expand Down Expand Up @@ -262,6 +270,26 @@ export class CallContext {
});
}

/* @conditional-compile-remove(video-background-effects) */
public setCallLocalVideoStreamVideoEffects(
callId: string,
localVideoStream: LocalVideoStreamState,
videoEffects: Partial<LocalVideoStreamVideoEffectsState>
): void {
this.modifyState((draft: CallClientState) => {
const call = draft.calls[this._callIdHistory.latestCallId(callId)];
if (call) {
const stream = call.localVideoStreams?.find((i) => i.mediaStreamType === 'Video');
if (stream) {
stream.videoEffects = {
isActive: videoEffects.isActive ?? stream.videoEffects?.isActive ?? false,
effectName: videoEffects.effectName ?? stream.videoEffects?.effectName
};
}
}
});
}

public setCallIsMicrophoneMuted(callId: string, isMicrophoneMuted: boolean): void {
this.modifyState((draft: CallClientState) => {
const call = draft.calls[this._callIdHistory.latestCallId(callId)];
Expand Down Expand Up @@ -625,20 +653,36 @@ export class CallContext {
});
}

public setDeviceManagerUnparentedView(
localVideoStream: LocalVideoStreamState,
view: VideoStreamRendererViewState | undefined
): void {
public setDeviceManagerUnparentedView(args: {
localVideoStream: LocalVideoStreamState;
view: VideoStreamRendererViewState | undefined;
/* @conditional-compile-remove(video-background-effects) */
Comment thread
JamesBurnside marked this conversation as resolved.
Outdated
localVideoStreamEffectsAPI: VideoEffectsFeature;
}): void {
/* @conditional-compile-remove(video-background-effects) */
{
this._unparentedViewVideoEffectsSubscriber?.unsubscribe();
Comment thread
JamesBurnside marked this conversation as resolved.
Outdated
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!

parent: 'unparented',
context: this,
localVideoStream: args.localVideoStream,
localVideoStreamEffectsAPI: args.localVideoStreamEffectsAPI
});
}

this.modifyState((draft: CallClientState) => {
draft.deviceManager.unparentedViews.push({
source: localVideoStream.source,
mediaStreamType: localVideoStream.mediaStreamType,
view: view
source: args.localVideoStream.source,
mediaStreamType: args.localVideoStream.mediaStreamType,
view: args.view
});
});
}

public deleteDeviceManagerUnparentedView(localVideoStream: LocalVideoStreamState): void {
/* @conditional-compile-remove(video-background-effects) */
this._unparentedViewVideoEffectsSubscriber?.unsubscribe();

this.modifyState((draft: CallClientState) => {
const foundIndex = draft.deviceManager.unparentedViews.findIndex(
(stream) =>
Expand All @@ -650,6 +694,26 @@ export class CallContext {
});
}

/* @conditional-compile-remove(video-background-effects) */
public setDeviceManagerUnparentedViewVideoEffects(
localVideoStream: LocalVideoStreamState,
videoEffects: LocalVideoStreamVideoEffectsState
): void {
this.modifyState((draft: CallClientState) => {
const foundIndex = draft.deviceManager.unparentedViews.findIndex(
(stream) =>
stream.source.id === localVideoStream.source.id && stream.mediaStreamType === localVideoStream.mediaStreamType
);
if (foundIndex !== -1) {
const draftStream = draft.deviceManager.unparentedViews[foundIndex];
draftStream.videoEffects = {
isActive: videoEffects.isActive ?? draftStream.videoEffects?.isActive ?? false,
effectName: videoEffects.effectName ?? draftStream.videoEffects?.effectName
};
}
});
}

public getAndIncrementAtomicId(): number {
const id = this._atomicId;
this._atomicId++;
Expand Down
26 changes: 26 additions & 0 deletions packages/calling-stateful-client/src/CallSubscriber.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
convertSdkParticipantToDeclarativeParticipant
} from './Converter';
import { InternalCallContext } from './InternalCallContext';
/* @conditional-compile-remove(video-background-effects) */
import { LocalVideoStreamVideoEffectsSubscriber } from './LocalVideoStreamVideoEffectsSubscriber';
import { ParticipantSubscriber } from './ParticipantSubscriber';
import { RecordingSubscriber } from './RecordingSubscriber';
import { disposeView } from './StreamUtils';
Expand All @@ -32,6 +34,8 @@ export class CallSubscriber {
private _participantSubscribers: Map<string, ParticipantSubscriber>;
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 {};


constructor(call: CallCommon, context: CallContext, internalContext: InternalCallContext) {
this._call = call;
Expand All @@ -55,6 +59,8 @@ export class CallSubscriber {
this._context,
this._call.feature(Features.Transcription)
);
/* @conditional-compile-remove(video-background-effects) */
this._localVideoStreamVideoEffectsSubscribers = new Map();

this.subscribe();
}
Expand Down Expand Up @@ -218,8 +224,28 @@ export class CallSubscriber {
undefined
);
this._context.setCallLocalVideoStream(this._callIdRef.callId, [...localVideoStreams]);

/* @conditional-compile-remove(video-background-effects) */
{
const localVideoStreamKey = event.added[0].source.id;
this._localVideoStreamVideoEffectsSubscribers.get(localVideoStreamKey)?.unsubscribe();
this._localVideoStreamVideoEffectsSubscribers.set(
localVideoStreamKey,
new LocalVideoStreamVideoEffectsSubscriber({
parent: this._callIdRef,
context: this._context,
localVideoStream: this._call.localVideoStreams[0],
localVideoStreamEffectsAPI: this._call.localVideoStreams[0].feature(Features.VideoEffects)
})
);
}
}
if (event.removed.length > 0) {
/* @conditional-compile-remove(video-background-effects) */
{
const localVideoStreamKey = event.removed[0].source.id;
this._localVideoStreamVideoEffectsSubscribers.get(localVideoStreamKey)?.unsubscribe();
}
disposeView(
this._context,
this._internalContext,
Expand Down
1 change: 1 addition & 0 deletions packages/calling-stateful-client/src/Converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export function convertSdkLocalStreamToDeclarativeLocalStream(
source: stream.source,
mediaStreamType: stream.mediaStreamType,
view: undefined
// TODO [video-background-effects]: Add video effects state when it is added to the SDK
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/* @conditional-compile-remove(video-background-effects) */
import { VideoEffectErrorPayload, VideoEffectName, VideoEffectsFeature } from '@azure/communication-calling';
/* @conditional-compile-remove(video-background-effects) */
import { LocalVideoStreamState, LocalVideoStreamVideoEffectsState } from './CallClientState';
/* @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) */
/**
* Subscribes to a LocalVideoStream's video effects events and updates the call context appropriately.
* @private
*/
export class LocalVideoStreamVideoEffectsSubscriber {
private _parent: CallIdRef | 'unparented';
private _context: CallContext;
private _localVideoStream: LocalVideoStreamState;
private _localVideoStreamEffectsAPI: VideoEffectsFeature;

constructor(args: {
/** Owner of the local video stream. This is either the Call (referenced by CallIdRef) or is the device manager's unparented view (referenced by 'unparented') */
parent: CallIdRef | 'unparented';
context: CallContext;
localVideoStream: LocalVideoStreamState;
localVideoStreamEffectsAPI: VideoEffectsFeature;
}) {
this._parent = args.parent;
this._context = args.context;
this._localVideoStream = args.localVideoStream;
this._localVideoStreamEffectsAPI = args.localVideoStreamEffectsAPI;

this.subscribe();
}

private subscribe = (): void => {
this._localVideoStreamEffectsAPI.on('effectsStarted', this.effectsStarted);
this._localVideoStreamEffectsAPI.on('effectsStopped', this.effectsStopped);
this._localVideoStreamEffectsAPI.on('effectsError', this.effectsError);
};

public unsubscribe = (): void => {
this._localVideoStreamEffectsAPI.off('effectsStarted', this.effectsStarted);
this._localVideoStreamEffectsAPI.off('effectsStopped', this.effectsStopped);
this._localVideoStreamEffectsAPI.off('effectsError', this.effectsError);
};

private effectsStarted = (effectName: VideoEffectName): void => {
this.updateEffectsState({
isActive: true,
effectName: effectName
});
};

private effectsStopped = (effectName: VideoEffectName): void => {
this.updateEffectsState({
isActive: false,
effectName: effectName
});
};

private effectsError = (error: VideoEffectErrorPayload): void => {
// When there is an error the effects have stopped. Update the state to reflect this.
this.updateEffectsState({
isActive: false
});
// TODO [video-background-effects]: future PR will tee error to state
console.error(
`LocalVideoStreamVideoEffectsSubscriber effectError: Method not fully implemented. Error needs teed to state. ${error}`
);
};

private updateEffectsState = (newEffectsState: LocalVideoStreamVideoEffectsState): void => {
if (this._parent === 'unparented') {
this._context.setDeviceManagerUnparentedViewVideoEffects(this._localVideoStream, newEffectsState);
} else {
this._context.setCallLocalVideoStreamVideoEffects(this._parent.callId, this._localVideoStream, newEffectsState);
}
};
}

export {};
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
DeviceManager,
UserFacingDiagnosticsFeature,
Features,
LocalVideoStream,
RecordingCallFeature,
TranscriptionCallFeature,
VideoStreamRendererView,
Expand All @@ -25,6 +24,7 @@ import {
createMockCall,
createMockCallAgent,
createMockCallClient,
createMockLocalVideoStream,
createMockRemoteParticipant,
createMockRemoteScreenshareStream,
createMockRemoteVideoStream,
Expand Down Expand Up @@ -237,7 +237,7 @@ describe('Stateful call client', () => {

{
const listener = new StateChangeListener(client);
call.testHelperPushLocalVideoStream({} as LocalVideoStream);
call.testHelperPushLocalVideoStream(createMockLocalVideoStream());
expect(await waitWithBreakCondition(() => client.getState().calls[callId]?.localVideoStreams.length !== 0)).toBe(
true
);
Expand Down Expand Up @@ -346,7 +346,7 @@ describe('Stateful call client', () => {
const participantKey = toFlatCommunicationIdentifier(participant.identifier);

// Add a local video stream as well.
call.testHelperPushLocalVideoStream({} as LocalVideoStream);
call.testHelperPushLocalVideoStream(createMockLocalVideoStream());
expect(await waitWithBreakCondition(() => client.getState().calls[callId]?.localVideoStreams.length !== 0)).toBe(
true
);
Expand Down Expand Up @@ -638,15 +638,15 @@ describe('errors should be reported correctly from Call when', () => {

{
const listener = new StateChangeListener(client);
await expect(call.startVideo({} as LocalVideoStream)).rejects.toThrow(
await expect(call.startVideo(createMockLocalVideoStream())).rejects.toThrow(
new CallError('Call.startVideo', new Error('startVideo: injected error'))
);
expect(listener.onChangeCalledCount).toBe(1);
expect(client.getState().latestErrors['Call.startVideo']).toBeDefined();
}
{
const listener = new StateChangeListener(client);
await expect(call.stopVideo({} as LocalVideoStream)).rejects.toThrow(
await expect(call.stopVideo(createMockLocalVideoStream())).rejects.toThrow(
new CallError('Call.stopVideo', new Error('stopVideo: injected error'))
);
expect(listener.onChangeCalledCount).toBe(1);
Expand Down Expand Up @@ -715,7 +715,7 @@ const prepareCall = async (): Promise<PreparedCall> => {
const prepareCallWithLocalVideoStream = async (): Promise<PreparedCall> => {
const prepared = await prepareCall();
const { client, callId, call } = prepared;
call.testHelperPushLocalVideoStream({} as LocalVideoStream);
call.testHelperPushLocalVideoStream(createMockLocalVideoStream());
expect(await waitWithBreakCondition(() => client.getState().calls[callId]?.localVideoStreams.length !== 0)).toBe(
true
);
Expand Down
20 changes: 6 additions & 14 deletions packages/calling-stateful-client/src/StreamUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { CallState, LocalVideoStreamState, RemoteParticipantState, RemoteVideoSt
import { CallContext } from './CallContext';
import { InternalCallContext } from './InternalCallContext';
import { createView, disposeView, disposeAllViewsFromCall, disposeAllViews } from './StreamUtils';
import { createMockRemoteVideoStream } from './TestUtils';
import { createMockLocalVideoStream, createMockRemoteVideoStream } from './TestUtils';
import { toFlatCommunicationIdentifier } from '@internal/acs-ui-common';

jest.mock('@azure/communication-calling', () => {
Expand All @@ -27,18 +27,7 @@ jest.mock('@azure/communication-calling', () => {
}),
// eslint-disable-next-line @typescript-eslint/no-unused-vars
LocalVideoStream: jest.fn().mockImplementation((info: VideoDeviceInfo) => {
return {
source: () => {
return {} as VideoDeviceInfo;
},
mediaStreamType: () => {
return 'Video';
},
// eslint-disable-next-line @typescript-eslint/no-unused-vars
switchSource: (videoDeviceInfo: VideoDeviceInfo) => {
return Promise.resolve();
}
};
return createMockLocalVideoStream();
}),
// eslint-disable-next-line @typescript-eslint/no-unused-vars
VideoStreamRenderer: jest.fn().mockImplementation((videoStream: SdkLocalVideoStream | SdkRemoteVideoStream) => {
Expand All @@ -50,7 +39,10 @@ jest.mock('@azure/communication-calling', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
dispose: () => {}
};
})
}),
Features: {
VideoEffects: undefined
}
};
});

Expand Down
Loading