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

Proposal: Allow multiple (and conditional) stream readers per topic #1427

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Mar 4, 2025

For built-in text/byte streams that we'd like to use in agents we are having trouble keeping things simple while also adding flexibility to layer nice abstractions on top on the client side. The tension mostly comes down to the client-side constraint that you can have only one handler per topic. This means that if we want to add an abstraction that uses the lk.transcription topic, it needs to be all or nothing. and leads to confusion in SDKs where the abstraction is available and the topic should not be used manually or else things will go wrong, and others where the abstraction is unavailable and the topic must be used manually.

I propose that we make it possible to register multiple handlers per topic. Not because people should commonly do this, but because it allows flexibility without running into hard error conditions.

In order to make this work reliably, if you register a handler you must consume the stream you are given. This is of course tricky so we will also allow a filter method: you can use this to decide based on the header packet whether or not you will handle the stream.

In this way its easy to add a handler that is for lk.transcription but only for participant X or track Y. And we can further extend this by adding convenience e.g. participant.registerTranscriptionHandler(...) that applies the correct topic and filters for you. and we could abstract further by having default handling of transcription streams for each participant that are then broadcast a different way.

This spike adds basic support and modifies the demo to prove that it works, although I'll probably back out the demo changes before merging (if we merge).

As an alternative we could also keep the filter methods but require that not more than one handler pass the filter per incoming stream. if more than one topic+filter applies to a stream, we'd give it only to the first one (order undefined?) and log an error. But I think that's not preferable.

Feedback very welcome.

@bcherry bcherry requested review from longcw, lukasIO and theomonnom March 4, 2025 06:59
Copy link

changeset-bot bot commented Mar 4, 2025

⚠️ No Changeset found

Latest commit: 0795cab

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 4, 2025

size-limit report 📦

Path Size
dist/livekit-client.esm.mjs 90.62 KB (+0.28% 🔺)
dist/livekit-client.umd.js 98.23 KB (+0.22% 🔺)

@ladvoc
Copy link

ladvoc commented Mar 5, 2025

Overall, I think this proposal looks solid. I have one suggestion regarding handler registration: instead of returning a cleanup function, it might be better to return an opaque registration object that can be used to unregister the handler on the room:

const registration = room.registerTextStreamHandler('some-topic', someHandler);
room.unregisterTextStreamHandler(registration);

The current approach presents implementation challenges in other languages (specifically Swift and Rust) which would make this API inconsistent across languages.

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.

2 participants