Repository navigation
Conversation
Linear PipeWire frames are read-only views of the producer buffer until the last frame reference drops, so the encoder can finish without a per-frame row copy. DMA-BUF mappings stay cached while the fd and size are stable. Instant-with-camera still copies because the alias is read-only and av_frame_make_writable allocates private pixels before the camera blend. A private copy is also used when the buffer pool has no spare slot.
|
@greptileai review |
|
@kyzer1023 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let plane = if (dmabuf || memfd) && fd >= 0 { | ||
| SourcePlane { | ||
| data: std::ptr::null_mut(), | ||
| data_len: 0, | ||
| fd, | ||
| sync_dma_buf: dmabuf, | ||
| pw_buffer, | ||
| } |
There was a problem hiding this comment.
If a MemFd buffer has a nonzero spa_data.mapoffset, this new path reads the wrong part of the file. It ignores the already-mapped pointer and maps the fd from byte zero, then adds only chunk.offset, which is relative to the producer's mapped region. Both the borrowed-frame and copy paths therefore record incorrect pixels. Preserve the producer mapping or carry mapoffset through the mapping, bounds, and cache key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/screen_capture/linux.rs
Line: 1016-1023
Comment:
**Frames read the wrong bytes**
If a `MemFd` buffer has a nonzero `spa_data.mapoffset`, this new path reads the wrong part of the file. It ignores the already-mapped pointer and maps the fd from byte zero, then adds only `chunk.offset`, which is relative to the producer's mapped region. Both the borrowed-frame and copy paths therefore record incorrect pixels. Preserve the producer mapping or carry `mapoffset` through the mapping, bounds, and cache key.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let _ = std::fs::create_dir_all("/opt/cursor/artifacts"); | ||
| let _ = std::fs::write("/opt/cursor/artifacts/pipewire-alias-4k.log", &report); | ||
| assert!(copy_ms.is_finite() && alias_ms.is_finite()); | ||
| assert!(alias_ms < copy_ms); |
There was a problem hiding this comment.
four_k_bgra_alias_avoids_the_row_copy_and_keeps_the_mapping runs as an ordinary unit test, but passing depends on one short timing sample set beating another. A scheduler pause during the borrowed-frame samples can fail the suite even when the buffer behavior is correct. Keep the pointer and mapping-count checks in the unit test, and move the timing comparison to an ignored benchmark.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/screen_capture/linux_pipewire_frames.rs
Line: 1018
Comment:
**Timing makes tests flaky**
`four_k_bgra_alias_avoids_the_row_copy_and_keeps_the_mapping` runs as an ordinary unit test, but passing depends on one short timing sample set beating another. A scheduler pause during the borrowed-frame samples can fail the suite even when the buffer behavior is correct. Keep the pointer and mapping-count checks in the unit test, and move the timing comparison to an ignored benchmark.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
MemFd chunk offsets are relative to spa_data.mapoffset. Map that window, include it in the cache key, and bound the readable tail to the bytes after it so borrowed and copied frames see the producer's pixels. The 4K timing comparison is an ignored benchmark. The unit test keeps the pointer and mapping-count checks.
|
@greptileai review |
The GNOME reliability test kept captured frames until after the capture thread joined. Aliased DMA-BUFs hold the producer buffer until that last reference drops, so the thread hit its idle timeout and reported that capture stopped while a frame was still borrowed. Copy the pixels and drop the frames before joining.
|
@greptileai please re-review — GNOME CI fix: DMA-BUF alias test now copies pixels and drops borrowed frames/channel before joining the capture thread (was timing out |
Summary
Linear PipeWire frames are now read-only views of the producer buffer until the last frame reference drops (
AVBuffer+AV_BUFFER_FLAG_READONLY). Encoder and muxer held last-frame share that buffer; when the last ref drops, the PipeWire buffer is returned on the loop thread (pw_stream_queue_bufferis single-producer — encoder only posts the buffer address).DMA-BUF / memfd are mmap'd through a dup of the producer fd and reused until fd or mapped size changes. A mapping still borrowed is not unmapped.
Writable copy still runs when
av_frame_make_writablebefore blend — alias is read-only)pool < 2, or every buffer but one already borrowed). Capture asks for 1–32 buffers, default 8Benchmarks
3840×2160 BGRA, 9 samples after 2 warmup (release).
map_callsstayed 1. Honest note: the tempfile is not a GPU dma-buf (DMA_BUF_IOCTL_SYNC→ENOTTY), so this run does not reproduce the scanner's ~1.1 ms GPU map/unmap. The row-copy number is the cost removed on the alias path.Tests
cargo test -p cap-recording --lib pipewire— 17 passed, 1 ignored (last-ref lifetime, read-only copy before write, no unmap while borrowed, unmap after size change, single requeue). Clippy clean.Notes
perf/linux-pipewire-alias-dma-bufon kyzer1023/Cap.
The PR appears safe to merge; no blocking issue remains from this re-review.Summary
This PR lets linear PipeWire frames share producer memory until their last reference drops and keeps DMA-BUF mappings for reuse.
Reviews (3) · Last reviewed commit: "fix(linux-pipewire): release niri DMA-BU..." · Reviewed by Greptile