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

Update rust ffi #217

Merged
merged 24 commits into from
Jul 23, 2024
Merged

Update rust ffi #217

merged 24 commits into from
Jul 23, 2024

Conversation

keepingitneil
Copy link
Contributor

No description provided.

@keepingitneil keepingitneil marked this pull request as ready for review July 12, 2024 17:21
@keepingitneil keepingitneil marked this pull request as draft July 12, 2024 18:08
@keepingitneil keepingitneil marked this pull request as ready for review July 16, 2024 23:41
self.emit("track_published", rpublication, rparticipant)
elif which == "track_unpublished":
rparticipant = self.participants[event.track_unpublished.participant_sid]
rpublication = rparticipant.tracks.pop(
rparticipant = self.remote_participants[
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we should always use .get() on these to guard against the object not being present. if participant is removed, it'd be better to handle the None responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theomonnom is it possible for track_unpublished (and friends) to come after the participant has been disconnected. I'm not super familiar with the async rust stuff and what guarantees we have for ffi events.

@@ -88,13 +91,14 @@ def __init__(
) -> None:
super().__init__(owned_info)
self._room_queue = room_queue
self.tracks: dict[str, LocalTrackPublication] = {} # type: ignore
self.track_publications: dict[str, LocalTrackPublication] = {} # type: ignore

async def publish_data(
self,
payload: Union[bytes, str],
Copy link
Member

Choose a reason for hiding this comment

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

should payload be positional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it positional because it's the only value with no default so it's really convenient

Copy link
Member

@theomonnom theomonnom left a comment

Choose a reason for hiding this comment

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

awesome lgtm! 🔥

@keepingitneil keepingitneil merged commit 821d4bf into main Jul 23, 2024
15 checks passed
@keepingitneil keepingitneil deleted the neil/ffi branch July 23, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants