Data streams v2#1192
Conversation
ChangesetThe following package versions will be affected by this PR:
|
| bytes = "1.10.1" | ||
| bmrng = "0.5.2" | ||
| flate2 = "1" | ||
| base64 = "0.22" |
There was a problem hiding this comment.
Note that flate2 is now a required dependency of livekit. Previously it was an optional dependency of livekit-api but that's it.
There was a problem hiding this comment.
Not sure if this is is worth doing yet, but it might be worth adding a "compression" feature to this crate to conditionally include this dep. It would be enabled by default, but would provide users who do not want compression to disable it at the feature level.
There was a problem hiding this comment.
I lean towards not doing it and adding it later if people wanted to opt out. But, I could be convinced otherwise if there's a really good use case. Also, in case it's relevant, flate2 is ~80kb.
There was a problem hiding this comment.
My concern would be more how it affects compile time and binary size; probably insignificant but it might be worth checking with cargo build --timings and cargo bloat --release --crates (crate) respectively.
| /// Streams a file from disk to participants as a byte stream. | ||
| /// | ||
| /// Never uses the inline single-packet path (deciding inline-eligibility would require | ||
| /// buffering and compressing the whole file up front). Compresses when every recipient | ||
| /// supports it. The whole file is never buffered in memory at once. |
There was a problem hiding this comment.
Note to self: make this docstring more user centric and not talk about the implementation.
| /// A capability a participant's client advertises (mirrors `ClientInfo.Capability`). | ||
| /// | ||
| /// Stored typed rather than as the raw protobuf `i32` so the public accessor doesn't leak | ||
| /// protobuf types, while `Unknown` preserves values this SDK build doesn't recognize. | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
| pub enum ClientCapability { | ||
| PacketTrailer, | ||
| CompressionDeflateRaw, | ||
| Unknown(i32), | ||
| } | ||
|
|
||
| impl From<i32> for ClientCapability { | ||
| fn from(value: i32) -> Self { | ||
| match proto::client_info::Capability::try_from(value) { | ||
| Ok(proto::client_info::Capability::CapPacketTrailer) => Self::PacketTrailer, | ||
| Ok(proto::client_info::Capability::CapCompressionDeflateRaw) => { | ||
| Self::CompressionDeflateRaw | ||
| } | ||
| // `CapUnused` and any value not recognized by this build. | ||
| _ => Self::Unknown(value), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<ClientCapability> for i32 { | ||
| fn from(value: ClientCapability) -> Self { | ||
| match value { | ||
| ClientCapability::PacketTrailer => { | ||
| proto::client_info::Capability::CapPacketTrailer as i32 | ||
| } | ||
| ClientCapability::CompressionDeflateRaw => { | ||
| proto::client_info::Capability::CapCompressionDeflateRaw as i32 | ||
| } | ||
| ClientCapability::Unknown(value) => value, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
question: Is shadowing the protobuf client capability enum like this the best idea? I think so but not 100% sure I've thought through all the pros and cons.
There was a problem hiding this comment.
Yes, this is the pattern I have taken for new public APIs to keep Protobuf as the wire contract only. One variation you might consider taking in this case is implementing TryFrom instead of From and discard unknown variants—I don't think an old client would ever be interested in the unknown case for client capabilities since it can't act on it anyway without being updated. Also, I would mark this enum as #[non_exhaustive].
There was a problem hiding this comment.
Migrated to TryFrom in 7162dbc. Still not 100% sure this is exactly right (Unused is a little weird and I'm not sure if I should be exposing it) but I think mostly there.
There was a problem hiding this comment.
Do you mean Unknown? I don't think it should be exposed since it doesn't provide any useful information to older client; it doesn't make sense to check for a capability that you don't know how to use.
There was a problem hiding this comment.
No, I mean ClientCapability::Unused - I'm not exactly sure why that was added to the protocol version of that struct since it's a repeated, so I'd think the default state would be an empty list, not 0? But either way, I suppose it's in the protocol version so it probably should be in the version here too.
I'll let jacob's in flight pull request push this all the way to the latest
…_compressed / is_inline" This reverts commit 3768723.
Add MaybeCompressed so that compression only ever happens at most once even if it has to happen speculatively sometimes
… sending a data stream This matches with how web does it
… can_compress / should_compress more clear
Not needed for now, can be re-added with an feature later
2 participants
Overview
This change introduces data streams v2, a set of major updates to data streams to add three things:
More info about data streams v2 can be found on the corresponding web sdk pull request. These updates are fully backwards compatible - the sdk will fall back to data streams v1 whenever an old participant is in the room.
Also as part of this, I've introduced a significant amount of new test coverage to data streams (both v1 and v2). There were some small bugs I found along the way which I fixed as well.