perf(ffmpeg): reference engine frames instead of copying them

[patrickhampson]

make_av_video_frame copied every frame the FFmpeg consumer received into a newly allocated AVFrame (it carried a TODO (perf) Avoid extra memcpy). On an interlaced channel the consumer receives two full frames per tick, so a consumer with fps=25 on a 1080i50 channel copied 50 frames a second and the filter then discarded half of them.

This change wraps the frame's image data in read-only AVBuffers that hold a reference to the const_frame, so the frame stays alive until FFmpeg releases it. Because the buffers are read-only, a filter that writes in place gets its own copy through av_frame_make_writable, and the engine's frame is never modified. Image data that does not meet FFmpeg's alignment (32 bytes, or 64 for 16-bit) is still copied as before.

The FFmpeg consumer is the only caller of make_av_video_frame.

Measurements

Three FFmpeg consumers on 1080i50 channels, each -filter:v fps=25,format=bgra -codec:v h264_nvenc, with nothing playing. CPU of the server process from top (100% = one core).

Build	Idle CPU
2.5.1	~190%
2.5.1 + this change	~93%

Before, perf showed about one core in libc, almost all of it memcpy. After, libc is about 0.36 cores, which matches NVENC copying the 25 frames per second that fps= keeps into its own input buffers. perf shows no time in the copy fallback.

Testing

• Measured on Linux with FFmpeg 8.0.1 and an NVIDIA RTX A2000, on v2.5.1-stable with this commit cherry-picked (it applies cleanly).
• A standalone harness built real const_frames and ran them through make_av_video_frame, five filter chains (null, fps=25, drawbox which writes in place, fps=25,format=yuv420p, scale) and an FFV1 encoder. It compared the zero-copy path against the copy fallback, built against FFmpeg 6.1:
  • The output is identical.
  • The source frames are unmodified.
  • Every frame is released.
  • It runs clean under AddressSanitizer and UBSan.
• Not tested on Windows.

Summary by CodeRabbit

• Performance

  • Video frames with compatible memory alignment can be processed without copying their image data. Frames that do not meet alignment requirements continue to use the copy-based path.

• Reliability

  • Improved handling of padded image buffers during GPU processing and video readback while preserving the reported frame size across supported graphics backends. Returned image data remains limited to the source frame’s size.

[dedicatedbroadcastsolutions]

Reviewed the zero-copy change against the frame ownership and GPU readback allocation paths at this commit. One source-verified padding concern is noted inline. This is a documented buffer-contract concern, not a reproduced crash; I have not run the standalone harness or verified Windows behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3d7da082-3865-463e-b3d0-e5eb123d6f0c

📥 Commits

Reviewing files that changed from the base of the PR and between 1324eb5 and 9497c4d.

📒 Files selected for processing: 6

• src/accelerator/ogl/image/image_mixer.cpp
• src/accelerator/ogl/util/device.cpp
• src/accelerator/vulkan/image/image_mixer.cpp
• src/accelerator/vulkan/util/transfer.cpp
• src/core/frame/frame.h
• src/modules/ffmpeg/util/av_util.cpp

🚧 Files skipped from review as they are similar to previous changes: 1

• src/modules/ffmpeg/util/av_util.cpp

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

GPU image and texture paths allocate 64 bytes of extra buffer space while preserving the returned data lengths. Vulkan buffer return handling uses the requested size as its pool key. FFmpeg video-frame creation references aligned source planes and retains the source frame; otherwise, it uses the existing copy path.

Changes: Frame Buffers and FFmpeg

Layer / File(s)	Summary
Padded GPU buffers and readback sizing src/core/frame/frame.h, src/accelerator/ogl/image/image_mixer.cpp, src/accelerator/vulkan/image/image_mixer.cpp, src/accelerator/ogl/util/device.cpp, src/accelerator/vulkan/util/transfer.cpp, src/accelerator/vulkan/util/device.cpp	OpenGL and Vulkan empty-frame buffers and texture readback buffers allocate 64 additional bytes. Returned array lengths remain tied to the source data. Vulkan buffer return handling uses the requested size as the host-pool key.
Aligned plane references and fallback copying src/modules/ffmpeg/util/av_util.cpp	When every plane meets the applicable alignment requirements, make_av_video_frame wraps source planes in read-only AVBufferRefs that retain the source frame. If any plane is unaligned, it uses the existing allocation and copy path with the selected alignment.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: niklaspandersson

Merge Risk: ⚪ Minimal · up to 9497c

The changed FFmpeg path references aligned mixed frames, while both render backends reserve the required padding. No material merge-blocking risk was found; the change is ready for normal checks.

### Architecture Summary

Architecture risk: 🟡 Medium · up to 9497c

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — src (service) was modified; 7 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in src/accelerator/vulkan/util/device.cpp: create_buffer now captures the requested size in the buffer-deleter and uses it as the host-pool key instead of the buffer’s allocated size.
• observed — Modified behavior in src/accelerator/ogl/image/image_mixer.cpp: The empty-layer bypass buffer allocation now includes core::frame_padding beyond max_frame_size_; its zero initialization and the returned array’s format_desc.size length are unchanged.
• observed — Modified behavior in src/accelerator/ogl/util/device.cpp: Adds the core frame header include, providing the frame-related definition used by the readback buffer allocation.
• observed — Modified behavior in src/accelerator/ogl/util/device.cpp: Readback now allocates a non-write host buffer with core::frame_padding bytes beyond the source texture size; previously its allocation matched the texture size.

Reliability and maintainability

