Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(participants): update participants session status from signaling #12556

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jun 21, 2024

☑️ Resolves

  • Attempt to reduce the number of participants list updates

Important

Stores all sessions in signaling store 🍍:

  • internal: by nextcloud sessionId
  • external: by signaling server sessionId
    If no attendee was mapped to the session, fetch participants and patch session later

Update participant object with data from signaling:

  • permissions
  • nextcloud sessionId
  • inCall
  • participantType
  • lastPing
  • displayName

Warning

  • CI is skipped;
  • debounce intervals from previous signal signaling-participant-list-changed were extended to 30 seconds to reduce total amount
Examples of signaling messages inside:

INTERNAL JOINED

[
    [
        {
            "userId": "alice",
            "roomId": 7,
            "lastPing": 1718882327,
            "sessionId": "4eQPZvqBSnQ1AUb6DgO50JUgneyhx7K31c0qNZ7+M3gHEeU365NU0/1+jGueeO5oEboyJNG0cCA7xXkWxrK9dQzbNuY2ZjXdYhePoyh4j1M3h2F78DUocHvKF/I+Q56PNBT8QZts+XaswE2qW2gkm7pgr+3yD3+qaEKOZtptuE+m1JaKP7E4LGoE7ES8aD4GV+tKIgONhcurl1PNPzgMWj7NmWskJ44P/ZR3K0EYCoERCF6DLLcDXvldy4S4qqj",
            "inCall": 0,
            "participantPermissions": 246
        },
        {
            "userId": "antreesy", // Current user
            "roomId": 7,
            "lastPing": 1718882332,
            "sessionId": "0uf9N3nvaWKxz1Gn4mG5oHqnKu4N8XabhWHOSv+kmuPt8RFyK9tEk82+itf+iGflPyPzfeZr5SObW5PXDbXOO0y/wjt3O/xgoJrfI3qK7+7xmtz0WYZj0zdYaQXsv9jsEDpU6/QPXXMEoD9r+Alv4GlrzPwi0IMVo/Z4wtes2OM/jBXRhAU0AM7uwMlLfXS5ztA9QZRk8nT8sooBTiD0/D4y1rTkiXRBiX21YHNHVwVQfs38vOPnqxnNATX1Byh",
            "inCall": 0,
            "participantPermissions": 254
        }
    ]
]

INTERNAL LEFT (alice)

[
    [
        {
            "userId": "antreesy", // Current user
            "roomId": 7,
            "lastPing": 1718882333,
            "sessionId": "0uf9N3nvaWKxz1Gn4mG5oHqnKu4N8XabhWHOSv+kmuPt8RFyK9tEk82+itf+iGflPyPzfeZr5SObW5PXDbXOO0y/wjt3O/xgoJrfI3qK7+7xmtz0WYZj0zdYaQXsv9jsEDpU6/QPXXMEoD9r+Alv4GlrzPwi0IMVo/Z4wtes2OM/jBXRhAU0AM7uwMlLfXS5ztA9QZRk8nT8sooBTiD0/D4y1rTkiXRBiX21YHNHVwVQfs38vOPnqxnNATX1Byh",
            "inCall": 0,
            "participantPermissions": 254
        }
    ]
]

EXTERNAL JOINED

