Port data streams over to rust livekit-ffi version#697
Conversation
🦋 Changeset detectedLatest commit: df58880 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| async sendText( | ||
| text: string, | ||
| options?: { | ||
| topic?: string; | ||
| attributes?: Record<string, string>; | ||
| destinationIdentities?: Array<string>; | ||
| streamId?: string; | ||
| }, | ||
| ) { | ||
| const writer = await this.streamText(options); | ||
| await writer.write(text); | ||
| await writer.close(); | ||
| return writer.info; | ||
| const req = new StreamSendTextRequest({ | ||
| localParticipantHandle: this.ffi_handle.handle, |
There was a problem hiding this comment.
Note that I have not added any additional tests (unit or e2e) tests for these updates. IMO it's not really possible to adapt the web or rust data streams unit test suites since the "units" that are being tested aren't exposed over livekit-ffi.
I could add some more data stream specific e2e tests but the rust implementation already has them and it's unclear how much more value they would provide over the existing e2e data streams test given that really all the test would be exercising at that point is livekit-ffi. Maybe that's a good enough reason though, idk.
If anybody has a strong opinion on this, please surface it here.
…m and not dropped Previously they were just being ignored, which meant that a "data stream too big" error (probably along with others too) would not get surfaced.
1 participant
Warning
The tests will not pass until a new
livekit-ffibuild is made incorporating the data stream v2 updates and the version is upgraded in here.Previously, the node-sdk had its own data streams implementation. My understanding is there isn't really a good reason for this, and it's more of an artifact of history - the node/python sdks had data streams added first, and the rust implementation came later on.
With data streams v2, the rust implementation will both be quite a bit more stable and gain some new features that make it significantly more performant (single packet data streams and DEFLATE compression when it makes the payload smaller). This roughly doubles data stream throughput in local testing.
So, port the node sdk to use the rust sdk data streams v2 implementation, and completely remove the typescript implementation. This is being done via the already-existing
livekit-ffidata streams messages which the C++ and unity sdks use today.New behaviors worth being aware of
All of these are either data streams v2 related changes, or bug fixes.
1.
compressoptionWhen sending a data stream, there is a new
compressoption. Just like how this works on web / rust, this defaults totrue. is set tofalse, then compression will be disabled (useful if you know the data you are sending isn't compressible / you are doing your own compression, which is not uncommon in robotics use cases). For 95% of users, they should leave this set totrue.2. Max data stream size
In a rust data streams v2 pull request review comment, we decided that for security reasons it made sense to introduce a maximum data stream size as a DOS protection. This limit is by default
5gb- any data stream that is larger will now read up until that point, and if the stream keeps going, a "payload too large" error will be raised on the stream and exposed to a user on the subsequent.read()call.If a user is sending a large file, they can override this by setting a new
maxPayloadByteLengthoption on theroom.connectcall:3. Throwing data stream errors rather than exclusively logging them
The old node specific data streams implementation didn't properly surface errors to the caller which were encountered while reading the stream. As a prerequisite for exposing the error for item 2, I fixed this, but it means that now if a data stream were to fail, errors would get thrown when in the past they would get logged and dropped.
Testing
I've tested this as best as I can locally but I'd really like some help from agents folks to really put this through the ringer and make sure I haven't inadvertently broken something.
Todo