DataTracks integration through FFI#66
Conversation
| - src/** | ||
| - include/** | ||
| - examples/** | ||
| - bridge/** |
There was a problem hiding this comment.
the bridge/ folder code only be internal ?
if so, I think we should put the code under src folder
the structure should be clean like
src/ : contains all the internal source code
tests/ contains all the test code
include/livekit : contain all the public interface
as we are adding bridge, if it is only internal to src, lets put it under src/internal/bridge
There was a problem hiding this comment.
sounds good -- i will make this a single git mv commit at the end so comments are easier to follow!
| }; | ||
|
|
||
| virtual ~DataTrackSubscription(); | ||
|
|
There was a problem hiding this comment.
from what I understand, this is more like a data_track_stream ?
that will align with our audio_stream or video_stream.
One of the main purpose of data track is to build similar techs like audio / video tracks, to align with the same mental modle
There was a problem hiding this comment.
agree -- poor naming. what are your thoughts on deprecating (removing) the existing data_stream work and fully replacing it with data tracks?
There was a problem hiding this comment.
This should be called subscription. Unlike audio and video stream, this object actually represents a subscription and follows RAII semantics (e.g., when the subscription is dropped, the SDK communicates with the SFU to unsubscribe in order to stop being forwarded frames).
There was a problem hiding this comment.
I think it might not be immediately clear, but RAII is implemented on the Rust side. C++ only needs to worry about dropping the FFI handle (which it is doing) and cleanup will be taken care of on the Rust side.
There was a problem hiding this comment.
At least one confusing thing about "subscription" is that this object can also call pushFrame(), so its not purely a subscription
There was a problem hiding this comment.
At least one confusing thing about "subscription" is that this object can also call pushFrame(), so its not purely a subscription
From sync: data flow is unidirectional, so there should be no push method on the subscription.
There was a problem hiding this comment.
pushFrame in this context pushes a received DataFrame to the local queue, not to the data track
There was a problem hiding this comment.
audio_stream and video_stream are very similar that they offer the blocking read() functions, and they are subscribed to ffi events on the audio / video frames and move frames over. Similar to TextStreamReader for the legacy data stream.
@ladvoc, can you point me to the Rust example code how to hook up remove video / audio tracks with the screen and speaker?
I think there are some gaps between rust and c++ / python SDKs that make things different
There was a problem hiding this comment.
@shishirng @ladvoc i think we're all in agreement of the path here?
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
31fac42 to
e153e2e
Compare
ladvoc
left a comment
ladvoc
left a comment
There was a problem hiding this comment.
Some initial comments, but I will make another pass. Looking clean so far!
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
00f046a to
b07035d
Compare
|
|
||
| proto::FfiResponse resp = FfiClient::instance().sendRequest(req); | ||
| const auto &r = resp.local_data_track_try_push(); | ||
| return !r.has_error(); |
There was a problem hiding this comment.
question: Should we be exposing the error reason to the caller? There are several reasons why pushing a frame could fail (e.g., the track is unpublished, buffer full, etc.). For the ROS bridge, a boolean might be fine if there is logging, but for the core SDK, I lean towards we should report the error reason.
Related question: the FFI interface currently flattens all errors to strings for simplicty, however, on the Rust side there are proper error enums that describe the reason for the error. Should I expose these over FFI rather than using strings? Can an enum describing the error reason map nicely into a C++ exception?
There was a problem hiding this comment.
great question -- I need to sync with @xianshijing-lk on what the client facing applications should do. I think in general Livekit should never cause an application to crash (other than assert calls), so the sdk should catch any exceptions. Of course we still want to flow up the root cause (which we are currently doing). I could be way off here.
ladvoc
left a comment
ladvoc
left a comment
There was a problem hiding this comment.
Some additional comments.
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
3 times, most recently
from
8647582 to
74e4b99
Compare
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
4bdc8ec to
1a1b5ac
Compare
realsense-livekit: Examples of using DataTracks, realsense and mcap bridge/tests/: integration, stress, unit bridge_human_robot: DataTracks
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
6413736 to
2b792db
Compare
src/data_track_subscription.cpp
Outdated
|
|
||
| // rust side handles buffering, so we should only really ever have one item | ||
| if (queue_.size() >= 2) { | ||
| LK_LOG_ERROR("[DataTrackSubscription] Queue size is greater than 1, this " |
| } | ||
|
|
||
| // rust side handles buffering, so we should only really ever have one item | ||
| if (frame_.has_value()) { |
There was a problem hiding this comment.
@xianshijing-lk (cc @1egoman @ladvoc ) FYI because buffering is controlled on the rust side of things, we dont do any queueing/buffering on the cpp receiving side. Instead, we just store the latest frame and warn if there alreadya. frame when rust sends us a new one
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
73062e1 to
98658f7
Compare
stephen-derosa
force-pushed
the
sderosa/data_tracks
branch
from
98658f7 to
ce5565a
Compare
4 participants
Overview
Integrate DataTracks into the CPP SDK through the FFI
Building
This library is attached to the build system of the core C++ SDK library. Use
build.shas is.Testing
bridge/tests/ implements new tests for stress, integration, and unit. These closely follow the testing format of the base SDK.
Examples
examples/bridge_human_robot/has been updated to include a DataTrack. The Robot sends a string with a time and count, the human prints it.examples/realsense-livekit/has examples for getting data from a realsense camera, serializing, compressing and sending it. It also shows writing receiving participant data and writing it to an mcap which can be visualized in foxglove.Unit tests
bridge/tests/unit/test_bridge_data_track.cppLimitations
Blocked By
Rust SDKs PR