Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } | ||
| // SAFETY: Source and destination slices are valid for the dimensions and strides. | ||
| let ret = unsafe { | ||
| yuv_sys::rs_RGB24ToI420( |
There was a problem hiding this comment.
🟡 MJPEG fallback path swaps red and blue
When rs_MJPGToI420 fails, the fallback decodes to R,G,B via to_rgb8() then calls rs_RGB24ToI420, which expects B,G,R. Frames decoded on this path come out with red and blue swapped. It should use rs_RAWToI420.
| yuv_sys::rs_RGB24ToI420( | |
| yuv_sys::rs_RAWToI420( |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn open_device(selector: &DeviceSelector) -> Result<Device, DeviceVideoSourceError> { | ||
| match selector { | ||
| DeviceSelector::Default => Device::new(0).map_err(open_error), | ||
| DeviceSelector::Index(index) => Device::new(*index).map_err(open_error), |
There was a problem hiding this comment.
🟡 Index selector opens wrong device node
open_device maps Index(n) to Device::new(n), opening /dev/video{n}, but the documented contract is the n-th device in enumeration order and devices() filters out non-capture nodes. On hardware exposing extra non-capture video nodes, Index(n) selects the wrong device; the macOS backend indexes enumeration order instead.
Prompt for agents
DeviceSelector::Index(n) is documented (in mod.rs) as selecting the device at position n in the platform enumeration order, and devices() filters context::enum_devices() down to VIDEO_CAPTURE-capable nodes. However open_device() implements Index(n) as Device::new(n), which opens /dev/video{n} directly rather than the n-th entry of the filtered enumeration. On systems where non-capture video nodes exist (metadata/output nodes interleaved with capture nodes), these two disagree, so Index selection can open the wrong or an invalid device, diverging from both the documented contract and the macOS backend (which uses .nth(index) over the enumeration order). Consider implementing Index by indexing into the same filtered enumeration used by devices() so the selector matches its documented meaning.
Was this helpful? React with 👍 or 👎 to provide feedback.
ladvoc
changed the title
1c3d018 to
a6ff484
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 |
40d3ca6 to
31eeea7
Compare
31eeea7 to
fdd1b7e
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.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // negotiated format actually delivers frames. | ||
| let first_frame = session.read_frame()?; | ||
| session.pending_frame = Some(first_frame); | ||
| log::info!("Opened device \"{}\": {} (converted to I420)", device_name, session.format,); |
There was a problem hiding this comment.
🟨 Device card name logged unescaped
On open, the device card string is logged with {} (log::info!). The repo rule requires device-provided strings be escaped ({:?}/escape_debug), since a card name with control characters can inject into log output.
Was this helpful? React with 👍 or 👎 to provide feedback.
alan-george-lk
left a comment
alan-george-lk
left a comment
There was a problem hiding this comment.
LGTM assuming AI flagged issues are fixed
| /// The request's own frame format (already validated as supported) is tried | ||
| /// first; an explicit constraint on the highest-* requests pins the list to | ||
| /// that one format. | ||
| fn frame_formats_for_request(request: &DeviceFormatRequest) -> Vec<DeviceFrameFormat> { |
There was a problem hiding this comment.
I'm a little out of my depths here Rust-wise and V4L2-wise, so I thought it might be useful to have another model check for errors while I learn more about the Rust code and V4L2. Take this with a grain of salt, and apologies in advance if low-usefulness:
P1 — Exact requests can silently fall back to a different pixel format.
frame_formats_for_request puts the requested format first but appends defaults. Then apply_ordered_format_request tries each one until one succeeds.
Example: Exact(1280×720, 30, Mjpeg) on a camera that cannot provide MJPEG but can provide YUYV will succeed as YUYV. Exact should instead return UnsupportedFormat.
Suggested comment:
P1: Exact requests are currently allowed to fall back to the default FourCC list. If the requested format is unavailable but a later fallback succeeds, construction returns a source in a different format instead of UnsupportedFormat. Could Exact pass only its requested frame_format to apply_ordered_format_request (or skip that fallback path entirely)? Please add a regression test where the requested FourCC is unavailable but a fallback FourCC is available. Rust angle: the code is type-safe, but its control flow changes a value the caller marked as “exact.” The compiler cannot infer that fallback is semantically forbidden.
P2 — stepwise V4L2 sizes are reported as a complete list even though only the endpoints are retained.
resolutions_from_frame_size converts a Stepwise range into only its minimum and maximum dimensions. Yet devices() marks that list formats_complete: true.
Stepwise means the driver supports a grid/range of intermediate sizes. A client asking for the “closest” resolution may be given an endpoint even when a much better intermediate size is supported; device enumeration also claims those valid sizes do not exist.
Suggested comment:
P2: FrameSizeEnum::Stepwise is reduced to min/max while formats_complete is set to true. This makes Closest choose poor endpoint-only results and misreports the device’s formats as complete. Could we retain enough stepwise metadata to negotiate the nearest aligned size, or set formats_complete to false whenever any format/interval is stepwise?
70d3627 to
093737c
Compare
093737c to
29d3264
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| FrameSizeEnum::Stepwise(stepwise) => { | ||
| let mut resolutions = Vec::new(); | ||
| push_stepwise_resolution( | ||
| &mut resolutions, | ||
| VideoResolution::new(stepwise.min_width, stepwise.min_height), | ||
| ); | ||
| push_stepwise_resolution( | ||
| &mut resolutions, | ||
| VideoResolution::new(stepwise.max_width, stepwise.max_height), | ||
| ); | ||
| resolutions |
There was a problem hiding this comment.
🟡 Stepwise camera modes disappear
resolutions_from_frame_size keeps only range endpoints, while enumeration reports a complete list. Valid intermediate modes become unavailable to selection and consumers.
Prompt for agents
V4L2 stepwise and continuous frame-size/frame-interval descriptors represent ranges, but enumerate_device_formats reduces them to only min/max and devices() marks the result complete. Preserve enough range information for request negotiation, or negotiate constrained requests directly with TRY_FMT/S_FMT and verify the result. DeviceInfo::formats_complete must remain false whenever the public flat DeviceFormat list cannot represent all valid intermediate modes. Apply the same treatment to stepwise frame intervals.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if let Some(frame) = self.pending_frame.take() { | ||
| return Ok(Some(frame)); |
There was a problem hiding this comment.
🟡 Stopped capture emits one frame
When pending_frame exists, next_frame returns it without checking the stop token. A stop racing the first read still publishes one frame.
| if let Some(frame) = self.pending_frame.take() { | |
| return Ok(Some(frame)); | |
| if stop.is_stopped() { | |
| return Ok(None); | |
| } | |
| if let Some(frame) = self.pending_frame.take() { | |
| return Ok(Some(frame)); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
29d3264 to
6497b5f
Compare
869ee09 to
e7b5689
Compare
e7b5689 to
e3eba14
Compare
f2d720f to
2386f1b
Compare
2386f1b to
e09ee03
Compare
e09ee03 to
3b6aff4
Compare
3b6aff4 to
b557e5d
Compare
2 participants
Add a V4L2 backend for device source.
Closes BOT-413