Skip to content

Navigation Menu

Sign in
Sign up

(WIP) Integrate livekit capture - #227

Draft
ladvoc wants to merge 27 commits into
main from
ladvoc/livekit-capture
Draft

(WIP) Integrate livekit capture #227
ladvoc wants to merge 27 commits into
main from
ladvoc/livekit-capture

Conversation

@ladvoc

@ladvoc ladvoc commented Jul 30, 2026

Copy link
Copy Markdown
Contributor


// Application options are merged in; source-dictated fields win.
TrackPublishOptions app_options;
app_options.source = TrackSource::SOURCE_CAMERA;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from sync: Should the capture source specify this field?

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ladvoc im trying to recall the conversation but i cant remember

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if this is actually needed or should be inferred

Stopped = 0,
/// The producer reached the end of its stream.
EndOfStream = 1,
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from sync: Add panic reason.

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ladvoc has this gotten into the rust layer?

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be in rust, need to float up here

libabsl-dev \
libcurl4-openssl-dev \
libwayland-dev libdecor-0-dev \
libgstreamer1.0-dev libgstreamer-plugins-base1.0-dev \

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

alan-george-lk reacted with thumbs up emoji
Comment on lines +62 to +64
Bps = 0,
/// Kilobits per second.
Kbps = 1,

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any precedence for bit rate enums/units elsewhere? Might be worth consolidating if so

@alan-george-lk alan-george-lk Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +68 to +71
struct CaptureResolution {
int width = 0;
int height = 0;
};

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread include/livekit/capture_source.h Outdated
Comment thread include/livekit/capture_source.h Outdated
Comment on lines +133 to +147
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,

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice good catch, ill align with our SDK

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment thread src/capture_source.cpp Outdated
Comment on lines +320 to +326
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;

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup good point, ill add the helper add some tests

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also broke this into the private api

Comment thread src/capture_source.cpp Outdated
Comment thread include/livekit/capture_source.h Outdated
Comment thread src/capture_source.cpp
Comment on lines +48 to +50
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));

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any upper invalid bounds to consider?

@stephen-derosa stephen-derosa Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

alan-george-lk reacted with thumbs up emoji
/// 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

@alan-george-lk alan-george-lk Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stephen-derosa stephen-derosa Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call no this should be LIVEKIT_CAPTURE_ENABLED specifically to cover the case where users can build with it off

@stephen-derosa stephen-derosa changed the title (削除) (WIP) Integrate capture (削除ここまで) (追記) (WIP) Integrate livekit capture (追記ここまで) Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephen-derosa stephen-derosa stephen-derosa left review comments
@alan-george-lk alan-george-lk alan-george-lk left review comments
@xianshijing-lk xianshijing-lk Awaiting requested review from xianshijing-lk xianshijing-lk will be requested when the pull request is marked ready for review xianshijing-lk is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /