Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let initial_frame = inner.wait_for_format(FIRST_FRAME_TIMEOUT)?; | ||
| inner.discard_pending_frame(); | ||
| let mut format = initial_frame.format; | ||
| format.framerate_fps = requested_framerate(&config.format).unwrap_or(30); |
There was a problem hiding this comment.
🟡 Highest-framerate capture reports a fabricated 30 fps
For a HighestFramerate request, requested_framerate returns None, so format.framerate_fps is hardcoded to 30 even when the selected format runs at 60 or 120 fps. The public format() then reports 30 fps for a device explicitly asked to deliver its highest rate.
Prompt for agents
In Session::open (avfoundation.rs:114) format.framerate_fps is set to requested_framerate(&config.format).unwrap_or(30). For DeviceFormatRequest::HighestFramerate and Default, requested_framerate returns None, so the reported framerate is always 30 regardless of what the negotiated device format actually supports. Consider deriving the reported framerate from the selected/active device format (e.g. device_format_max_framerate of the active format) when the request does not carry an explicit framerate, so format() reflects the real capture rate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let media_type = unsafe { AVMediaTypeVideo }.ok_or(DeviceVideoSourceError::DeviceNotFound)?; | ||
| match selector { | ||
| DeviceSelector::Default => { | ||
| unsafe { AVCaptureDevice::defaultDeviceWithMediaType(media_type) } | ||
| .ok_or(DeviceVideoSourceError::DeviceNotFound) | ||
| } | ||
| DeviceSelector::Index(index) => { | ||
| #[allow(deprecated)] | ||
| let devices = unsafe { AVCaptureDevice::devicesWithMediaType(media_type) }; | ||
| devices | ||
| .iter() | ||
| .nth(*index) | ||
| .map(|device| device.retain()) | ||
| .ok_or(DeviceVideoSourceError::DeviceNotFound) | ||
| } | ||
| DeviceSelector::Id(id) => { | ||
| let id = NSString::from_str(id); | ||
| unsafe { AVCaptureDevice::deviceWithUniqueID(&id) } | ||
| .ok_or(DeviceVideoSourceError::DeviceNotFound) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Unsafe FFI blocks lack required SAFETY comments
The four unsafe blocks calling AVFoundation FFI here carry no // SAFETY: comment, unlike every other unsafe block in the file. AGENTS.md requires a SAFETY comment on every unsafe block explaining why the operation is sound.
Prompt for agents
AGENTS.md requires that every unsafe block have a // SAFETY: comment. select_device (avfoundation.rs:761-785) has four unsafe FFI blocks (AVMediaTypeVideo, defaultDeviceWithMediaType, devicesWithMediaType, deviceWithUniqueID) with none. Add SAFETY comments consistent with the rest of the file (which documents these exact calls elsewhere).
Was this helpful? React with 👍 or 👎 to provide feedback.
| exact_session_preset(resolution).or(Some(unsafe { AVCaptureSessionPresetHigh })) | ||
| } | ||
|
|
||
| fn exact_session_preset( | ||
| resolution: VideoResolution, | ||
| ) -> Option<&'static objc2_av_foundation::AVCaptureSessionPreset> { | ||
| match (resolution.width, resolution.height) { | ||
| (1920, 1080) => Some(unsafe { AVCaptureSessionPreset1920x1080 }), | ||
| (1280, 720) => Some(unsafe { AVCaptureSessionPreset1280x720 }), | ||
| (640, 480) => Some(unsafe { AVCaptureSessionPreset640x480 }), | ||
| (w, h) if w <= 640 && h <= 480 => Some(unsafe { AVCaptureSessionPresetMedium }), | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
🟡 Session-preset unsafe blocks lack SAFETY comments
The unsafe blocks reading AVFoundation preset constants in session_preset and exact_session_preset carry no // SAFETY: comment. AGENTS.md requires one on every unsafe block.
Prompt for agents
AGENTS.md requires a // SAFETY: comment on every unsafe block. session_preset (avfoundation.rs:1084) and exact_session_preset (avfoundation.rs:1090-1096) read framework preset constants inside unsafe blocks without any SAFETY comment. Add SAFETY comments matching the style used elsewhere in the file for framework-constant access.
Was this helpful? React with 👍 or 👎 to provide feedback.
bb2dfc0 to
8a8d6f8
Compare
Changeset incompleteThis PR's changeset is missing version bumps for packages that are affected by the change. The following packages still require a bump:
Already covered:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
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.
View 2 additional findings in Devin Review. (Configure)
| // and the destination planes come from a freshly allocated I420Buffer with matching | ||
| // width, height, and strides. | ||
| let ret = unsafe { | ||
| yuv_sys::rs_BGRAToI420( |
There was a problem hiding this comment.
🔴 BGRA camera frames converted with wrong color channels
convert_bgra feeds a CoreVideo 32BGRA buffer (bytes B,G,R,A in memory) to yuv_sys::rs_BGRAToI420, but libyuv names that memory layout ARGB and its BGRAToI420 expects A,R,G,B. The repo's own renderer converts the same B,G,R,A layout with rs_ARGBToI420 (livekit-capture/src/renderer.rs:31-33,446). Every BGRA-delivered camera frame gets red and blue swapped.
| yuv_sys::rs_BGRAToI420( | |
| yuv_sys::rs_ARGBToI420( |
Was this helpful? React with 👍 or 👎 to provide feedback.
46713c7 to
70e6561
Compare
70e6561 to
7d55c0f
Compare
| let preferred = [ | ||
| // WebRTC's VideoToolbox H.264 encoder allocates full-range NV12 | ||
| // buffers for its CPU upload path. Prefer the same CoreVideo | ||
| // format for direct CVPixelBuffer input so the native path does | ||
| // not have to reset VideoToolbox into a separate video-range pool. | ||
| kCVPixelFormatType_420YpCbCr8BiPlanarFullRange, | ||
| kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange, | ||
| ]; |
There was a problem hiding this comment.
Not confident in this, but does this NV12-only preferred list limit the ability to use non-NV12? i.e. not using frame_format here to match requested frame format
7d55c0f to
e7a4f70
Compare
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.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let session = unsafe { AVCaptureSession::new() }; | ||
| let input = unsafe { AVCaptureDeviceInput::deviceInputWithDevice_error(&device) }.map_err( | ||
| |err| DeviceVideoSourceError::Backend(err.localizedDescription().to_string()), | ||
| )?; | ||
| let output = unsafe { AVCaptureVideoDataOutput::new() }; |
There was a problem hiding this comment.
🟡 Unsafe blocks missing required SAFETY comments
AGENTS.md requires a // SAFETY: comment on every unsafe block. The session, input, and output constructor unsafe blocks here carry none; the comment just above covers only the two string getters preceding them. Other unsafe sites in the file (select_device, best_device_format) are likewise undocumented.
Was this helpful? React with 👍 or 👎 to provide feedback.
e7a4f70 to
510d689
Compare
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 2 new potential issues.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| DeviceFormatRequest::HighestFramerate { resolution, .. } => { | ||
| Ok(best_device_format(device, *resolution, None, SelectionMode::HighestFramerate)) | ||
| } | ||
| DeviceFormatRequest::HighestResolution { framerate_fps, .. } => { | ||
| Ok(best_device_format(device, None, *framerate_fps, SelectionMode::HighestResolution)) | ||
| } |
There was a problem hiding this comment.
🟡 Unsupported constraints silently fall back
select_active_format falls back when highest-mode constraints have no match. Construction then succeeds with a resolution or frame rate the request forbids.
Prompt for agents
Make HighestFramerate and HighestResolution constraints mandatory in livekit-capture/src/sources/device/avfoundation.rs. select_active_format must return an error when no candidate matches a supplied resolution or frame-rate constraint. best_device_format must not retain unsupported frame-rate candidates as an unconstrained fallback. Preserve unconstrained selection only when the corresponding request field is None.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut format = initial_frame.format; | ||
| format.framerate_fps = requested_framerate(&config.format).unwrap_or(30); | ||
| let target_resolution = requested_output_resolution(&config.format, format.resolution); |
There was a problem hiding this comment.
🟡 Highest frame rate remains unapplied
HighestFramerate selects by maximum capability but never configures that rate. format() also reports 30 fps regardless of the delivered rate.
Prompt for agents
Track the frame rate selected for HighestFramerate and apply its supported frame duration during device/input configuration. Session::open must report the negotiated rate rather than substituting 30 whenever the request lacks an explicit frame rate. The same reporting path must derive the actual delivered rate for Default and unconstrained HighestResolution requests.
Was this helpful? React with 👍 or 👎 to provide feedback.
510d689 to
374e4b4
Compare
374e4b4 to
cf55e8b
Compare
cf55e8b to
6335f2a
Compare
6335f2a to
6087dd2
Compare
cc7e5ec to
9b0e427
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Clock frames reverse capture time
ClockVideoSource emits 0 then 1_000; capture_frame replaces only zero with current epoch time. The second frame jumps backward by decades, corrupting WebRTC timing.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
9b0e427 to
e5e88a0
Compare
2 participants
Add an AVFoundation backend for device source.
Closes BOT-412