(WIP) Integrate livekit capture - #227
Conversation
653a32f to
328419a
Compare
|
|
||
| // Application options are merged in; source-dictated fields win. | ||
| TrackPublishOptions app_options; | ||
| app_options.source = TrackSource::SOURCE_CAMERA; |
There was a problem hiding this comment.
from sync: Should the capture source specify this field?
There was a problem hiding this comment.
@ladvoc im trying to recall the conversation but i cant remember
There was a problem hiding this comment.
check if this is actually needed or should be inferred
| Stopped = 0, | ||
| /// The producer reached the end of its stream. | ||
| EndOfStream = 1, | ||
| }; |
There was a problem hiding this comment.
from sync: Add panic reason.
There was a problem hiding this comment.
should be in rust, need to float up here
b96c554 to
33cda61
Compare
| libabsl-dev \ | ||
| libcurl4-openssl-dev \ | ||
| libwayland-dev libdecor-0-dev \ | ||
| libgstreamer1.0-dev libgstreamer-plugins-base1.0-dev \ |
There was a problem hiding this comment.
Help me understand the difference in versions between these installs and the one at the top of this file for Windows? 1.0-dev vs. 1.28.5? Just making sure no surprises
There was a problem hiding this comment.
Good callout, I added a clarifying comment. Effectively we are doing this to ensure we go through through each platform’s native package mechanism (linux/mac use latest whereas windows uses an explicit pinned version).
| Bps = 0, | ||
| /// Kilobits per second. | ||
| Kbps = 1, |
There was a problem hiding this comment.
Do we have any precedence for bit rate enums/units elsewhere? Might be worth consolidating if so
There was a problem hiding this comment.
After reviewing the Rust side earlier today, looks like we have GStreamer-specific bit rates in Rust too, so maybe better to match that? CC @ladvoc on thoughts, I'm curious if both Rust and C++ can consolidate bitrates into a more generic unit/enum. Not a biggie if not, just trying to minimize API surface
| struct CaptureResolution { | ||
| int width = 0; | ||
| int height = 0; | ||
| }; |
There was a problem hiding this comment.
Similar comment here: https://github.com/livekit/client-sdk-cpp/pull/227/changes#r3845438732
If we have a generic video resolution struct somewhere, a smaller API surface would be ideal
| Nv12 = 1, | ||
| /// Packed BGRA. | ||
| Bgra = 2, | ||
| /// Packed RGB24. | ||
| Rgb24 = 3, | ||
| /// Packed BGR24. | ||
| Bgr24 = 4, | ||
| /// Packed YUYV/YUY2. | ||
| Yuyv = 5, | ||
| /// Packed UYVY. | ||
| Uyvy = 6, | ||
| /// Single-plane 8-bit luma. | ||
| Grey = 7, | ||
| /// Encoded MJPEG frames. | ||
| Mjpeg = 8, |
There was a problem hiding this comment.
Definitely a fan of enum styling consistency if we have precedence for CapitalLetterEnum style, but wondering if some of these would be all caps, like RGB24. I'd default first to our SDK precedent and second to how Rust defines them
There was a problem hiding this comment.
nice good catch, ill align with our SDK
| const proto::CaptureDeviceList list = devices.get(); | ||
| std::vector<CaptureDeviceInfo> out; | ||
| out.reserve(static_cast<std::size_t>(list.devices_size())); | ||
| for (const proto::CaptureDeviceInfo& info : list.devices()) { | ||
| out.push_back(fromProto(info)); | ||
| } | ||
| return out; |
There was a problem hiding this comment.
Worth collapsing into a from helper (see below) for similar pattern/unit testability? listDevices has tests but am assuming most if not all will fail in CI, moving this bit out at least covers some of it
There was a problem hiding this comment.
yup good point, ill add the helper add some tests
There was a problem hiding this comment.
also broke this into the private api
| if (resolution.width <= 0 || resolution.height <= 0) { | ||
| throw CaptureSourceError(std::string(field) + " must be positive, got " + std::to_string(resolution.width) + "x" + | ||
| std::to_string(resolution.height)); |
There was a problem hiding this comment.
Are there any upper invalid bounds to consider?
There was a problem hiding this comment.
hm, i wasnt able to find upper bounds on resolution anywhere in the rust SDK (@ladvoc any idea if this exists ?) and i dont feel like its appropriate to set the precedent here.
The resolution is currently just bounded by INT_MAX
| /// Skips the calling test when the FFI library was built without the capture | ||
| /// feature. A feature-less FFI reports only a generic invalid handle, so this | ||
| /// has to be decided at compile time rather than sniffed from an error string. | ||
| #ifdef LIVEKIT_TEST_CAPTURE_ENABLED |
There was a problem hiding this comment.
Do we need a specific test flag? What about just using a LIVEKIT_CAPTURE_ENABLED from the public API/build, or is the idea to hide from users
There was a problem hiding this comment.
good call no this should be LIVEKIT_CAPTURE_ENABLED specifically to cover the case where users can build with it off
e844dd6 to
57e6fe9
Compare
stephen-derosa
changed the title
3 participants
Integrates livekit/rust-sdks#1299