-
Notifications
You must be signed in to change notification settings - Fork 132
Expose participant's permission to ffi layer #824
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
base: main
Are you sure you want to change the base?
Conversation
davidzhao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! just a couple of nits and I think we can merge this one.
|
|
||
| message ParticipantPermissionChanged { | ||
| required string participant_identity = 1; | ||
| optional ParticipantPermission permission = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be optional? seems like it should always be there
| }, | ||
| ParticipantPermissionChanged { | ||
| participant: Participant, | ||
| permission: Option<proto::ParticipantPermission>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same concern here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same concerns from the beginning and have been struggling with this decision.
The root cause is that the protocol layer uses proto3 syntax, which makes the generated nested structs optional by default. Even though we know in practice it will never be empty (and we can get the semantic default value even if it's empty in the proto3 world), after careful consideration, I decided to make the permission field optional in the ParticipantInfo struct in the FFI layer to avoid potential breaking changes.
I completely agree that it should be required in the ParticipantPermissionChanged event. However, I initially thought it might be better to keep consistency with the ParticipantInfo struct. I'm happy to change it to required if you think that's the better approach.
BTW, I made a new commit to initialize the permission field(It should be discovered earlier if choose required instead, LOL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR exposes participant permissions to the FFI (Foreign Function Interface) layer to fix issue #823. The change enables FFI consumers to access and monitor permission changes for both local and remote participants.
Key changes:
- Added
permissionfield toParticipantInfostruct and exposed it via a public getter method - Implemented
ParticipantPermissionChangedevent that fires when participant permissions are updated - Added FFI protocol definitions and conversion logic to bridge Rust types to the FFI layer
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| livekit/src/room/participant/mod.rs | Added permission field to ParticipantInfo, permission change event handler, and update logic to detect and dispatch permission changes |
| livekit/src/room/participant/remote_participant.rs | Added permission getter method and permission change event handler for remote participants |
| livekit/src/room/participant/local_participant.rs | Added permission getter method and permission change event handler for local participants |
| livekit/src/room/mod.rs | Added ParticipantPermissionChanged room event and registered event handlers for both local and remote participants |
| livekit-ffi/src/server/room.rs | Added event forwarding logic to convert Rust ParticipantPermissionChanged events to FFI protocol messages |
| livekit-ffi/src/conversion/participant.rs | Implemented conversion from livekit_proto::ParticipantPermission to FFI proto::ParticipantPermission |
| livekit-ffi/protocol/participant.proto | Defined ParticipantPermission message structure with proper field mappings and added import for TrackSource enum |
| livekit-ffi/protocol/room.proto | Added ParticipantPermissionChanged message to RoomEvent oneof |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fix for ISSUE-823