Pass packets as plain AVPacket through the demux/decode seams - #1695
Merged
Merged
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1695
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
2 times, most recently
from
September 4, 2026 09:28
7ebd839 to
2b0ac1b
Compare
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
from
September 4, 2026 10:02
2b0ac1b to
665ea66
Compare
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
from
September 4, 2026 10:18
665ea66 to
2d404db
Compare
NicolasHug
removed this pull request from stack #1700
September 10, 2026 08:52
NicolasHug
added this pull request to stack #1702
September 10, 2026 08:55
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
from
September 10, 2026 08:55
2d404db to
ee088c3
Compare
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
from
September 11, 2026 10:53
ee088c3 to
d0618ef
Compare
NicolasHug
changed the title
next_packet() has to return a packet the caller owns indefinitely: the Python-level Packet can be held, moved to another thread, and decoded much later. That is a UniqueAVPacket, and the AutoAVPacket / ReferenceAVPacket pair is for the opposite thing - a loop that reuses one allocation and unrefs at the end of each iteration. So next_packet() was using the loop idiom and then undoing it: it read into the shared packet, allocated a second one anyway, and moved the reference across. Neither half of the idiom paid for itself - the allocation isn't saved, and av_packet_move_ref() leaves the source blank so the scope-exit unref has nothing to do. read_next_packet() has no opinion on how the packet is owned, it just needs somewhere to write, so it takes the AVPacket itself. Its callers keep the ownership type that suits them: a ReferenceAVPacket for the two read loops (SingleStreamDecoder and scan), a UniqueAVPacket for next_packet(). It also unrefs between iterations, which av_read_frame() documents as the caller's job rather than something it does for you.
NicolasHug
force-pushed
the
demuxer-owned-packets
branch
from
September 14, 2026 10:51
d0618ef to
4bc28de
Compare
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.
Applies F.7:
read_next_packet()now takesAVPacket&andsend_packet()takesconst AVPacket&, instead ofReferenceAVPacket&. Callers keep whatever ownership suits them. This drops two defensive refcount dances: the alloc +av_packet_move_ref()inDemuxer::next_packet(), and the per-packetav_packet_refinPacketDecoder::send_packet()(now done only inapply_bsf(), the one place that actually consumes the packet).Drive-by fix:
read_next_active_packet()never unref'd the packets it skipped.av_read_frame()doesn't free what it overwrites, so each one leaked its payload — worst case, a scan of an A/V file leaked the whole audio track.