[
    [
        {
            "sessionid": "Li3jtK-0B4FW3m4eJDLajKQ7asilwKlxnUzc91Mnlrt8MVFnWnJQNlFpSFRvbkwxWGxoc3NxS19JMjNwUl9fLWNVb1FTSER4eFFnaHFFbUkyZFFWNWVzVFFRMGlMX003cVdaeWNGWXhDbWxlelR0UE5pZG1iTkcwZVdkVUJoZ2o4ZDRva2I5VERNSlcycW5iazFyMnVlX3JndkJkT1FKYzI2MGpIcHV4aUlhQWRHcGVVNWxYX2NTc0ZFMExvbGNJMFRWU19rU3I2fDI4OTE4ODgxNzE=",
            "userid": "bob",
            "user": {
                "displayname": "Bob Odenkirk"
            },
            "roomsessionid": "RnObY28zWT6p0tF84Jwlm0Xr8miMdUaCN0XBiqPuWdATkeVPxo7G7OFuIFkYft67vhDPXBpgeKRN4VUvvP8CTnNDRcsb9Xi+GDmFcV0jUxsIfZp/5PQuHpaZuWh7udfnmMc8nwC8rQWEDuH14G2OAvyJNtt5jQXeu08vt+h/gHBMboaETsEYgZZwNSSjmZHY5i/MvFu6gYHTLwojtrxIZp0iiyQmSDwPv8yjOLNxLV/eBRIySeeQzfFnDOgOMjl" // Nextcloud sessionId
        },
        {
            "sessionid": "hymgxogGWSihSgwbrtU58KVvZE073Hn0wSZM8mQDz2J8UzRMdl9IMVZDRVU2WXJpZDFCTm9SNENIMFhyNlNfM1Q3R0QxekkwLXNuMkNRenRCMDZ3ZE9Nb3EwVk5vaW1xOVdXem96Zk1DektJMmdJV2dLLWRwVW9YWjZTNE1pN2ZGT2RHeEdUVXFDa2RjRF9LZlNNQ3lHN3VuUlFXZl9qd2ZJNnI0a3VvZllWVWNYWDUzSVZxRGd4ZW1saTFEOTVDUWpXMV9QQU1RfDUyODA4ODgxNzE=",
            "userid": "alice",
            "user": {
                "displayname": "Alice Adair"
            },
            "roomsessionid": "ZljR8uqzJQYbi66LcM2z0sVFkNRtraNxQ8IfkDB0HIw1CB+lmeL8KXBO2OAoKOIoPW/enIsmsCs2fQPLP4g5rI8olyUrL3wv9bKMEJ1IdVjtaUbMXuA0GNqfzbxwr2+JfukhSecmjB7Rnf8OZGN76BXZtuXd90UKCOepoia4jXNZtg7ca4lZMpNZlWmMUueZd6IWMVvO/0Xoy4Nu/ePwTtB31DhCyIwfkwqHE6/3RlD6NHAeHKYwRv96R8IuWdN" // Nextcloud sessionId
        }
    ]
]

EXTERNAL LEFT (bob)

[
    [
        "Li3jtK-0B4FW3m4eJDLajKQ7asilwKlxnUzc91Mnlrt8MVFnWnJQNlFpSFRvbkwxWGxoc3NxS19JMjNwUl9fLWNVb1FTSER4eFFnaHFFbUkyZFFWNWVzVFFRMGlMX003cVdaeWNGWXhDbWxlelR0UE5pZG1iTkcwZVdkVUJoZ2o4ZDRva2I5VERNSlcycW5iazFyMnVlX3JndkJkT1FKYzI2MGpIcHV4aUlhQWRHcGVVNWxYX2NTc0ZFMExvbGNJMFRWU19rU3I2fDI4OTE4ODgxNzE="
    ]
]

EXTERNAL CHANGED (alice joined call)

[
    [
        {
            "lastPing": 1718962151,
            "sessionId": "cuFDIw8ygQQ29NMcr49-6WHTW7MZ4BCSdr16Vd6uvop8aXRYREVOYko2Mzg5LTBZM0NjSVB5REV0OEE2UXpMaEFxeklMVldEMEZyOGYwTHJMcmlfYjUwVlF4OXlrMU5iTEJlR0hvR1V4a2puZFdFeGlpcmhQZ1Q4R2JzdXFNbTNqMEIxTlF3QnNJWG9seHBvVFhTLTNqM0s1UUh3ekRrcWpTck9xaU9BWTUxY3dBcncxLW9Od1lhX09lME1vNWVuZmJzZExJY2lBfDI0MTI2OTgxNzE=",
            "nextcloudSessionId": "p8XiddHgLezUngrcg7GdkkTvrb+9kXzMNB2hd26YdImC5OmrTu3pPsZQ2atCvNEffYdMqyRTWuYt0OdIeZvFqGFztWNEveN5LJkXhlWiBElEedr6xZdJlDjwwnQn9O+pO3J8xivDyHrmvjDM+IwlLp/8etHLMxmLXauNHHfN1RlRXo/tlBu2AWGxqnFl54xJM8ip+4reyZckt5zQXucB3odU4x3VRP25G2NLkNDd0+42gwocfBPcxmO65Cr2IxX",
            "participantType": 3,
            "participantPermissions": 246,
            "userId": "alice",
            "inCall": 7
        }
    ]
]

