fix/feat: SystemTime instead of timestamp #178
Conversation
Added sequence numbers as well. Created function to grab latest buffer instead of latest ready one.
WalkthroughThe changes introduce a new Changes
Sequence DiagramsequenceDiagram
participant Stream as PipeWire Stream
participant Engine as Linux Engine
participant Buffer as Buffer
participant Meta as Metadata
participant Frame as VideoFrame
rect rgb(100, 200, 150)
Note over Engine: Buffer Processing
Engine->>Stream: Find most recent buffer
Stream-->>Engine: pw_buffer handle
Engine->>Buffer: Dequeue buffer
Buffer-->>Engine: Buffer data
end
rect rgb(150, 180, 220)
Note over Engine: Metadata Extraction
Engine->>Meta: Extract timestamp & sequence<br/>(find_meta_in_buffer)
Meta-->>Engine: pts, sequence
Engine->>Engine: Convert pts to display_time<br/>Capture processed_time
end
rect rgb(200, 150, 180)
Note over Engine: Frame Construction
Engine->>Frame: Construct VideoFrame<br/>(data, display_time, processed_time, sequence)
Frame-->>Engine: VideoFrame instance
Engine->>Engine: Wrap in Frame::Video(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/frame/video.rs (1)
17-18: Public API break: new required fields across video frame structs.Adding processed_time and sequence makes these structs non-backward compatible for all producers/consumers constructing them. Confirm all capture engines and downstream users are updated. Consider Option fields or constructors/builders to ease migration; also clarify whether YUVFrame/RGB8Frame should carry the same metadata for API consistency.
Also applies to: 34-35, 44-45, 54-55, 64-65, 74-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/capturer/engine/linux/mod.rs(5 hunks)src/frame/video.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/capturer/engine/linux/mod.rs (1)
src/frame/audio.rs (1)
time(68-70)
🔇 Additional comments (3)
src/capturer/engine/linux/mod.rs (3)
122-140: Latest-buffer dequeue logic LGTM; watch for CPU churn under backlog.Strategy correctly drains to the freshest buffer and re-queues intermediates. Monitor callback CPU if backlogs are common; consider a bounded drain count or metrics if needed.
291-302: Good: explicitly request SPA_META_Header.Advertising spa_meta_header ensures pts/seq availability. This aligns with the new metadata extraction.
150-152: Review comment is based on incorrect assumptions about PipeWire semantics.PipeWire's spa_meta_header.pts field is a 64-bit signed integer containing presentation timestamp in nanoseconds, but PipeWire timestamps are epoch-based on the system monotonic clock (CLOCK_MONOTONIC), expressed in nanoseconds since boot. Since CLOCK_MONOTONIC is always non-negative and monotonically increasing, the i64-to-u64 cast at line 151 will never wrap. The fallback value is 0 (when no meta header exists), which is also valid.
The current code is correct and safe. Defensive checks for negative/zero values are unnecessary because PipeWire's protocol guarantees non-negative timestamps; a negative pts would indicate a bug in PipeWire itself, not something the application should work around.
Likely an incorrect or invalid review comment.
| unsafe fn find_meta_in_buffer<T: Copy>(buffer: *mut spa_buffer, type_: spa_meta_type) -> Option<T> { | ||
| let n_metas = (*buffer).n_metas; | ||
| if n_metas > 0 { | ||
| let mut meta_ptr = (*buffer).metas; | ||
| let metas_end = (*buffer).metas.wrapping_add(n_metas as usize); | ||
| while meta_ptr != metas_end { | ||
| if (*meta_ptr).type_ == SPA_META_Header { | ||
| let meta_header: &mut spa_meta_header = | ||
| &mut *((*meta_ptr).data as *mut spa_meta_header); | ||
| return meta_header.pts; | ||
| } | ||
| meta_ptr = meta_ptr.wrapping_add(1); | ||
| let mut meta_ptr = (*buffer).metas; | ||
| let metas_end = (*buffer).metas.wrapping_add(n_metas as usize); | ||
|
|
||
| while meta_ptr != metas_end { | ||
| if (*meta_ptr).type_ == type_ { | ||
| let target: T = *((*meta_ptr).data as *mut T); | ||
|
|
||
| return Some(target); | ||
| } | ||
| 0 | ||
|
|
||
| meta_ptr = meta_ptr.wrapping_add(1); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
UB risk: unchecked meta->data deref and missing size/alignment validation.
find_meta_in_buffer dereferences meta->data without verifying it's non-null or large enough for T, and assumes alignment. This can read invalid memory.
Apply this safer pattern:
-unsafe fn find_meta_in_buffer<T: Copy>(buffer: *mut spa_buffer, type_: spa_meta_type) -> Option<T> {
+unsafe fn find_meta_in_buffer<T: Copy>(buffer: *mut spa_buffer, type_: spa_meta_type) -> Option<T> {
let n_metas = (*buffer).n_metas;
let mut meta_ptr = (*buffer).metas;
let metas_end = (*buffer).metas.wrapping_add(n_metas as usize);
while meta_ptr != metas_end {
if (*meta_ptr).type_ == type_ {
- let target: T = *((*meta_ptr).data as *mut T);
- return Some(target);
+ // Validate pointer and size; use unaligned read to be safe.
+ if !(*meta_ptr).data.is_null()
+ && ((*meta_ptr).size as usize) >= std::mem::size_of::<T>()
+ {
+ let target = std::ptr::read_unaligned((*meta_ptr).data as *const T);
+ return Some(target);
+ }
}
meta_ptr = meta_ptr.wrapping_add(1);
}
None
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe fn find_meta_in_buffer<T: Copy>(buffer: *mut spa_buffer, type_: spa_meta_type) -> Option<T> { | |
| let n_metas = (*buffer).n_metas; | |
| if n_metas > 0 { | |
| let mut meta_ptr = (*buffer).metas; | |
| let metas_end = (*buffer).metas.wrapping_add(n_metas as usize); | |
| while meta_ptr != metas_end { | |
| if (*meta_ptr).type_ == SPA_META_Header { | |
| let meta_header: &mut spa_meta_header = | |
| &mut *((*meta_ptr).data as *mut spa_meta_header); | |
| return meta_header.pts; | |
| } | |
| meta_ptr = meta_ptr.wrapping_add(1); | |
| let mut meta_ptr = (*buffer).metas; | |
| let metas_end = (*buffer).metas.wrapping_add(n_metas as usize); | |
| while meta_ptr != metas_end { | |
| if (*meta_ptr).type_ == type_ { | |
| let target: T = *((*meta_ptr).data as *mut T); | |
| return Some(target); | |
| } | |
| 0 | |
| meta_ptr = meta_ptr.wrapping_add(1); | |
| } | |
| None | |
| } | |
| unsafe fn find_meta_in_buffer<T: Copy>(buffer: *mut spa_buffer, type_: spa_meta_type) -> Option<T> { | |
| let n_metas = (*buffer).n_metas; | |
| let mut meta_ptr = (*buffer).metas; | |
| let metas_end = (*buffer).metas.wrapping_add(n_metas as usize); | |
| while meta_ptr != metas_end { | |
| if (*meta_ptr).type_ == type_ { | |
| // Validate pointer and size; use unaligned read to be safe. | |
| if !(*meta_ptr).data.is_null() | |
| && ((*meta_ptr).size as usize) >= std::mem::size_of::<T>() | |
| { | |
| let target = std::ptr::read_unaligned((*meta_ptr).data as *const T); | |
| return Some(target); | |
| } | |
| } | |
| meta_ptr = meta_ptr.wrapping_add(1); | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
In src/capturer/engine/linux/mod.rs around lines 94 to 110, the function
unsafely dereferences meta->data without null/size/alignment checks; update the
loop to first ensure (*meta_ptr).data is not null and that the metadata length
(e.g., (*meta_ptr).size or equivalent field) is >= std::mem::size_of::<T>(),
then safely load the value using ptr::read_unaligned or copy into a
std::mem::MaybeUninit<T> (avoiding alignment UB) instead of directly
dereferencing; only after these checks return Some(value), otherwise continue
the loop.
| let pw_buffer = unsafe { find_most_recent_buffer(stream) }; | ||
| if !pw_buffer.is_null() { | ||
| 'outside: { |
There was a problem hiding this comment.
Null pw_buffer re-queued when no buffers available.
When find_most_recent_buffer returns null, queue_raw_buffer(pw_buffer) still runs, passing null into FFI.
Apply this guard:
- unsafe { stream.queue_raw_buffer(pw_buffer) };
+ if !pw_buffer.is_null() {
+ unsafe { stream.queue_raw_buffer(pw_buffer) };
+ }Also applies to: 207-207
🤖 Prompt for AI Agents
In src/capturer/engine/linux/mod.rs around lines 142-144 (and similarly at line
207), find_most_recent_buffer may return a null pointer but later code still
calls queue_raw_buffer(pw_buffer); add a guard that checks pw_buffer for null
immediately after the call and skip (or return from) the re-queue/FFI call when
pw_buffer.is_null(), ensuring no null pointer is passed into queue_raw_buffer;
apply the same null-check-and-skip fix at the other occurrence around line 207.
| if let Err(e) = match user_data.format.format() { | ||
| VideoFormat::RGBx => user_data.tx.send(Frame::RGBx(RGBxFrame { | ||
| display_time: timestamp as u64, | ||
| VideoFormat::RGBx => user_data.tx.send(Frame::Video(VideoFrame::RGBx(RGBxFrame { | ||
| display_time: timestamp, | ||
| processed_time: SystemTime::now(), | ||
| sequence, | ||
| width: frame_size.width as i32, | ||
| height: frame_size.height as i32, | ||
| data: frame_data, | ||
| })), | ||
| VideoFormat::RGB => user_data.tx.send(Frame::RGB(RGBFrame { | ||
| display_time: timestamp as u64, | ||
| }))), | ||
| VideoFormat::RGB => user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame { | ||
| display_time: timestamp, | ||
| processed_time: SystemTime::now(), | ||
| sequence, | ||
| width: frame_size.width as i32, | ||
| height: frame_size.height as i32, | ||
| data: frame_data, | ||
| })), | ||
| VideoFormat::xBGR => user_data.tx.send(Frame::XBGR(XBGRFrame { | ||
| display_time: timestamp as u64, | ||
| }))), | ||
| VideoFormat::xBGR => user_data.tx.send(Frame::Video(VideoFrame::XBGR(XBGRFrame { | ||
| display_time: timestamp, | ||
| processed_time: SystemTime::now(), | ||
| sequence, | ||
| width: frame_size.width as i32, | ||
| height: frame_size.height as i32, | ||
| data: frame_data, | ||
| })), | ||
| VideoFormat::BGRx => user_data.tx.send(Frame::BGRx(BGRxFrame { | ||
| display_time: timestamp as u64, | ||
| }))), | ||
| VideoFormat::BGRx => user_data.tx.send(Frame::Video(VideoFrame::BGRx(BGRxFrame { | ||
| display_time: timestamp, | ||
| processed_time: SystemTime::now(), | ||
| sequence, | ||
| width: frame_size.width as i32, | ||
| height: frame_size.height as i32, | ||
| data: frame_data, | ||
| })), | ||
| }))), | ||
| _ => panic!("Unsupported frame format received"), |
There was a problem hiding this comment.
🧩 Analysis chain
Minor: compute processed_time once per frame; also handle RGBA or remove it from negotiation.
- processed_time is called multiple times; capture once to ensure consistency within the frame build.
- RGBA is advertised in negotiation but not matched here—this will panic on RGBA selection.
Apply:
- if let Err(e) = match user_data.format.format() {
- VideoFormat::RGBx => user_data.tx.send(Frame::Video(VideoFrame::RGBx(RGBxFrame {
- display_time: timestamp,
- processed_time: SystemTime::now(),
+ let processed = SystemTime::now();
+ if let Err(e) = match user_data.format.format() {
+ VideoFormat::RGBx => user_data.tx.send(Frame::Video(VideoFrame::RGBx(RGBxFrame {
+ display_time: timestamp,
+ processed_time: processed,
sequence,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: frame_data,
}))),
- VideoFormat::RGB => user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame {
- display_time: timestamp,
- processed_time: SystemTime::now(),
+ VideoFormat::RGB => user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame {
+ display_time: timestamp,
+ processed_time: processed,
sequence,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: frame_data,
}))),
- VideoFormat::xBGR => user_data.tx.send(Frame::Video(VideoFrame::XBGR(XBGRFrame {
- display_time: timestamp,
- processed_time: SystemTime::now(),
+ VideoFormat::xBGR => user_data.tx.send(Frame::Video(VideoFrame::XBGR(XBGRFrame {
+ display_time: timestamp,
+ processed_time: processed,
sequence,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: frame_data,
}))),
- VideoFormat::BGRx => user_data.tx.send(Frame::Video(VideoFrame::BGRx(BGRxFrame {
- display_time: timestamp,
- processed_time: SystemTime::now(),
+ VideoFormat::BGRx => user_data.tx.send(Frame::Video(VideoFrame::BGRx(BGRxFrame {
+ display_time: timestamp,
+ processed_time: processed,
sequence,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: frame_data,
}))),
_ => panic!("Unsupported frame format received"),
} {Additionally, either add a VideoFormat::RGBA branch or remove RGBA from the negotiated formats to prevent panic. Example (remove RGBA):
- pw::spa::param::video::VideoFormat::RGBA,I'll verify the claims in this review comment by examining the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for RGBA references in the Linux module
rg -n "RGBA" src/capturer/engine/linux/mod.rs
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Find the negotiation code and context around RGBA advertising
rg -n -B5 -A5 "pw::spa::param::video::VideoFormat" src/capturer/engine/linux/mod.rs | head -60Length of output: 524
🏁 Script executed:
#!/bin/bash
# Get context around the match statement to confirm RGBA is not handled
cat src/capturer/engine/linux/mod.rs | sed -n '150,210p'Length of output: 2538
CRITICAL: RGBA negotiation/handling mismatch causes panic at runtime.
The review comment is verified as accurate. RGBA is advertised in format negotiation (line 256) but not handled in the frame format match statement (lines 166–195), which will panic with "Unsupported frame format received" if RGBA is selected.
The suggested fixes are correct:
- Capture
processed_timeonce before the match to ensure consistency - Either add a
VideoFormat::RGBAbranch or remove RGBA from the negotiated formats (line 256)
Apply the provided diff to resolve both issues.
🤖 Prompt for AI Agents
In src/capturer/engine/linux/mod.rs around lines 166 to 199, the match over
user_data.format currently omits VideoFormat::RGBA and captures processed_time
inside each arm, causing a panic when RGBA is negotiated; capture let
processed_time = SystemTime::now() once before the match to use consistently,
and add a VideoFormat::RGBA arm that sends
Frame::Video(VideoFrame::RGBA(RGBAFrame { display_time: timestamp,
processed_time, sequence, width: frame_size.width as i32, height:
frame_size.height as i32, data: frame_data })) (mirroring the other frame arms),
or alternatively remove RGBA from the offered negotiation at line ~256 if you
intentionally do not support RGBA.
1 participant
Added sequence numbers as well.
Created function to grab latest buffer instead of latest ready one.
Summary by CodeRabbit