Conversation
f590f27 to
3f9fa7d
Compare
Changeset ✓This PR includes a changeset covering all affected packages:
|
01e3fbd to
c7580d8
Compare
alan-george-lk
left a comment
alan-george-lk
left a comment
There was a problem hiding this comment.
Approved with some minor stuff
| /// Packed BGRA. | ||
| Bgra, | ||
| /// Packed RGB24. | ||
| Rgb24, |
There was a problem hiding this comment.
Had this feedback with @stephen-derosa on the C++ side, any reason to not make these all-caps matching the comment/acronym?
| Self::Nv12 => "nv12", | ||
| Self::Bgra => "bgra", | ||
| Self::Rgb24 => "rgb24", | ||
| Self::Bgr24 => "bgr24", |
There was a problem hiding this comment.
Do these need to be lowercase?
| #[allow(dead_code)] | ||
| fn capture_frame_metadata( | ||
| capture_wall_time_us: u64, | ||
| ) -> livekit::webrtc::video_frame::FrameMetadata { | ||
| livekit::webrtc::video_frame::FrameMetadata { | ||
| user_timestamp: Some(capture_wall_time_us), | ||
| frame_id: None, | ||
| user_data: None, | ||
| } | ||
| } | ||
|
|
||
| /// Validates the platform-neutral parts of a configuration; `supported` | ||
| /// reports whether the backend can deliver a frame format. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Similar to other PRs, why allow dead code on new stuff?
| //! platform-native buffers without a CPU copy. Otherwise they are converted | ||
| //! to I420. | ||
|
|
||
| use unsupported as backend; |
There was a problem hiding this comment.
I wasn't sure on this so I asked GPT, so take with a grain of salt:
Device capture is not implemented on any platform. mod.rs:28 unconditionally aliases the backend to unsupported, and source-device = [] adds no platform backend or dependencies. Consequently, devices(), devices_blocking(), DeviceVideoSource::new(), and the new FFI requests will always return UnsupportedPlatform, including on macOS/Linux/Windows.
use unsupported as backend;is not a runtime fallback decision—it selects that module at compile time. The selected module’s Session::open() always returns an error. So this PR exposes a polished public API, but no code path can ever open a camera.
Is there a way to make a unit test or some sort of test that exercises this? Is this actually an issue for this PR?
5beef13 to
9b4398b
Compare
9b4398b to
fd4c331
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| // Format selection requested from a capture device. The device negotiates | ||
| // the delivered format; CaptureSourceInfo reports the outcome. | ||
| message DeviceFormatRequest { |
There was a problem hiding this comment.
🟡 Negotiated camera format goes missing
When DeviceFormatRequest permits negotiation, the response omits the selected frame rate and pixel format. FFI clients cannot discover the delivered format promised by this API.
Prompt for agents
Extend the FFI result for device source creation to expose the complete negotiated DeviceFormat, not only CaptureSourceInfo.resolution. Update livekit-ffi/protocol/capture.proto, populate the value from DeviceVideoSource::format in livekit-ffi/src/server/capture.rs before erasing the source behind PixelVideoSource, and regenerate all protocol bindings. Preserve behavior for non-device capture sources, likely with an optional device-specific field.
Was this helpful? React with 👍 or 👎 to provide feedback.
e0da75a to
ea9bd68
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| server.async_runtime.spawn(async move { | ||
| let message = match device::devices().await { |
There was a problem hiding this comment.
🟡 Enumeration panics lose callbacks
A backend panic makes on_list_capture_devices drop its callback. devices resumes blocking-task panics, while the detached task remains unobserved.
Learn more
devices deliberately resumes a panic from its blocking worker. That panic then terminates this detached Tokio task before it constructs ListCaptureDevicesCallback. The returned async ID has no terminal callback, and the discarded JoinHandle also hides the failure from the FFI panic reporting path.
Example: A platform backend panics while querying a malformed native device descriptor. The request immediately returns async ID 42, but callback 42 never arrives.
Recommended fix: Keep and supervise the task handle through the server's task-watching mechanism, and convert enumeration panics into the callback's error branch so every accepted async ID completes exactly once.
Was this helpful? React with 👍 or 👎 to provide feedback.
ea9bd68 to
8b19169
Compare
8b19169 to
929dc5f
Compare
5326fba to
7fc151f
Compare
7fc151f to
be05ba8
Compare
2 participants
Add a capture source for camera devices.
This PR defines the API for device source (which is the same across platforms); backend implementations (e.g., AVFoundation for macOS, V4L2 for Linux, etc.) are left to subsequent PRs).
Closes BOT-528