Skip to content

Data streams #593

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

Merged
merged 100 commits into from
Mar 7, 2025
Merged

Data streams #593

merged 100 commits into from
Mar 7, 2025

Conversation

ladvoc
Copy link
Contributor

@ladvoc ladvoc commented Feb 18, 2025

Summary of changes

  • Data streams support: full support for text and byte data streams.
  • Data channel buffering: outgoing data packets are now buffered and sent asynchronously.

Differences from other SDKs

  • Error reporting: most data streams methods are throwing, allowing the user to handle errors.
    • Open streams are coordinated with IncomingStreamManager and OutgoingStreamManager to allow detection of errors (e.g. duplicate stream ID).
  • File APIs: byte streams come with additional APIs for sending/receiving files on disk with MIME type and file extension inferencing.

- When possible, prefer using compiler synthesized methods for async/throwing methods
- Also mark deprecated instead of unavailable
@lukasIO
Copy link
Contributor

lukasIO commented Mar 1, 2025

for JS the start method of the ReadableStream is invoked immediately when the stream is constructed, see https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/ReadableStream#start

We don't want to depend on when the reader is being awaited as a signal to accept/discard the stream. Registering the handler on the room is showing a user's intent to process the streams already.

@ladvoc
Copy link
Contributor Author

ladvoc commented Mar 3, 2025

Registering the handler on the room is showing a user's intent to process the streams already.

That makes sense, thanks for the clarification. I’ll update the Swift implementation to match this behavior.

@bcherry bcherry self-requested a review March 4, 2025 07:23
Copy link
Contributor

@bcherry bcherry left a comment

Choose a reason for hiding this comment

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

lgtm and thank you for including thorough docstrings!

ladvoc added 4 commits March 4, 2025 14:15
- Open streams immediately after header is received instead of waiting for handler.
- Run handler in a detached task.
- Simplify handler resolution.
@ladvoc ladvoc requested a review from hiroshihorie March 4, 2025 23:03
@ladvoc
Copy link
Contributor Author

ladvoc commented Mar 5, 2025

The issue is now fixed. @lukasIO, I would appreciate your feedback on the new method I added to stream reader. This method allows the user to incrementally write a file to disk as it is received, returning its path once the entire file is received:

try await room.localParticipant
    .registerByteStreamHandler(for: "my-topic") { reader, participantIdentity in
        let fileURL = try await reader.readToFile()
        print("Wrote file to: \(fileURL)")
    }

By default, this method writes to the system temporary directory, and chooses the file name and extension by inspecting the name and MIME type fields from the stream header. Additional overloads also exist which allow the user to customize the destination directory and override the file name.

@lukasIO
Copy link
Contributor

lukasIO commented Mar 5, 2025

that's a nice addition!
readToFile sounds a bit confusing, .writeToFile or .toFile makes more sense in my head

@ladvoc
Copy link
Contributor Author

ladvoc commented Mar 5, 2025

I agree; I've renamed it to writeToFile(in:name:).

Copy link
Contributor

@lukasIO lukasIO left a comment

Choose a reason for hiding this comment

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

linting CI seems to be failing still, but lgtm otherwise! nice job!

Copy link
Member

@hiroshihorie hiroshihorie left a comment

Choose a reason for hiding this comment

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

Nice, tested it with the example app. ✅

@hiroshihorie hiroshihorie merged commit e7172ac into livekit:main Mar 7, 2025
10 of 13 checks passed
/// cannot be sent to remote participants.
///
public func write(_ text: String) async throws {
try await destination.write(Data(text.utf8))
Copy link
Contributor

Choose a reason for hiding this comment

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

@ladvoc sorry, missed this before, this is missing utf8 multi byte character splitting.
We want to ensure that for text, a utf8 character isn't arbitrarily split in half by chunking it. JS does this here https://github.com/livekit/client-sdk-js/blob/main/src/room/utils.ts#L629-L652

ladvoc added a commit that referenced this pull request Mar 10, 2025
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.

4 participants