Pass AVFrames by reference, not by smart pointer - #1598
Merged
Conversation
Everything downstream of an AVFrame's owner took `UniqueAVFrame&` or `const UniqueAVFrame&`. That's a constraint on the caller's storage rather than a statement about what the function does, and it has two costs. A non-const `UniqueAVFrame&` lets a callee take ownership of the caller's frame. Both CUDA interfaces did: BetaCudaDeviceInterface moved out of it, CudaDeviceInterface reassigned it. That is invisible under SingleStreamDecoder, whose frame is a loop local that dies right after conversion, but the building-block ops own their frame in a handle that outlives the call, so the frame gets freed twice. `const UniqueAVFrame&` doesn't allow the steal, but it still forces anyone holding a plain AVFrame* -- which is what the ops' tensor handle really is -- to manufacture a unique_ptr just to make the call, and manufacturing a second owner for an already-owned object is its own bug factory. So: functions that only look at a frame now take `const AVFrame&`, and the one that writes to it takes `AVFrame&`. Ownership stays with whoever actually owns the frame. Producers (receive_frame) keep `UniqueAVFrame&` because they really do hand back ownership. With that, the ops layer needs no ownership sleight-of-hand: wrap_pointer_to_tensor() gains a deleter parameter, so the one generic handle covers Demuxer/PacketDecoder/ColorConverter and the FFmpeg types too, and both bespoke wrap_*_pointer_to_tensor() functions go away. Demuxer::next_packet() returns UniqueAVPacket rather than a raw pointer plus a comment telling the caller to free it. The encoder is left alone: it owns and mutates its frames, and it uses a null frame as the flush signal, so a reference is the wrong shape there.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1598
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ⏳ No Failures, 20 PendingAs of commit 5ccac34 with merge base 3f7692b ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
Two things only show up when building against the older FFmpeg headers. get_num_channels() was patching av_frame.channel_layout when FFmpeg 4 left it unset, so it was a mutator wearing an observer's name, and the layout fix-up was a side effect that swresample setup silently relied on. Pull the fix-up into get_channel_layout() and call that from the two places that actually need a layout; get_num_channels() just counts. swr_alloc_set_opts2() only became const-correct in FFmpeg 6 (libswresample 4.12). Cast for the older headers, which don't modify the layout either.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Almost all our APIs take a
UniqueAVFrame, or a potentiallyconstref ofUniqueAVFrame. The C++ core guidelines recommend to use smart pointers only when the interface has ownership semantics:So this PR aligns the use of AVFrame with that, i.e. we now mostly just take
const AVFrame&as input rather than aUniqueAVFrame, and only useUniqueAVFramefor those interfaces that deal with ownership.This isn't just a cosmetics change: working on the Blocks APIs showed that we sometimes need to create a dummy
UniqueAVFramejust to be able to call our APIs, and things started to be really subtle / complicated as I started working on CUDA support. This small refac should avoid lots of headaches.There are other instances in the codebase that we should cleanup too, with other smart pointers and with AVFrame as well (particularly the
Encoder).