DataTracks integration through FFI#66
Conversation
31fac42 to
e153e2e
Compare
ladvoc
left a comment
There was a problem hiding this comment.
Some initial comments, but I will make another pass. Looking clean so far!
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.
There was a problem hiding this comment.
for SDKs used in real-time systems, in general, exceptions are discouraged for "expected" failures like "buffer full" or "track unpublished".
I think the best practice here might be returning an result, which will include the success or error codes/Enums ?
ladvoc
left a comment
There was a problem hiding this comment.
Some additional comments.
8647582 to
74e4b99
Compare
6413736 to
2b792db
Compare
73062e1 to
98658f7
Compare
realsense-livekit: Examples of using DataTracks, realsense and mcap
…ich deprecates bridge. Add helper set/clearDataTrackCallback(), and publishDataTrack helper functions
…ame, assert(since that breaks cpp<->ffi<->rust agreement of rust handling buffering
f14faa5 to
f8f7685
Compare
| * The proto field is a bare uint64 with no prescribed unit. | ||
| * By convention the SDK examples use microseconds since the Unix epoch. | ||
| */ | ||
| std::optional<std::uint64_t> user_timestamp; |
There was a problem hiding this comment.
nit, suggested adding things like
DataFrame() = default;
DataFrame(DataFrame&&) = default;
DataFrame& operator=(DataFrame&&) = default;
These constructors will help compiler optimize those move, assignment use cases
|
|
||
| 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.
for SDKs used in real-time systems, in general, exceptions are discouraged for "expected" failures like "buffer full" or "track unpublished".
I think the best practice here might be returning an result, which will include the success or error codes/Enums ?
| * // process frame.payload | ||
| * } | ||
| */ | ||
| class DataTrackSubscription { |
There was a problem hiding this comment.
maybe I asked this question before, I wonder if should be called data_track_stream ?
or remote_data_track_stream ?
so it will be aligned with audio_stream / video_stream
|
|
||
| const auto &dts = event.data_track_subscription_event(); | ||
| if (dts.subscription_handle() != | ||
| static_cast<std::uint64_t>(subscription_handle_.get())) { |
There was a problem hiding this comment.
this worries me a bit, onFfiEvent is a callback. If DataTrackSubscription is moved to a new memory location, the lambda captured this.
take a step back, why do we need to support the move operator ? do we see real use cases from it ?
| auto *msg = req.mutable_data_track_subscription_read(); | ||
| msg->set_subscription_handle( | ||
| static_cast<uint64_t>(subscription_handle_.get())); | ||
| FfiClient::instance().sendRequest(req); |
There was a problem hiding this comment.
I think there are some races there.
like one thread calls this read(), and pass this section under std::lock_guardstd::mutex lock(mutex_); and get to this section.
meanwhile, close() is being called, and reset the subscription_handle_
| } | ||
|
|
||
| out = std::move(frame_.value()); | ||
| frame_.reset(); |
There was a problem hiding this comment.
nit, If the DataFrame have move constructor, you don't need this frame_.reset()
| * @return true on success, false if the push failed (e.g. back-pressure | ||
| * or the track has been unpublished). | ||
| */ | ||
| bool tryPush(const std::uint8_t *data, std::size_t size, |
There was a problem hiding this comment.
curiously, do we need all these 3 overloaded methods ? or we should push for one instead?
Sorry, I think I missed some details and have some more comments to address before landing this PR.
| } | ||
|
|
||
| std::shared_ptr<LocalDataTrack> | ||
| LocalParticipant::publishDataTrack(const std::string &name) { |
There was a problem hiding this comment.
curiously, what will happen if calling publishDataTrack with the same |name| more than once ?
| bool LocalDataTrack::tryPush(const std::vector<std::uint8_t> &payload, | ||
| std::optional<std::uint64_t> user_timestamp) { | ||
| DataFrame frame; | ||
| frame.payload = payload; |
There was a problem hiding this comment.
this makes me wondering if we should allow developers to pass things like
bool LocalDataTrack::tryPush(std::vectorstd::uint8_t&& payload,
std::optionalstd::uint64_t user_timestamp) { ... }
So that we can use the move constructor for the payload ?
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
Resolves BOT-268