Repository navigation
LK Capture integration - #276
stephen-derosa wants to merge 14 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
this is a helper for some repeat code in the Cmake
There was a problem hiding this comment.
Not super following the need for these changes, mind explaining or asking your agent context?
There was a problem hiding this comment.
still relevant?
| target_link_libraries(livekit_unit_tests | ||
| PRIVATE | ||
| livekit | ||
| livekit_ffi # Capture ownership tests verify Rust handle disposal directly. |
There was a problem hiding this comment.
Feel a lil iffy about this, only because our test binaries are a good indicator that our linking is working, and depending on livekit should depend on livekit_ffi. Is there a way to avoid this? I ran into this for the uniffi work, I ended up going with the private src/ uniffi stuff to work around this, maybe an option?
There was a problem hiding this comment.
good callout -- sol recommended going with the same private src/ approach -- implemented
There was a problem hiding this comment.
done here -- it adds a new
[[nodiscard]] LIVEKIT_INTERNAL_API bool dropFfiHandle(std::uint64_t handle) noexcept;
to ffi_handle.h/cpp
|
|
||
| /// Serialize capture configurations for the FFI request. | ||
| /// Internal test seam; not part of the public SDK API. | ||
| LIVEKIT_INTERNAL_API proto::NewCaptureSourceRequest toProto(GstreamerVideoSourceConfig config); |
There was a problem hiding this comment.
I assume some clang reason, but why is this one not const & like the others?
There was a problem hiding this comment.
good catch, this was actually just an AI choice so that it could call
rate_control->set_element(std::move(config.rate_control->element));
rate_control->set_property(std::move(config.rate_control->property));
sionce in the gstreamer case these atr strings. But since its done once i think its relatively safe to conform to const ref
There was a problem hiding this comment.
@alan-george-lk this adds a little more overhead in effort to reduce complexity in the builds.yml -- lmk your thoughts. I like it but dont love it.
There was a problem hiding this comment.
Not the biggest fan, adds a layer of indirection when looking at builds.yml, and IMHO actions are best for use in other repos compared to internal. Not gonna hold things up for this but I'd rather have builds.yml be larger
There was a problem hiding this comment.
On second read, looks like it does save repeating things between tests.yml and make-release.yml, maybe it is worthwhile, not sure.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8027eef to
14478b4
Compare
…ope of the public API WRT capture.
…/build.rs file Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
14478b4 to
d5a4299
Compare
#227 minus device capture source and addressing comments.
This implementation assumes that the livekit capture build is behind a build arg which would bring in gstreamer deps (very large)