Skip to content

fix/feat: SystemTime instead of timestamp #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix/feat: SystemTime instead of timestamp #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 72 additions & 35 deletions src/capturer/engine/linux/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use std::{
mpsc::{self, sync_channel, SyncSender},
},
thread::JoinHandle,
time::Duration,
time::{Duration, SystemTime},
};

use pipewire as pw;
use pipewire::{self as pw, spa::sys::spa_meta_type, sys::pw_buffer};
use pw::{
context::Context,
main_loop::MainLoop,
Expand All @@ -31,7 +31,7 @@ use pw::{

use crate::{
capturer::Options,
frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, XBGRFrame},
frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, VideoFrame, XBGRFrame},
};

use self::{error::LinCapError, portal::ScreenCastPortal};
Expand Down Expand Up @@ -91,38 +91,68 @@ fn state_changed_callback(
}
}

unsafe fn get_timestamp(buffer: *mut spa_buffer) -> i64 {
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
}
Comment on lines +94 to +110
Copy link

@coderabbitai coderabbitai bot Oct 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.


unsafe fn get_timestamp_and_sequence(buffer: *mut spa_buffer) -> (i64, u64) {
let meta_header: Option<spa_meta_header> = find_meta_in_buffer(buffer, SPA_META_Header);

if let Some(meta_header) = meta_header {
(meta_header.pts, meta_header.seq)
} else {
0
(0, 0)
}
}

unsafe fn find_most_recent_buffer(stream: &StreamRef) -> *mut pw_buffer {
let mut buffer: *mut pw_buffer = std::ptr::null_mut();

loop {
let tmp = stream.dequeue_raw_buffer();
if tmp.is_null() {
break;
}

if !buffer.is_null() {
stream.queue_raw_buffer(buffer);
}

buffer = tmp;
}

buffer
}

fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) {
let buffer = unsafe { stream.dequeue_raw_buffer() };
if !buffer.is_null() {
let pw_buffer = unsafe { find_most_recent_buffer(stream) };
if !pw_buffer.is_null() {
'outside: {
Comment on lines +142 to 144
Copy link

@coderabbitai coderabbitai bot Oct 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

let buffer = unsafe { (*buffer).buffer };
let buffer = unsafe { (*pw_buffer).buffer };
if buffer.is_null() {
break 'outside;
}
let timestamp = unsafe { get_timestamp(buffer) };

let (timestamp, sequence) = unsafe { get_timestamp_and_sequence(buffer) };
let timestamp = SystemTime::UNIX_EPOCH + Duration::from_nanos(timestamp as u64);

let n_datas = unsafe { (*buffer).n_datas };
if n_datas < 1 {
return;
break 'outside;
}
let frame_size = user_data.format.size();
let frame_data: Vec<u8> = unsafe {
Expand All @@ -134,30 +164,38 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) {
};

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"),
Comment on lines 166 to 199
Copy link

@coderabbitai coderabbitai bot Oct 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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 -60

Length 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:

  1. Capture processed_time once before the match to ensure consistency
  2. Either add a VideoFormat::RGBA branch 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.

} {
eprintln!("{e}");
Expand All @@ -166,8 +204,7 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) {
} else {
eprintln!("Out of buffers");
}

unsafe { stream.queue_raw_buffer(buffer) };
unsafe { stream.queue_raw_buffer(pw_buffer) };
}

// TODO: Format negotiation
Expand Down
12 changes: 12 additions & 0 deletions src/frame/video.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ pub struct YUVFrame {
#[derive(Debug, Clone)]
pub struct RGBFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand All @@ -29,6 +31,8 @@ pub struct RGB8Frame {
#[derive(Debug, Clone)]
pub struct RGBxFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand All @@ -37,6 +41,8 @@ pub struct RGBxFrame {
#[derive(Debug, Clone)]
pub struct XBGRFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand All @@ -45,6 +51,8 @@ pub struct XBGRFrame {
#[derive(Debug, Clone)]
pub struct BGRxFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand All @@ -53,6 +61,8 @@ pub struct BGRxFrame {
#[derive(Debug, Clone)]
pub struct BGRFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand All @@ -61,6 +71,8 @@ pub struct BGRFrame {
#[derive(Debug, Clone)]
pub struct BGRAFrame {
pub display_time: SystemTime,
pub processed_time: SystemTime,
pub sequence: u64,
pub width: i32,
pub height: i32,
pub data: Vec<u8>,
Expand Down