dailz/wl-webrtc
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

chore: clear clippy errors, document all unsafe blocks, deny new SAFETY debt

Browse Source 
Audit-driven cleanup pass. End state:
  - cargo clippy --release --all-targets: 0 errors (was 4)
  - undocumented_unsafe_blocks warnings: 0 (was 67)
  - Cargo.toml: undocumented_unsafe_blocks escalated warn -> deny

Clippy correctness errors fixed:
  - src/bin/{sw_encode_bench,vaapi_import_bench}.rs: receive_first_frame
    rewritten per Oracle plan with total 10s deadline + 200ms wait slice +
    while-let drain of all control events. The previous loop body always
    exited on first iteration (never_loop); the new version actually retries
    and matches production's repeated-poll semantics in state_portal.rs.
  - src/avhw.rs: hash_sampled_y_plane tests now use a row_range(row, stride,
    width) helper instead of inline stride * N. Preserves the row-index
    intent across all sibling tests without tripping erasing_op (row==0) or
    identity_op (row==1).

Machine-applicable clippy autofixes applied via 'cargo clippy --fix':
  - unnecessary_cast, manual_is_multiple_of, needless_borrows_for_generic_args
  - manual_abs_diff, derivable_impls, new_without_default
  - unnecessary_map_or, unneeded_struct_pattern, redundant_locals

webrtc_gop_formula test rewritten to wrap the (fps * 2).max(20) formula in
a runtime lambda. The previous clippy --fix pass had constant-folded the
5fps case into assert_eq!(20, 20), silently stripping the floor-case
coverage. The lambda blocks the fold while keeping the formula exercisable.

67 SAFETY comments added across 7 files (cap_portal.rs 26, sw_encode_bench
21, state_portal.rs 7, vaapi_import_bench.rs 6, avhw.rs 5, state.rs 1,
main.rs 1). Two sites carry load-bearing invariant documentation:
  - cap_portal.rs:806 process callback documents the PipeWire raw_buf
    ownership contract across all 10 exit paths (audited: every path
    correctly requeues; fd ownership via dup() is independent and also
    exactly-once closed).
  - avhw.rs:341 unsafe impl Send for EncState documents the single-thread
    exclusivity assumption referenced by AGENTS.md.

