Skip to content

[MM-63430] Calls 1-1 video support (MVP) 📹 #997

Open
streamer45 wants to merge 6 commits intomainfrom
MM-63430
Open

[MM-63430] Calls 1-1 video support (MVP) 📹 #997
streamer45 wants to merge 6 commits intomainfrom
MM-63430

Conversation

@streamer45
Copy link
Contributor

@streamer45 streamer45 commented Mar 24, 2025

Summary

PR implements initial video support in DMs. The functionality is expected to be behind a new experimental EnableVideo config setting, which defaults to false.

The only slightly more involved logic is the image segmentation that powers the background blur functionality. This required adding an external dependency (@mediapipe/tasks-vision) plus some WASM assets and a model file (see under public/). We can consider removing this from the initial MVP if we think it's too much.

UX is a real work in progress, so don't focus too much on it. It will most certainly change before we merge as we are waiting for some designs.

Screenshots

Widget view
image

Popout view
image

Screen sharing

image

User settings

image

Related PRs

mattermost/rtcd#173
mattermost/calls-common#47
mattermost/mattermost-mobile#8716

Ticket Link

https://mattermost.atlassian.net/browse/MM-63430

@streamer45 streamer45 added the Do Not Merge Should not be merged until this label is removed label Mar 24, 2025
@streamer45 streamer45 self-assigned this Mar 24, 2025
@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester 3: Security Review labels Mar 24, 2025
}
}

window.callsClient?.on('mute', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an inconsistency I found when testing video on desktop. On the web side we do this to keep the redux state in sync with the local client. This avoids the issue of a perceivable lag between pressing mute/unmute and getting the UX to react, since otherwise we'd be waiting for a server side websocket event.

Comment on lines +39 to +47
frameRate: {
ideal: 30,
},
aspectRatio: {
ideal: 4 / 3,
},
width: {
ideal: 640,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is temporary, for testing things. In the absence of user preferences, we should respect the device's default aspect ratio.

Copy link
Member

Choose a reason for hiding this comment

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

Did you get the aspect ratio fixed (from the test we did)? (is that issue actually these settings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely, although this is just about the source.

I still need to review aspect ratios in general in the UI, was waiting for some designs to understand what we want to do with them. I see a few options:

  • Clipping to cover all the available space without stretching (what we do today through object-fit: cover).
  • Show the full video content but add bands (lateral or vertical) to maintain the original ratio.
  • Dynamically modify widget dimensions (either height or width) to match the original ratio.
  • Stretch to cover the entire space (I wouldn't do this).

onToggle={noVideoInputDevices ? undefined : this.onVideoToggle}
ariaLabel={videoTooltipText}

//shortcut={} TODO: add shortcut
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBD, waiting for UX input.

Copy link
Contributor

Choose a reason for hiding this comment

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

@streamer45 is @asaadmahmood looking at this for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewbirtch I haven't explicitly asked for it as I had to park this effort for a few weeks. It would be great to get a more formal UX pass on #1008, which is the tip of this work and should include most UX improvements to match against the designs you guys provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I'll add @asaadmahmood to that PR then

Comment on lines -25 to -27
const onAnimationEnd = () => {
setAnimationEnded(true);
};
Copy link
Contributor Author

@streamer45 streamer45 Mar 24, 2025

Choose a reason for hiding this comment

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

This would never fire and leave the component mounted for the whole duration of the call, still spinning but not visible (zero opacity). I am fixing it as part of this PR.

tooltipText={videoTooltipText}
tooltipSubtext={videoTooltipSubtext}
// eslint-disable-next-line no-undefined
// shortcut={noVideoInputDevices || noVideoPermissions ? undefined : reverseKeyMappings.popout[MUTE_UNMUTE][0]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as for widget, TBD.

/>
<label htmlFor='mirror'>{formatMessage({defaultMessage: 'Mirror video'})}</label>
</CheckBoxContainer>
{!isFirefox() &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, GPU acceleration isn't working yet on Firefox (see google-ai-edge/mediapipe#5879). I tested CPU only but it's just not going to work. Anyhow, this is experimental on top of an already experimental feature.

Comment on lines +337 to +338
const theme = getTheme(store.getState());
setCallsGlobalCSSVars(theme.sidebarBg);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another small bug I found when testing on Desktop.

Copy link
Member

Choose a reason for hiding this comment

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

ah, that's why it was up above. What was the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--calls-bg CSS variable wouldn't be defined when running widget on desktop.

@streamer45 streamer45 requested a review from cpoile March 24, 2025 23:31
@streamer45 streamer45 marked this pull request as ready for review March 25, 2025 01:35
@streamer45
Copy link
Contributor Author

/cc @matthewbirtch for visibility.

@matthewbirtch
Copy link
Contributor

/cc @matthewbirtch for visibility.

@streamer45 do you want me to review this from a UX standpoint, or are you just keeping me in the loop?

@streamer45
Copy link
Contributor Author

/cc @matthewbirtch for visibility.

@streamer45 do you want me to review this from a UX standpoint, or are you just keeping me in the loop?

@matthewbirtch The latter, for the time being. Of course early feedback is always welcome, but ideally I'd be waiting to have some Figma designs ready before asking for UX review because I've mostly improvised so far :)

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

🎉 Amazing work.
Just lots of questions. 🧑‍🎓

Comment on lines +39 to +47
frameRate: {
ideal: 30,
},
aspectRatio: {
ideal: 4 / 3,
},
width: {
ideal: 640,
},
Copy link
Member

Choose a reason for hiding this comment

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

Did you get the aspect ratio fixed (from the test we did)? (is that issue actually these settings)

}}
ref={videoInputsRef}
/>
<VideoPreview
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to reuse components, or is it not worth the trouble?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the widget you mean? They serve very different purposes. Maybe a shared subcomponent but I am not sure.

Comment on lines +337 to +338
const theme = getTheme(store.getState());
setCallsGlobalCSSVars(theme.sidebarBg);
Copy link
Member

Choose a reason for hiding this comment

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

ah, that's why it was up above. What was the bug?

@streamer45 streamer45 requested a review from cpoile April 2, 2025 18:01
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester 3: Security Review Do Not Merge Should not be merged until this label is removed Lifecycle/1:stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants