Fix avatars in federated calls #12863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danxuliu
added
2. developing
bug
feature: call 📹
14 tasks
When the Nextcloud server notifies the signaling server about updates in the participants it provides a list of properties about each participant that will be relayed to the clients in the "participants->update" signaling messages. When federated users were added to the participants whose properties were relayed the "userId", which until then was set only for regular users, was set too to the actor id for federated participants. However, this approach was lacking, as it was not possible for clients to really know from the "userId" property if the participant was federated or not or, in a more general way, its actor type. Due to that the "userId" is again set only for regular users for backwards compatibility, and the "actorType" and "actorId" properties are added to the data of each participant included in the "participants->update" signaling message. For consistency, "actorType" and "actorId" is also set in the equivalent message of the internal signaling server. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The actor type and id are now set in "CallParticipantModel" when a participant joins the call, similarly to how it was done for the user ID. Actor type and id now take precedence over the user ID in the "VideoView" component, so "AvatarWrapper" now uses the correct source for participant avatars (as long as the actor type and id are correct, which in federated conversations require a conversion from the values sent by the host server and the values used by the federated server). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
fix-avatars-in-federated-calls
branch
from
3c72fcf
to
4203f4d
Compare
| @@ -293,6 +293,7 @@ function usersChanged(signaling, newUsers, disconnectedSessionIds) { | |||
| webRtc: webrtc, | |||
| }) | |||
| } | |||
| callParticipantModel.setActor(user.actorType, user.actorId) | |||
There was a problem hiding this comment.
desktop client goes 💥 when speaking to a 19 server?
There was a problem hiding this comment.
The attributes would be simply undefined, which would not change the previous behaviour nor break anything, no?
There was a problem hiding this comment.
ah, no PHP style Undefined key X error. fine by me then
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
fix-avatars-in-federated-calls
branch
from
d570cd5
to
0264d3d
Compare
Merged
|
The conversion between Therefore while avatars are still broken in the current state this pull request prepares the WebUI to correctly shown them once the signaling server is updated :-) |
nickvergessen
approved these changes
Aug 1, 2024
fancycode
added a commit
to strukturag/nextcloud-spreed-signaling
that referenced
this pull request
Aug 5, 2024
|
Tested and works 👍 Thanks! |
fancycode
added a commit
to strukturag/nextcloud-spreed-signaling
that referenced
this pull request
Aug 6, 2024
fancycode
added a commit
to strukturag/nextcloud-spreed-signaling
that referenced
this pull request
Aug 6, 2024
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #12668
Although the
userIdproperty in theparticipants->updatesignaling message, which is used by clients to know the participants in a call, was set to the cloud ID for federated participants this was not enough to identify a federated user, as it was not possible to know for sure the actor type only based on the user ID (as a local user could have a user ID that resembled a cloud ID, so looking for @ remote would not be reliable).Due to that now the
participants->updatesignaling messages explicitly provide theactorTypeandactorIdof each participant in the message (anduserIdis again provided only for regular users like it was always done). Knowing the actor type and id clients can now request the avatar from the right endpoint and with the right data.In the WebUI now
actorTypeandactorIdtake precedence overuserIdwhen identifying a participant inVideoView, which automatically causesAvatarWrapperto use the right endpoint (once the type and id are converted, see below).Converting actor type and id
Conversion of
actorTypeandactorIdinparticipants->updatesignaling messages will be automatically done by the external signaling server, so clients just need to use the received values as is (so the client of a federated user will seeactorType: usersfor users in its own server andactorType: federated_usersfor users in the host server, in both cases withactorIdadjusted as needed, just like when getting the participants from the API). However, this might become relevant again once/if federated calls are implemented in the internal signaling server, so leaving the section below for reference.Note, however, that in order to get the avatars only converting from local users to federated users is strictly needed. Avatars can be got using two different API endpoints, one that accepts a user ID and another one that accepts a cloud ID; in the first case only avatars for local users are returned, while in the second case avatars are returned for both local and remote users. Therefore, if a federated user is not converted to a local user the proxy endpoint will be used, but it will correctly return the avatar for the local user. Nevertheless it might be better to do the conversion anyway for any other future use of the actor type and id.
How to test
actorTypeandactorIdinparticipants->updatesignaling messagesResult with this pull request
The right avatars are shown for all users
Result without this pull request
Broken avatars are shown for all users (except oneself)