fix(breakout): keep history csis for meeting members#5023
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d643c44e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
WeijuanShao
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b32a862948
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const history = new Set<number>(); | ||
| MemberUtil.extractCsis(existingMember.participant).forEach((csi) => history.add(csi)); | ||
| if (history.size > 0) { | ||
| this.historyCsisByMemberId.set(memberId, history); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier thread is that this newly added removal path overwrites any existing CSI history for the member. When a member already has cached CSIs from an earlier update and the later removedParticipantIds delta still has a current/partial CSI set, this replaces the old set with only the CSIs extracted at removal time, so findMemberByCsi can no longer resolve delayed media-layout CSIs after the member is re-added with incomplete devices; the new unit case named “merges existing history with CSIs captured at removal time” exercises this sequence as well.
Useful? React with 👍 / 👎.
| const history = new Set<number>(); | ||
| MemberUtil.extractCsis(existingMember.participant).forEach((csi) => history.add(csi)); | ||
| if (history.size > 0) { | ||
| this.historyCsisByMemberId.set(memberId, history); |
There was a problem hiding this comment.
we're overwriting the history entry here, instead we should be adding new CSIs to it
There was a problem hiding this comment.
Wouldn't this introduce redundant data? If a signed-in user joins the meeting again, the CSI generated during the first join would already be invalid. I'm not sure whether an expired CSI could later be reused by another participant.
There was a problem hiding this comment.
yes, actually, you're right, if CSIs change we shouldn't really need the old ones for anything.
| * and re-added (e.g. when entering/leaving a breakout session). | ||
| * @private | ||
| */ | ||
| private historyCsisByMemberId: Map<string, Set<number>> = new Map(); |
There was a problem hiding this comment.
suggestion: we're only using this to find a member for a specific csi, so maybe instead of having a map of sets we could just have a map where the keys are CSI and values are memberId? It would also simplify the code:
for storing it, we would just call:
historyCsisByMemberId.set(csi, member.id)
and that's it no other checks or code needed
and for reading it:
memberId=historyCsisByMemberId.get(csi)
and that's it, no for loop or anything else
what do you think?
There was a problem hiding this comment.
I considered that approach as well. Personally, I feel that using memberId as the key provides a more member-centric view of the data, which makes the overall data model clearer.
That said, I agree that using the data can be more convenient when csi is the key. If you think using csi as the key is the better approach, I'm happy to switch to that instead.
There was a problem hiding this comment.
The main downside of current approach is that we have to loop over all the entries in findMemberByCsi() - when we have a 1k meeting or in the future 5k meetings in theory we may have to be doing a loop over thousands of members and findMemberByCsi() is called whenever the person shown in one of video panes changes, so that can be quite often in a large meeting where more than 6 people are speaking (although maybe in practice not all members will have entries in historyCsisByMemberId?), so that's the main reason why I think using csi as key would be better.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 948d193e9b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
Cache history csis information for case that: host end breakouts and host return to main earlier than other panelists, there may exist a time gap between homer video layout users and locus users.
Csis are meeting-level identifiers. When a user switches between a BO session and the main session, their CSIs are not regenerated. So, to bridge this time gap, we should still be able to use the existing CSIs to match the member in
findMemberByCsi, allowing media users to resolve the correct member even while the member data in the main session is temporarily incomplete.by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.