EXTERNAL END CALL FOR ALL

undefined

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Screencast.from.21.06.2024.13.04.46.webm

🚧 Tasks

  • Internal signaling
  • External signaling
  • Reduce list updates
  • Cache participants

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@Antreesy Antreesy added this to the 💙 Next Major (30) milestone Jun 21, 2024
@Antreesy Antreesy self-assigned this Jun 21, 2024
@Antreesy Antreesy force-pushed the perf/noid/update-participants-from-signaling branch from 1220066 to 3cf9b0c Compare June 21, 2024 11:08
const updateUsersFromInternalSignaling = async ([participants]) => {
const hasUnknownSessions = await store.dispatch('updateParticipantsFromInternalSignaling', { token: token.value, participants })
if (hasUnknownSessions) {
debounceUpdateParticipants()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RFC: Maybe we need to update immediately here?

Should happen in two cases:

  • we don't have a list yet, so we already fetching it, (then request will be cancelled);
  • a new participant (guest) joined and not in the list yet (would be a disaster for webinars, where new people are joining every time)

Copy link
Contributor Author

@Antreesy Antreesy Jun 24, 2024

Choose a reason for hiding this comment

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

Kept as-is, so will be updated in 3-15s

  • add trigger to cancel this request (if everyone was found in very first server response, but data wasn't there yet)
  • OR
  • Do not make an immediate request, get everyone as unknown and trigger debounce interval (will be anyway for conversation, just not provoke another one)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to keep it as-is.

As we know from signaling message what attendees are joining, what about create a temporary participant with the info available and data will be completed once fetched ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have enough information to show a participant from signaling until fetch. And we got all the actual info from it, there's no need to complete it later.
Though we can consider cache participants list 🤔

@Antreesy Antreesy force-pushed the perf/noid/update-participants-from-signaling branch from 3cf9b0c to 0c6efa3 Compare June 24, 2024 13:51
src/stores/signaling.ts Outdated Show resolved Hide resolved
src/stores/signaling.ts Outdated Show resolved Hide resolved
src/stores/signaling.ts Outdated Show resolved Hide resolved
src/stores/signaling.ts Outdated Show resolved Hide resolved
src/stores/signaling.ts Show resolved Hide resolved
src/stores/signaling.ts Outdated Show resolved Hide resolved
src/composables/useGetParticipants.js Outdated Show resolved Hide resolved
src/stores/signaling.ts Outdated Show resolved Hide resolved
const updateUsersFromInternalSignaling = async ([participants]) => {
const hasUnknownSessions = await store.dispatch('updateParticipantsFromInternalSignaling', { token: token.value, participants })
if (hasUnknownSessions) {
debounceUpdateParticipants()
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to keep it as-is.

As we know from signaling message what attendees are joining, what about create a temporary participant with the info available and data will be completed once fetched ?

…es from participants update

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…nternal signaling

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…tandalone signaling

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…signaling

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
[skip ci]

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the perf/noid/update-participants-from-signaling branch from 0c6efa3 to 53727a7 Compare June 25, 2024 12:45
@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 25, 2024

Call stats:

  • Duration: 47 minutes;
  • Users count: 83;
  • Amount of calls: 6020 (72 per user);
  • Amount with this PR + Talk desktop: 32 requests.
    image

if (!attendeeUsersToUpdate[attendeeId]) {
attendeeUsersToUpdate[attendeeId] = { sessionIds: [...attendee.sessionIds] }
}
if (participant.user.displayname) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (participant.user.displayname) {
if (participant.user?.displayname) {

index.js:2 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'displayname')

const { token, attendeeId, sessionId } = session
const attendee = store.getters.getParticipant(token, attendeeId)
const updatedData : { sessionIds: [], inCall?: number } = {
sessionIds: attendee.sessionIds.filter((id: string) => id !== sessionId)
Copy link
Contributor Author

@Antreesy Antreesy Jun 27, 2024

Choose a reason for hiding this comment

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

When actor leaves conversation

Suggested change
sessionIds: attendee.sessionIds.filter((id: string) => id !== sessionId)
sessionIds: attendee?.sessionIds.filter((id: string) => id !== sessionId)

index.js:2 Uncaught TypeError: Cannot read properties of null (reading 'sessionIds')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants