Add send_bytes method

[bcherry]

No description provided.

[ladvoc]

Summary of change:
The send_text and send_bytes methods now use push by default, where the receiver will automatically read incoming streams and dispatch them using the new TextReceived and BytesReceived events accordingly.

Compatibility:

• ⚠️ Breaking change: for applications that already use send_text, the receiver side must be updated to handle the new event types.
• Clients that do not support push are unaffected and will continue to receive stream opened events.

Example: See the new send_bytes example.

[bcherry]

@ladvoc I think it's a a pretty big challenge to break send_text in this way. is it not possible to change the receiver option so you can choose - per-topic - whether you want to receive incoming streams as streams or as wholes?

[lukasIO]

mapping the send_* APIs to a single event makes sense to me, but I have similar reservations about the breaking change of send_text.

My preference would be to address this consistently across clients for v3.

[ladvoc]

If we want to add this prior to v3, I see a few possibilities to make this a non-breaking change:

1. Only apply push to send_bytes since it is a new method.
2. Make push opt-in by default:

let options = StreamByteOptions {
   topic: "some-topic".into(),
   push: true,
   ..Default::default()
};
room.local_participant().send_bytes([0xFF; 16], options).await?;

3. As per Ben's suggestion, add an option on the receiving end to opt-in on a per-topic basis.

For client v3, push can become the default for the send_* methods.

[1egoman]

If we want to add this prior to v3, I see a few possibilities to make this a non-breaking change:

@ladvoc There might be an option 4 too - keep send_text / send_bytes working as is, and add brand new send_text_push/send_bytes_push (idk about the names) which could be breaking, and then deprecate the old versions. Then in v3 rename the new ones back to send_text / send_bytes.

[1egoman]

I'm not sure how big TextStreamInfo/ByteStreamInfo would be in practice (so I'm not sure how important / valid this suggestion really is) but you could probably get rid of the .clone() by adding something like a fn info_owned(self) -> Self::Info; method to the StreamReader implementation here and then moving this line below the reader.read_all so info_owned could fully take the value / not just operate on a reference?

[1egoman]

(The same suggestion / caveats from previous comment also apply here!)

[1egoman]

I'm not super familiar with what is expected to be within attributes in this context by external systems / other clients, but is this just being used as an internal flag within the rust sdk to track whether push is enabled or disabled, or are other external system / other clients depending on this value being set as well?

If it is purely internal, it might be cleaner to have an explicit flag somewhere else? To be fair I can see why you did it this way (I don't see any other obvious places where it could go) but I think worst case, if you have to keep it there, it would be worth giving it a lk: prefix like other attribute keys in other places so it should never collide with a customer provided value.

Also no matter whether it is internal or external, IMO it would be good to extract it into a named constant somewhere (or maybe even an enum if there are more values like this) which can make this a bit more self descriptive / not just be a "magic string".

[ladvoc]

Agreed, this is a temporary solution to demonstrate proof-of-concept. I have proposed adding a dedicated "push" field to stream header to replace this:

• Add push flag to data stream header protocol#1208

[bcherry]

What is the benefit of tightly coupling the send and receive methods to only be compatible with the same variant on each side?

It seems like you could have all of the benefits if you just have the receiving client either register a stream handler or a blob handler (but not both) while the sender can choose to send as a stream or as a blob. Then its fully compatible either way and also gives flexibility to developers to choose implementations that match their needs (for instance, maybe on your memory-constrained hardware device you'll choose to stream a large file from disk to avoid a memory usage spike, but on the agent side you'll just want to receive it in one blob to upload to your LLM).

In JS etc it could be something like this:

room.registerByteStreamHandler(for: "my-topic") { reader, participantIdentity in
        for try await chunk in reader {
            // do something
        }
}

vs

room.registerBytesHandler(for: "my-topic") { event, participantIdentity in
    // event.bytes would hold all the data
}

I think only Rust is more of a challenge due to the events-based structure but to be honest, maybe it's a problem we don't really need to solve anyways. The rust SDK is not used independently by very many developers and the existing reader.take() syntax isn't that much more complicated than the proposal here.

btw I don't think clients v3 is a good solve here either. Making client v3 a breaking change vs your existing code is fine (developers just need to update some stuff) but making them incompatible with other participants running older sdk versions is going to make it all very complicated to roll out.

The thing I want us to optimize for in my head is ensuring that agents developers who just adopt a starter agent and starter frontend don't have any difficulties with chat and transcriptions. We would need to tightly coordinate the release of a new agents sdk with new frontend starters for all platforms at the same time to roll out a breaking change here, and we'd still be left with the risk that a developer unknowingly updates their agents SDK (of which the RTC SDK is just a hidden dependency) without updating their frontend SDK as well (not to mention the challenges in actually forcing a quick update when using a mobile app distributed through a store). There's really no good way to introduce a breaking change into the way text transcriptions and chat go back and forth here so we should do it very sparingly.

My advice is to add the new send_bytes API using the existing stream primitives, and separately work on simplifying the receiving side so there's an easier way to just collect whole streams.

[lukasIO]

hm, maybe I misunderstood the main motivation behind this PR.

If it's mainly about enabling users to deal with the blobs directly then we already have a readAll method available on readers in other SDKs which makes that possible.

That seems more obvious to me than introducing a second, separate handler

[ladvoc]

After further discussion, we decided to limit the scope of this PR to only add the send_bytes method.

[1egoman]

Not sure how helpful my comments are, but you re-requested my review so here's one more small thing I noticed! Otherwise it looks good to me - if you want me to approve it I can. Not sure if there's somebody else who should be doing that though who understands how things work more deeply!