Implementation of call feature streams Support #5241
Conversation
…treams that is not tied to participant
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-aodkuihnvy.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-huocwmgaga.chromatic.com/ |
cn0151
added
breaking-change
does not need changelog
Chat bundle size is not changed.
|
CallWithChat bundle size is not changed.
|
Calling bundle size is not changed.
|
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-fdigliyicb.chromatic.com/ |
cn0151
changed the title
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-mizzdumluh.chromatic.com/ |
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
cn0151
requested review from
JamesBurnside,
alkwa-msft,
dmceachernmsft,
carocao-msft,
edwardlee-msft,
PorterNan,
mgamis-msft and
prabhjot-msft
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-jvmkavwwwz.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-cexqwwtoqg.chromatic.com/ |
|
Failed to pass the 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. |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-xkiduxmroi.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-ldbczuekhf.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-nzubgjigwa.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-eujijakdvr.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-spuzcsbrdd.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-wxlnmermlm.chromatic.com/ |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-tuppniffwu.chromatic.com/ |
|
Hey @cn0151 Can you please add a changelog to this PR? if there are API changes there should always be one. |
cn0151
removed
the
does not need changelog
|
Failed to pass the 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. |
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-jbgwkmrxhv.chromatic.com/ |
| if (!stream[1].view) { | ||
| const createViewResult = await callClient.createCallFeatureView(call.id, stream[1], options); | ||
| // SDK currently only supports 1 Video media stream type | ||
| if (stream[1].mediaStreamType === 'Video') { |
There was a problem hiding this comment.
Is there a way we can tell from the Together mode feature which stream is which? I feel that we might be at the mercy here of which stream is first for the positions in the array. We need to make sure that when we are assigning the stream to the MainVideoView position that it is indeed the main video and not the screenshare view.
| @@ -278,21 +278,39 @@ export interface RaiseHandCallFeatureState { | |||
| /** | |||
| * State only version of {@link @azure/communication-calling#TogetherModeCallFeature}. {@link StatefulCallClient} will | |||
| * automatically listen for raised hands on the call and update the state exposed by {@link StatefulCallClient} accordingly. | |||
| * @alpha | |||
| * @beta | |||
There was a problem hiding this comment.
Just a nit with these tags in the UI lib we don't have a use for Alpha tags so anything not ready to go public just hit them with the @beta tag from the start
| * @beta | ||
| */ | ||
| export interface TogetherModeSeatingCoordinatesState { | ||
| // the y coordinate of the participant seating position in the together mode stream |
There was a problem hiding this comment.
Can we denote here which corner of the bounding box this will be?
There was a problem hiding this comment.
I'm guessing top left
| */ | ||
| export interface TogetherModeCallFeatureState { | ||
| /** | ||
| * Proxy of {@link @azure/communication-calling#TogetherModeCallFeature.togetherModeStream}. | ||
| */ | ||
| stream: TogetherModeStreamState[]; | ||
| streams: Map<string, TogetherModeStreamState>; |
There was a problem hiding this comment.
Is there a way that we can assign this as a object like { primaryStream: TogetherModeStreamState, secondary: TogetherModeStreamState }
If this is a map Contoso won't know what the keys are defined as when trying to access this state.
Same applies with the seatingCoordinates as well.
There was a problem hiding this comment.
Yes, like we discussed on previously Chuk, having an object is much more simple for contoso to tell what is available and what is not. Especially if we are keeping this under the TogeterModeCallFeatureState
|
|
||
| /* @conditional-compile-remove(together-mode) */ | ||
| // This function is used to create a view for a stream that is part of a call feature. | ||
| async function createCallFeatureViewVideo( |
There was a problem hiding this comment.
Since this is all new for the call feature, I think we can move this out to its own file. We should also write a dispose for this so we can properly clean up the stream when the user changes the gallery view or together mode stops.
| @@ -457,11 +459,25 @@ export class CallContext { | |||
| } | |||
|
|
|||
| /* @conditional-compile-remove(together-mode) */ | |||
| public setTogetherModeVideoStream(callId: string, addedStream: TogetherModeVideoStream[]): void { | |||
| public setTogetherModeVideoStream(callId: string, addedStreams: TogetherModeVideoStream[]): void { | |||
There was a problem hiding this comment.
Again, can we have this an an object for the TogetherModeVideoStream instead of an array?
What
The PR contains the implementation to render streams that are not tied to a participant or device Manager. it also contains seat map updates to together mode stream state.
Why
This resolves the issue where we want to render a call feature stream that is not tied to a participant
How Tested
Process & policy checklist
Is this a breaking change?