Pass packets as const AVPacket& through the decode seam - #1691
Open
NicolasHug wants to merge 1 commit into
Open
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1691
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 3 Unrelated FailuresAs of commit fe3317e with merge base 6f705bb ( NEW FAILURE - The following job has failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
NicolasHug
force-pushed
the
packet-seam-const-avpacket
branch
from
September 4, 2026 08:19
22e7fec to
d300cf7
Compare
NicolasHug
force-pushed
the
packet-seam-const-avpacket
branch
from
September 4, 2026 08:55
d300cf7 to
8be4e8a
Compare
NicolasHug
force-pushed
the
packet-seam-const-avpacket
branch
from
September 4, 2026 09:28
8be4e8a to
05f579e
Compare
NicolasHug
force-pushed
the
packet-seam-const-avpacket
branch
2 times, most recently
from
September 4, 2026 10:18
1c94f1e to
6e589f9
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
packet-seam-const-avpacket
branch
from
September 10, 2026 08:55
6e589f9 to
ff5352b
Compare
DeviceInterface::send_packet() only ever reads the packet - the default implementation hands it to avcodec_send_packet(), which takes a const one. Except that BetaCuda's bitstream filter path did not: av_bsf_send_packet() takes ownership of what it is given and resets it, so sending a packet through the seam could empty it, silently, with nothing in the signature saying so. SingleStreamDecoder never noticed, because its packet is dead after the send. PacketDecoder did: the packet it is handed is a user-visible Python object that has to survive being decoded, so it referenced it into a throwaway packet before every single send - a per-packet allocation, on every device, to defend against a contract that was never stated. State the contract instead. The seam takes a const AVPacket&, and the one implementation that needs ownership references the packet itself, where the reason for it is local.
NicolasHug
force-pushed
the
packet-seam-const-avpacket
branch
from
September 11, 2026 10:53
ff5352b to
fe3317e
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
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.
DeviceInterface::send_packet() only ever reads the packet - the default
implementation hands it to avcodec_send_packet(), which takes a const one.
Except that BetaCuda's bitstream filter path did not: av_bsf_send_packet()
takes ownership of what it is given and resets it, so sending a packet
through the seam could empty it, silently, with nothing in the signature
saying so.
SingleStreamDecoder never noticed, because its packet is dead after the
send. PacketDecoder did: the packet it is handed is a user-visible Python
object that has to survive being decoded, so it referenced it into a
throwaway packet before every single send - a per-packet allocation, on
every device, to defend against a contract that was never stated.
State the contract instead. The seam takes a const AVPacket&, and the one
implementation that needs ownership references the packet itself, where the
reason for it is local.