[MM-57019] Calls: Live captions support for mobile #7854

cpoile · 2024-03-05T22:15:53Z

Summary

Adds support for live captions on mobile
Reducing size of the avatars to 72px (as in the designs) when cc is turned on, gave a 48px space for captions, and captions overflow upwards to cover the bottom row.
Was able to add the Live captions turned on ephemeral message when switching captions on -- it lasts 2 seconds, and won't show if there are already captions showing.
Works in landscape
@enahum I added the voip UIBackgroundMode, as recommended by the react-native-webrtc folks -- I thought we already had it, but I guess we didn't, so adding it here. Let me know if I need to do anything else or add documentation.
@matthewbirtch I will DM you some info on how to use the calls load testing with voice to test this -- there were a few little assumptions I had to make, so it would be useful if you could play around with it to make sure everything looks good. Totally fine making adjustments.

Ticket Link

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
[ x Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.

Device Information

Android: 13, Pixel 6
Android: 13, Galaxy Tab s7+
iOS: 16.5.1, iPhone 14

Screenshots

Release Note

Calls: Added support for live captions

app/products/calls/components/captions.tsx

app/products/calls/screens/call_screen/call_screen.tsx

cpoile · 2024-03-06T15:10:06Z

@enahum Thanks for the comments! One question -- I added new i18n message ids, but npm run i18n-extract did not seem to create anything new in the i18n files. Has the system been changed recently?

enahum

As for the extract of i18n nothing has change as far as I'm aware

app/products/calls/components/captions.tsx

enahum

I18n extract is the one thing pending

cpoile · 2024-03-06T18:51:47Z

@enahum It looks like there's something wrong with mmjstool. It does seem to be picking up a diff in the ci test, but it's not failing the check. This seems to have been happening for awhile (there are some earlier i18n strings as well as the ones from the current PR). I've tried to debug by bisecting the commits to mattermost-utilities, but I can't get any of the earlier commits to work either, so I'm kind of stuck at the moment.

streamer45

Great work! Left a couple of minor comments.

app/products/calls/screens/call_screen/call_screen.tsx

streamer45 · 2024-03-06T21:38:13Z

app/products/calls/screens/call_screen/call_screen.tsx

@@ -472,6 +484,7 @@ const CallScreen = ({
    const waitingForRecording = Boolean(currentCall?.recState?.init_at && !currentCall.recState.start_at && !currentCall.recState.end_at && isHost);
    const showStartRecording = isHost && EnableRecordings && !(waitingForRecording || recording);
    const showStopRecording = isHost && EnableRecordings && (waitingForRecording || recording);
+    const ccAvailable = Boolean(EnableRecordings && EnableTranscriptions && EnableLiveCaptions && recording);


More of a comment than a request but I feel this is a lot and if we ever decoupled the job it will essentially break. Wondering if we should instead have a single server side boolean to inform whether a live caption is available for the current call.

Yah, could be another way to go. Of course it wouldn't simplify the logic, it would just move the logic to the backend--but then we wouldn't need to change the client in that future case. Is it on the horizon that we could have captions without transcriptions, and/or transcriptions without recordings?

Is it on the horizon that we could have captions without transcriptions, and/or transcriptions without recordings?

I am not sure to be honest but logic on the backend is advantageous because it can also be dynamic. For example, this boolean could go in the state call itself and potentially be turned to false in case of issues with live captions (e.g. due to performance) as we discussed.

The changes are made in mattermost/mattermost-plugin-calls#633 now, so adding them here.

streamer45 · 2024-03-06T21:40:52Z

app/products/calls/components/captions.tsx

+                            style={styles.text}
+                            numberOfLines={0}
+                        >
+                            {`(${displayUsername(sessionsDict[cap.sessionId]?.userModel, intl.locale, teammateNameDisplay)}) ${cap.text}`}


Again more of a question for my own understanding. Is there any downside in using the whole sessions dictionary in terms of re-renders? After all what we really need is just the participant's id, not even the session as technically the user id would be sufficient right?

Not sure what you mean here about 'all what we really need is just the participant's id' -- it's the userModel that we need. If that's what you meant, then I guess we could create a userModels dict in the parent component and only pass that in, but that wouldn't save any renders because it would be a new object also on every parent render, and we would then be creating another new object that would need to be collected later.

My point here is that the sessions and participants state is data that can mutate quite often whereas the users (participants) models would only change when joining/leaving. So I was wondering whether there could be value in making this component depend on the latter.

Normally yes, that would be the right thing to do. But the way that the userModels are stored, we would need to extract it out on any change anyway. That would potentially be a good optimization to do (make an independent useModel dict), so I'll put that on my list. On the other hand, the amount of speedup in this case would be very little I imagine, given how it's only a few lines of captions. But the principle is good for maybe the participants grid or something more heavy.

enahum · 2024-03-07T00:11:34Z

@cpoile I ran i18n-extract on my machine and it worked just fine.. so I guess there is something in your machine preventing it from running correctly.

I took the liberty to push the commit the with changes.

cpoile · 2024-03-07T01:05:45Z

@enahum Thank you, I appreciate it. I don't know why it's not working for me.
But there might be something misconfigured with CI also, because it wasn't failing after detecting the diff.

enahum · 2024-03-07T01:07:42Z

@mvitale1989 can you check why the CI is not failing when detecting the diff? thanks my friend.

mvitale1989 · 2024-03-07T16:20:35Z

afaiu it seems to be an issue in the scripts/precommit/i18n.sh script that has always been there:

The mjstool simply changes the files it needs to. Exit status is 0 regardless of whether files are changed or not.
This line specifically seems to have been put there as an assertion, to check if any lines were changed. It indeed returns exit status 1 if there were differences. But this doesn't terminate the script.

Given the above, I suppose the intention was for the script to fail if there were any changes, as Christopher expected. But the script has been missing the set -e to behave like that. I'll add it in a separate PR

matthewbirtch

This is great stuff @cpoile. Awesome job. From my tests I don't have any changes. The load testing worked like a charm!

…57019-live-captions

streamer45

Great work, thanks for bearing with me on the feedback.

* mobile support for live captions * add the 'Live captions turned on' ephemeral notice * PR comments * message should be translatable * run i18n-extract * call_recording_state -> call_job_state; ccAvailable is now dynamic * backwards compatibility with pre calls v0.26.0 * correct mobile version in deprecation comments --------- Co-authored-by: Elias Nahum <[email protected]>

cpoile added 2 commits March 5, 2024 14:58

mobile support for live captions

0b36822

add the 'Live captions turned on' ephemeral notice

2f3ac45

cpoile added 2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer labels Mar 5, 2024

cpoile requested review from streamer45, matthewbirtch and enahum March 5, 2024 22:15

mm-cloud-bot added release-note labels Mar 5, 2024

enahum requested changes Mar 6, 2024

View reviewed changes

PR comments

30bfb31

cpoile requested a review from enahum March 6, 2024 15:10

enahum requested changes Mar 6, 2024

View reviewed changes

app/products/calls/components/captions.tsx Outdated Show resolved Hide resolved

message should be translatable

7dfede4

cpoile requested a review from enahum March 6, 2024 16:00

enahum approved these changes Mar 6, 2024

View reviewed changes

matthewbirtch added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 6, 2024

streamer45 reviewed Mar 6, 2024

View reviewed changes

run i18n-extract

2c3d73f

mvitale1989 mentioned this pull request Mar 7, 2024

CLD-7307 Add set -e to i18n checker script #7857

Merged

matthewbirtch approved these changes Mar 8, 2024

View reviewed changes

matthewbirtch removed the 2: UX Review Requires review by a UX Designer label Mar 8, 2024

cpoile added 2 commits March 14, 2024 16:48

call_recording_state -> call_job_state; ccAvailable is now dynamic

ad008c7

Merge remote-tracking branch 'origin/MM-57019-live-captions' into MM-…

0336a93

…57019-live-captions

cpoile requested a review from streamer45 March 14, 2024 20:49

cpoile added 2 commits March 14, 2024 20:01

backwards compatibility with pre calls v0.26.0

dde966d

correct mobile version in deprecation comments

e0f0e68

streamer45 approved these changes Mar 18, 2024

View reviewed changes

streamer45 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 18, 2024

cpoile merged commit 4e68662 into main Mar 20, 2024
7 checks passed

cpoile deleted the MM-57019-live-captions branch March 20, 2024 13:10

amyblais added this to the v2.16.0 milestone Mar 20, 2024

amyblais added the Docs/Needed Requires documentation label Apr 10, 2024

cwarnermm added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-57019] Calls: Live captions support for mobile #7854

[MM-57019] Calls: Live captions support for mobile #7854

cpoile commented Mar 5, 2024

cpoile commented Mar 6, 2024

enahum left a comment

enahum left a comment

cpoile commented Mar 6, 2024

streamer45 left a comment

streamer45 Mar 6, 2024

cpoile Mar 8, 2024

streamer45 Mar 8, 2024

cpoile Mar 14, 2024

streamer45 Mar 6, 2024

cpoile Mar 8, 2024

streamer45 Mar 8, 2024

cpoile Mar 14, 2024

enahum commented Mar 7, 2024

cpoile commented Mar 7, 2024

enahum commented Mar 7, 2024

mvitale1989 commented Mar 7, 2024

matthewbirtch left a comment

streamer45 left a comment

[MM-57019] Calls: Live captions support for mobile #7854

[MM-57019] Calls: Live captions support for mobile #7854

Conversation

cpoile commented Mar 5, 2024

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

cpoile commented Mar 6, 2024

enahum left a comment

Choose a reason for hiding this comment

enahum left a comment

Choose a reason for hiding this comment

cpoile commented Mar 6, 2024

streamer45 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum commented Mar 7, 2024

cpoile commented Mar 7, 2024

enahum commented Mar 7, 2024

mvitale1989 commented Mar 7, 2024

matthewbirtch left a comment

Choose a reason for hiding this comment

streamer45 left a comment

Choose a reason for hiding this comment