All 97 unit tests + 3 integration tests still pass; cargo build --release
finishes clean. Lint escalation to deny freezes the SAFETY baseline: any
future patch adding an unsafe block without a // SAFETY: comment will fail
clippy at compile time.
This commit is contained in:
dailz
2026-06-28 13:44:27 +08:00
parent e7accecfec
commit 30f8fe51f2
10 changed files with 282 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/bin/sw_encode_bench.rs
+73 -9
View File
@@ -62,22 +62,32 @@ fn pix_fmt(p: ff::format::Pixel) -> ffi::AVPixelFormat {
}
fn receive_first_frame(cap: &CapPortal) -> Result<wl_webrtc::cap_portal::PwDmaBufFrame> {
// Drain-and-wait loop that mirrors production's repeated-poll semantics
// (state_portal.rs::poll_and_encode driven by main.rs's outer loop), but with
// a single bounded 10s total deadline appropriate for a bench tool. Unlike a
// single 10s blocking wait, this loop actually iterates: each turn drains ALL
// pending control events (the ctrl channel is bounded to 8 — a single
// if-let would silently miss backlog) and then waits a short slice for a
// frame, so StreamEnded/Error arriving mid-wait are observed within ~200ms.
const TOTAL_DEADLINE: std::time::Duration = std::time::Duration::from_secs(10);
const WAIT_SLICE: std::time::Duration = std::time::Duration::from_millis(200);
let deadline = Instant::now() + TOTAL_DEADLINE;
loop {
if let Ok(ctrl) = cap.event_receiver().try_recv() {
while let Ok(ctrl) = cap.event_receiver().try_recv() {
match ctrl {
PwCtrlEvent::StreamEnded => bail!("PipeWire stream ended before first frame"),
PwCtrlEvent::FormatChanged { .. } => {}
PwCtrlEvent::Error(e) => bail!("PipeWire error: {e}"),
}
}
match cap
.frame_receiver()
.recv_timeout(std::time::Duration::from_secs(10))
{
let remaining = match deadline.checked_duration_since(Instant::now()) {
Some(r) if !r.is_zero() => r,
_ => bail!("Timeout waiting for first frame (10s)"),
};
let slice = remaining.min(WAIT_SLICE);
match cap.frame_receiver().recv_timeout(slice) {
Ok(frame) => return Ok(frame),
Err(crossbeam_channel::RecvTimeoutError::Timeout) => {
bail!("Timeout waiting for first frame (10s)");
}
Err(crossbeam_channel::RecvTimeoutError::Timeout) => continue,
Err(crossbeam_channel::RecvTimeoutError::Disconnected) => {
bail!("PipeWire frame channel disconnected");
}
@@ -142,6 +152,9 @@ fn main() -> Result<()> {
println!("[3/4] Testing mmap on DMA-BUF...");
let mmap_size = (src_stride as usize) * (src_height as usize);
// SAFETY: first_frame.fd is an open DMA-BUF; offset/size come from PipeWire's
// negotiated format. PROT_READ+MAP_SHARED is the standard read-only DMA-BUF
// mapping. Returns MAP_FAILED on error (checked below).
let mmap_ptr = unsafe {
libc::mmap(
ptr::null_mut(),
@@ -173,6 +186,8 @@ fn main() -> Result<()> {
"[3/4] mmap SUCCESS — CPU can read DMA-BUF ({:.1} MB)\n",
mmap_size as f64 / 1024.0 / 1024.0
);
// SAFETY: mmap_ptr was returned by mmap above and is not MAP_FAILED (checked);
// mmap_size matches the original mapping. POSIX munmap(2) releases the mapping.
unsafe {
libc::munmap(mmap_ptr, mmap_size);
}
@@ -205,6 +220,9 @@ fn main() -> Result<()> {
let codec_name = codec.name();
if codec_name == "libx264" {
// SAFETY: enc is a valid AVCodecContext for the not-yet-opened encoder;
// priv_data is the x264 private options struct. All CStrings live across
// both av_opt_set calls. These set the x264 "preset" and "tune" options.
unsafe {
let key = CString::new("preset").unwrap();
let val = CString::new("veryfast").unwrap();
@@ -220,6 +238,8 @@ fn main() -> Result<()> {
// Create output format context via FFI
let mut fmt_ctx_ptr: *mut ffi::AVFormatContext = ptr::null_mut();
// SAFETY: fmt_ctx_ptr is an out-parameter initialized by FFmpeg; output_cstr
// lives across the call. Returns 0 on success; we check below.
let ret = unsafe {
ffi::avformat_alloc_output_context2(
&mut fmt_ctx_ptr,
@@ -232,21 +252,30 @@ fn main() -> Result<()> {
bail!("Failed to allocate output format context: error {ret}");
}
// SAFETY: fmt_ctx_ptr is the valid output context allocated above.
// avformat_new_stream returns a pointer to a new AVStream or NULL on failure.
let stream_ptr = unsafe { ffi::avformat_new_stream(fmt_ctx_ptr, ptr::null()) };
if stream_ptr.is_null() {
bail!("Failed to create new stream");
}
// SAFETY: stream_ptr and enc_video.as_ptr() are valid pointers; codecpar is
// the output destination inside stream. avcodec_parameters_from_context copies
// encoder parameters into the stream's codecpar.
let ret =
unsafe { ffi::avcodec_parameters_from_context((*stream_ptr).codecpar, enc_video.as_ptr()) };
if ret < 0 {
bail!("Failed to copy encoder parameters: error {ret}");
}
// SAFETY: stream_ptr and enc_video are valid; time_base is a plain AVRational
// field copied from encoder to stream.
unsafe {
(*stream_ptr).time_base = (*enc_video.as_ptr()).time_base;
}
// SAFETY: fmt_ctx_ptr is valid; pb is the AVIOContext slot to initialize;
// output_cstr is a valid NUL-terminated path; AVIO_FLAG_WRITE is a constant.
let ret = unsafe {
ffi::avio_open(
&mut (*fmt_ctx_ptr).pb,
@@ -261,17 +290,23 @@ fn main() -> Result<()> {
);
}
// SAFETY: fmt_ctx_ptr is fully configured (streams + pb set); NULL options
// is the default. Returns 0 on success.
let ret = unsafe { ffi::avformat_write_header(fmt_ctx_ptr, ptr::null_mut()) };
if ret < 0 {
bail!("Failed to write header: error {ret}");
}
// SAFETY: fmt_ctx_ptr is a fully initialized output context (header written).
// Output::wrap takes ownership of the pointer into a safe RAII wrapper.
let mut octx = unsafe { ff::format::context::Output::wrap(fmt_ctx_ptr) };
// Create sws_scale context: BGRZ (BGR0) -> YUV420P
let bgr0_fmt = pix_fmt(ff::format::Pixel::BGRZ);
let yuv420p_fmt = pix_fmt(ff::format::Pixel::YUV420P);
// SAFETY: all parameters are valid enum/pixel format values; NULL filters are
// allowed by FFmpeg. sws_getContext returns a heap-allocated SwsContext or NULL.
let sws_ctx = unsafe {
ffi::sws_getContext(
src_width as i32,
@@ -291,6 +326,9 @@ fn main() -> Result<()> {
}
// Allocate reusable YUV frame
// SAFETY: av_frame_alloc returns NULL only on OOM. After allocation we set
// width/height/format fields and call av_frame_get_buffer to allocate plane
// data. On failure we free the frame via av_frame_free before bailing.
let mut yuv_frame = unsafe {
let mut f = ffi::av_frame_alloc();
if f.is_null() {
@@ -349,6 +387,9 @@ fn main() -> Result<()> {
let mmap_start = Instant::now();
let frame_size = (frame.stride as usize) * (frame.height as usize);
// SAFETY: frame.fd is an open DMA-BUF owned by the frame; offset/size come
// from PipeWire's negotiated format. PROT_READ+MAP_SHARED for read-only
// DMA-BUF access. Returns MAP_FAILED on error (checked below).
let mmap_ptr = unsafe {
libc::mmap(
ptr::null_mut(),
@@ -369,8 +410,15 @@ fn main() -> Result<()> {
stats.mmap_us.push(mmap_start.elapsed().as_micros() as u64);
let scale_start = Instant::now();
// SAFETY: mmap_ptr is a valid mapping of frame_size bytes (checked above);
// constructing a read-only slice over it for the duration of sws_scale is
// sound as long as we don't hold it past munmap (we don't).
let src_data = unsafe { std::slice::from_raw_parts(mmap_ptr as *const u8, frame_size) };
// SAFETY: yuv_frame and sws_ctx are valid; src_data is a valid slice of the
// mmap'd DMA-BUF for this frame. sws_scale reads src planes (BGR0 -> YUV420P)
// and writes into yuv_frame's data planes. av_frame_make_writable ensures
// yuv_frame is not shared before writing.
unsafe {
ffi::av_frame_make_writable(yuv_frame);
@@ -391,6 +439,8 @@ fn main() -> Result<()> {
.scale_us
.push(scale_start.elapsed().as_micros() as u64);
// SAFETY: mmap_ptr was returned by mmap above and is not MAP_FAILED; frame_size
// matches the original mapping. Release before dropping frame (which closes fd).
unsafe {
libc::munmap(mmap_ptr, frame_size);
}
@@ -398,6 +448,9 @@ fn main() -> Result<()> {
let encode_start = Instant::now();
// SAFETY: yuv_frame is allocated and writable; enc_video is the opened encoder.
// Setting pts is a plain i64 field write. avcodec_send_frame submits the frame
// for encoding; returns < 0 on error (we log and continue).
unsafe {
(*yuv_frame).pts = pts;
pts += 1;
@@ -419,7 +472,7 @@ fn main() -> Result<()> {
.push(frame_start.elapsed().as_micros() as u64);
frames_encoded += 1;
if frames_encoded % 30 == 0 {
if frames_encoded.is_multiple_of(30) {
let fps = frames_encoded as f64 / total_start.elapsed().as_secs_f64();
println!(
" [{}/{}] {:.1} FPS",
@@ -431,6 +484,8 @@ fn main() -> Result<()> {
let total_elapsed = total_start.elapsed();
println!("\nFlushing encoder...");
// SAFETY: enc_video is the opened encoder; passing NULL frame signals EOF to
// drain the encoder's internal pipeline. Returns < 0 on error (ignored here).
unsafe {
ffi::avcodec_send_frame(enc_video.as_mut_ptr(), ptr::null());
}
@@ -440,6 +495,9 @@ fn main() -> Result<()> {
.map_err(|e| anyhow::anyhow!("Failed to write trailer: {e}"))?;
// Cleanup
// SAFETY: yuv_frame is the allocated frame from earlier (still owned by us);
// sws_ctx is the allocated sws context. av_frame_free and sws_freeContext take
// ownership and free their respective heap allocations.
unsafe {
ffi::av_frame_free(&mut yuv_frame as *mut _);
ffi::sws_freeContext(sws_ctx);
@@ -526,6 +584,9 @@ fn drain_encoder(
) -> Result<()> {
loop {
let mut pkt = ff::Packet::empty();
// SAFETY: enc_video is the opened encoder; pkt is an empty Packet whose
// inner AVPacket pointer is valid. avcodec_receive_packet fills pkt with
// the next encoded packet, or returns EAGAIN/EOF when drained.
let ret = unsafe { ffi::avcodec_receive_packet(enc_video.as_mut_ptr(), pkt.as_mut_ptr()) };
if ret < 0 {
if ret == ffi::AVERROR(ffi::EAGAIN) || ret == ffi::AVERROR_EOF {
@@ -536,6 +597,9 @@ fn drain_encoder(
}
let enc_tb = enc_video.time_base();
// SAFETY: octx.as_ptr() is a valid AVFormatContext; streams is a NULL-terminated
// array of AVStream*. We index [0] which exists because we created exactly one
// stream in setup. Reading time_base is a plain AVRational field access.
let stream_tb = unsafe {
let streams = (*octx.as_ptr()).streams;
let st = *streams.add(0);
src/bin/vaapi_import_bench.rs
+39 -10
View File
@@ -126,6 +126,9 @@ impl Drop for SwsContext {
fn av_err_to_string(ret: i32) -> String {
let mut buf = vec![0u8; 128];
// SAFETY: buf is a 128-byte Vec initialized to zeros; av_strerror writes at most
// buf.len() bytes (including NUL) into the buffer. The ret value is an FFmpeg
// error code. We treat the buffer as `*mut i8` for the C string out-param.
unsafe {
ffi::av_strerror(ret, buf.as_mut_ptr() as *mut i8, buf.len());
}
@@ -134,22 +137,32 @@ fn av_err_to_string(ret: i32) -> String {
}
fn receive_first_frame(cap: &CapPortal) -> Result<wl_webrtc::cap_portal::PwDmaBufFrame> {
// Drain-and-wait loop that mirrors production's repeated-poll semantics
// (state_portal.rs::poll_and_encode driven by main.rs's outer loop), but with
// a single bounded 10s total deadline appropriate for a bench tool. Unlike a
// single 10s blocking wait, this loop actually iterates: each turn drains ALL
// pending control events (the ctrl channel is bounded to 8 — a single
// if-let would silently miss backlog) and then waits a short slice for a
// frame, so StreamEnded/Error arriving mid-wait are observed within ~200ms.
const TOTAL_DEADLINE: std::time::Duration = std::time::Duration::from_secs(10);
const WAIT_SLICE: std::time::Duration = std::time::Duration::from_millis(200);
let deadline = Instant::now() + TOTAL_DEADLINE;
loop {
if let Ok(ctrl) = cap.event_receiver().try_recv() {
while let Ok(ctrl) = cap.event_receiver().try_recv() {
match ctrl {
PwCtrlEvent::StreamEnded => bail!("PipeWire stream ended before first frame"),
PwCtrlEvent::FormatChanged { .. } => {}
PwCtrlEvent::Error(e) => bail!("PipeWire error: {e}"),
}
}
match cap
.frame_receiver()
.recv_timeout(std::time::Duration::from_secs(10))
{
let remaining = match deadline.checked_duration_since(Instant::now()) {
Some(r) if !r.is_zero() => r,
_ => bail!("Timeout waiting for first frame (10s)"),
};
let slice = remaining.min(WAIT_SLICE);
match cap.frame_receiver().recv_timeout(slice) {
Ok(frame) => return Ok(frame),
Err(crossbeam_channel::RecvTimeoutError::Timeout) => {
bail!("Timeout waiting for first frame (10s)");
}
Err(crossbeam_channel::RecvTimeoutError::Timeout) => continue,
Err(crossbeam_channel::RecvTimeoutError::Disconnected) => {
bail!("PipeWire frame channel disconnected");
}
@@ -163,6 +176,9 @@ fn drain_encoder(
) -> Result<()> {
loop {
let mut pkt = ff::Packet::empty();
// SAFETY: enc_video is the opened encoder; pkt is an empty Packet whose inner
// AVPacket pointer is valid. avcodec_receive_packet fills pkt with the next
// encoded packet or returns EAGAIN/EOF when drained.
let ret = unsafe { ffi::avcodec_receive_packet(enc_video.as_mut_ptr(), pkt.as_mut_ptr()) };
if ret < 0 {
if ret == ffi::AVERROR(ffi::EAGAIN) || ret == ffi::AVERROR_EOF {
@@ -172,6 +188,9 @@ fn drain_encoder(
break;
}
let enc_tb = enc_video.time_base();
// SAFETY: octx.as_ptr() is a valid AVFormatContext; streams is a NULL-terminated
// array; we index [0] which exists because we created exactly one stream in
// setup. Reading time_base is a plain AVRational field access.
let stream_tb = unsafe {
let streams = (*octx.as_ptr()).streams;
let st = *streams.add(0);
@@ -596,7 +615,7 @@ fn run_cpu_pipeline(
stats.total_us.push(total_us);
stats.frames_encoded += 1;
if stats.frames_encoded <= 3 || stats.frames_encoded % 30 == 0 {
if stats.frames_encoded <= 3 || stats.frames_encoded.is_multiple_of(30) {
println!(
" CPU frame {:>4}/{frames}: import={:.2}ms transfer={:.2}ms scale={:.2}ms encode={:.2}ms total={:.2}ms",
stats.frames_encoded,
@@ -754,7 +773,7 @@ fn run_gpu_pipeline(
stats.total_us.push(total_us);
stats.frames_encoded += 1;
if stats.frames_encoded <= 3 || stats.frames_encoded % 30 == 0 {
if stats.frames_encoded <= 3 || stats.frames_encoded.is_multiple_of(30) {
println!(
" GPU frame {:>4}/{frames}: import={:.2}ms filter={:.2}ms transfer={:.2}ms format={:.2}ms encode={:.2}ms total={:.2}ms",
stats.frames_encoded,
@@ -919,6 +938,11 @@ fn main() -> Result<()> {
AvHwFrameCtx::for_capture(&hw_dev, src_width, src_height, ff::format::Pixel::BGRA)?;
println!(" VAAPI frames context created OK (sw_format=BGRA)");
// SAFETY: delegates to avhw::import_dma_buf_to_vaapi (itself an unsafe fn).
// frames_ctx is a valid AVBufferRef from AvHwFrameCtx::for_capture above;
// first_frame's fd/width/height/format/modifier/stride/offset are all sourced
// from the PipeWire-formatted PwDmaBufFrame. See that function's own SAFETY
// contract for the full rationale.
let vaapi_frame = unsafe {
import_dma_buf_to_vaapi(
frames_ctx.as_ptr(),
@@ -949,6 +973,9 @@ fn main() -> Result<()> {
let mmap_size = (first_frame.stride as usize) * (first_frame.height as usize);
let mmap_start = Instant::now();
// SAFETY: first_frame.fd is an open DMA-BUF; offset/size from PipeWire.
// PROT_READ+MAP_SHARED is the standard read-only DMA-BUF mapping. Returns
// MAP_FAILED on error (checked below).
let mmap_ptr = unsafe {
libc::mmap(
ptr::null_mut(),
@@ -970,6 +997,8 @@ fn main() -> Result<()> {
mmap_size as f64 / 1024.0 / 1024.0,
mmap_elapsed.as_secs_f64() * 1000.0
);
// SAFETY: mmap_ptr is a valid mapping (MAP_FAILED path was handled
// above); mmap_size matches the original mapping. POSIX munmap(2).
unsafe {
libc::munmap(mmap_ptr, mmap_size);
}