Move data streams into their own crate#1227
Conversation
Not needed for now, can be re-added with an feature later
ChangesetThe following package versions will be affected by this PR:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1egoman
force-pushed
the
data-streams-refactor
branch
from
1e4e420 to
4492d6e
Compare
| impl IncomingStreamManager { | ||
| pub fn new() -> (Self, UnboundedReceiver<(AnyStreamReader, String)>) { | ||
| pub fn new( | ||
| reserved_topics: Vec<String>, | ||
| ) -> (Self, UnboundedReceiver<(AnyStreamReader, String)>) { |
There was a problem hiding this comment.
Notable change: Instead of looking at a well known global, IncomingStreamManager now takes a list of reserved_topics so that upstream layers can pass down a list of reserved topics in a decoupled fashion.
| impl AnyStreamInfo { | ||
| enum_dispatch!( | ||
| [Byte, Text]; | ||
| pub fn id(self: &Self) -> &str; | ||
| pub fn total_length(self: &Self) -> Option<u64>; | ||
| pub fn encryption_type(self: &Self) -> EncryptionType; | ||
| ); | ||
| pub fn id(&self) -> &str { | ||
| match self { | ||
| Self::Byte(info) => info.id(), | ||
| Self::Text(info) => info.id(), | ||
| } | ||
| } | ||
|
|
||
| pub fn total_length(&self) -> Option<u64> { | ||
| match self { | ||
| Self::Byte(info) => info.total_length(), | ||
| Self::Text(info) => info.total_length(), | ||
| } | ||
| } | ||
|
|
||
| pub fn encryption_type(&self) -> EncryptionType { | ||
| match self { | ||
| Self::Byte(info) => info.encryption_type(), | ||
| Self::Text(info) => info.encryption_type(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Note to reviewers: I have dropped libwebrtc::enum_dispatch and written the implementation myself to avoid making libwebrtc a dependency of livekit-data-streams.
There was a problem hiding this comment.
How about moving enum_dispatch to livekit-common?
There was a problem hiding this comment.
Ah, yea, I could do that - done in 433b85d.
I've kept the libwebrtc copy around so that it isn't coupled to livekit-common, IMO the goal should be to migrate all users to livekit_common::enum_dispatch and then drop the libwebrtc version.
|
|
||
| /// Take ownership of the value in the cell if it matches some predicate. | ||
| /// | ||
| /// This method will only take the value if the provided predicate returns `true` when called with the current value. | ||
| /// If the predicate returns `false` or the value has already been taken, this method returns `None`. | ||
| pub(crate) fn take_if_raw(&self, predicate: impl FnOnce(&T) -> bool) -> Option<T> { | ||
| if self.value.read().as_ref().map_or(false, |v| predicate(v)) { | ||
| self.take() | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /// Take ownership of the value in the cell. If the value has, |
There was a problem hiding this comment.
Note to reviewers - this was dead code.
|
|
||
| [features] | ||
| # End-to-end testing hooks (exposes is_compressed/is_inline on stream info). Forwarded from | ||
| # the `livekit` crate's `__lk-e2e-test` feature. | ||
| __e2e-test = [] | ||
| # Exposes constructors used by downstream crates' test suites (e.g. `TextStreamReader::new_for_test`). | ||
| test-utils = [] |
There was a problem hiding this comment.
Note to reviewers: should these be separate features or just one?
There was a problem hiding this comment.
If we need to keep them, they should be separate (I plan to eventually drop the special feature for E2E testing and instead just skip E2E tests based on env).
However, there's the bigger question of if we should be using non-public methods in integration tests in the first place.
There was a problem hiding this comment.
However, there's the bigger question of if we should be using non-public methods in integration tests in the first place.
Right now, that's how it works so I don't think this makes it worse. IMO I'd rather decouple that conversation and revisit as part of the data streams v2 pull request since I will be making some more extensive updates there.
…d CLIENT_PROTOCOL_DATA_STREAM_V2 from livekit-common
2 participants
PR description
Breaks out a subset of the changes from #1192 into a pull request which exclusively moves the existing data streams implementation into its own crate,
livekit-data-streams, and adds a newlivekit-commoncrate (kept fairly minimal for now) for common utilities whichlivekit-data-streamsandlivekitboth need.This is largely being scoped to a "lift and shift" type effort, more extensive refactoring of the
livekit-data-streamscrate will be done in #1192.Breaking changes
There should not be any breaking changes from the perspective of
livekit's public api. If so, please surface them.Testing
This pull request doesn't right now add any new test coverage beyond the existing fairly minimal e2e test coverage.