• inferred — Risk-relevant change factors for src: blast_radius_2; direct_dependents_2

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: FFmpeg now references eligible engine frames instead of copying them.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[patrickhampson]

Added padding the backing allocations, as you suggested:

• OpenGL and Vulkan readback buffers, and the empty frame in both image mixers, are allocated with 64 extra bytes. FFmpeg documents 16; 64 matches its own padding constant.
• The array size is unchanged, so consumers that copy image_data().size() bytes (e.g. Bluefish) are unaffected. The Vulkan readback previously reported the allocation size, which can be larger than the frame; it now reports the frame size.
• The Vulkan host buffer pool now returns buffers under the requested size. It used buf->size(), the allocation size, which the driver can round up. With the padding, a readback buffer could have gone back to a pool that is never drawn from.
• make_av_video_frame notes that it relies on this padding. It can't check it from the frame, so the guarantee is documented where each frame is allocated. Every frame a consumer receives comes from the image mixer's readback or its empty frame.

Testing

Before (first commit):

• Appliance: three FFmpeg consumers on 1080i50, fps=25,format=bgra, h264_nvenc, idle, on v2.5.1-stable + this change, FFmpeg 8.0.1, RTX A2000. Idle CPU went from ~190% to ~93%; perf showed libc down from about a core to ~0.36 cores and no time in the copy fallback.
• Standalone harness (FFmpeg 6.1, x86-64): real const_frames through make_av_video_frame, five filter chains and FFV1, zero-copy compared against the copy fallback. Identical output, sources unmodified, all frames released, clean under ASan/UBSan. Removing the read-only flag makes the in-place drawbox case fail. Its allocations had 64 bytes of slack, so it couldn't see overreads.

After (your suggestion):

• The harness now places each zero-copy frame so it ends exactly on a page boundary, followed by a PROT_NONE page, so any read past the end faults. I ran 25 chains: swscale conversions to nv12, yuv420p, yuv422p10le, yuv444p, gbrp and rgb24; bicubic, lanczos and area scaling; hflip, vflip, crop, transpose and pad; boxblur, gblur and unsharp; overlay, lutrgb, colorchannelmixer and drawbox. None faulted on FFmpeg 6.1 / x86-64, with identical output to the copy path. That doesn't remove the documented allowance, hence the padding.
• The changed files compile. The Vulkan files were checked against older headers with one unrelated enum stubbed, since the unchanged lines need newer Vulkan headers.

[dedicatedbroadcastsolutions]

@/tmp/review_body.txt

[dedicatedbroadcastsolutions]

I had to delete the inline review as it would not allow me to resolve it.

Note: This is an AI review so please treat accordingly.
Re-reviewed at 1324eb5 (previous review was against ad1023c). The padding concern I raised inline on av_util.cpp is resolved for the most part.

Here is some feedback I was able to get from AI that should be considered. The whole test harness thing needs defined and there is a thread on the forum about that.

"The implementation of the invariant is fragile, and I undersold that in my first pass:

It's an invisible, distributed contract. The safety of the zero-copy branch in av_util.cpp depends on five separate allocation sites in four unrelated files (image_mixer.cpp ×2, device.cpp, device.cpp/transfer.cpp) all remembering to add exactly the same magic 64. The only thing enforcing this is a one-line comment at each site. Nothing checks it — not a static_assert, not a runtime assertion, not a type. If someone adds a third accelerator backend, changes the GL/Vulkan readback path, or "cleans up" one of those + 64s without reading the comment, the zero-copy path in av_util.cpp silently becomes an out-of-bounds read again — with no compile error and no obvious crash until a specific filter/alignment/allocator-reuse combination triggers it.
array<> can't express the guarantee. The consumer (make_av_video_frame) has no way to ask "does this buffer actually have N safe trailing bytes?" — it only sees .size(), which is deliberately the unpadded logical size. So it can't defend itself; it just has to trust upstream. That's the real gap: the one function that needs the guarantee is the one place that can't verify it.
There's no regression test anywhere. I checked — this repo has zero test infrastructure (no CMake add_test, no gtest/Boost.Test/Catch2 harness, nothing under any test/ directory). The author's guard-page/ASan harness that actually proved this is safe only exists as a description in the PR body — it's not in the repo. That's the thing that would have caught the original bug and is exactly the thing that would catch a regression, and it's not committed anywhere.
What I'd suggest instead of "approve as-is"
Centralize the padding constant (one constexpr shared by all five call sites) instead of a repeated bare 64.
Give make_av_video_frame something concrete to check rather than blind trust — even a cheap core::const_frame/array<> capacity accessor it can assert against, falling back to the copy path if the guarantee can't be confirmed, turns a silent memory-safety landmine into a safe (if slower) degradation.
Ask the author to commit the guard-page harness (even as a manually-run standalone tool, since there's no test framework to integrate it into) so the one piece of evidence this works doesn't live only in a PR description.
None of this means the fix is wrong — I'd stand by "the reported crash risk is fixed." But "no remaining blocking concerns" was too generous; this deserves a non-blocking follow-up about robustness/maintainability."

These sound like mainly maintainability improvements that are probably wise to think ahead about.

[patrickhampson]

These sound like mainly maintainability improvements that are probably wise to think ahead about.

Moved padding to a constant that should hopefully make it less distributed.

Capacity check in make_av_video_frame

I agree that the function can’t check the guarantee itself, since it only sees size(). Fixing that properly means adding a capacity to caspar::array in common/array.h and filling it in for every storage type it wraps. That change reaches well beyond this PR, while every frame a consumer receives today comes from the mixer’s readback or its empty frame, and both are padded. I’d rather not widen this PR for it.