Make the output dtype config the only dtype on VideoStreamOptions - #1696
Open
NicolasHug wants to merge 1 commit into
Open
Make the output dtype config the only dtype on VideoStreamOptions#1696NicolasHug wants to merge 1 commit into
NicolasHug wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1696
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6cbab16 with merge base 2312413 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
NicolasHug
force-pushed
the
nvdec-surface-depth
branch
from
September 4, 2026 08:55
56ee0a6 to
73f11dd
Compare
NicolasHug
changed the title
NicolasHug
changed the title
NicolasHug
force-pushed
the
nvdec-surface-depth
branch
2 times, most recently
from
September 4, 2026 10:02
53f6d35 to
0d854f0
Compare
Two halves of one thing. VideoStreamOptions held both the user's
OutputDtypeConfig and the OutputDtype resolved from it, and both the decode
building block and BetaCuda were reaching for the resolved one when what
they wanted was the config.
First, the NVDEC surface. The decode building block asked for a 16-bit
surface by setting output_dtype to FLOAT32, which nothing downstream of it
reads: it does no color conversion, so it has no output dtype. What it
computed to get there, `stream_bit_depth > 8 ? FLOAT32 : UINT8`, is
character for character the rule resolve_output_dtype() applies to AUTO, so
the block was resolving AUTO by hand and leaving BetaCuda to map the answer
back into a surface width. Pass the config and the mapping is total:
UINT8 -> 8-bit surface, all a uint8 output needs
FLOAT32 -> 16-bit surface, so precision survives
AUTO -> whatever the source has
A caller that color-converts elsewhere wants that last one, so it asks for
AUTO and says nothing about NVDEC. BetaCuda's output_dtype_ member goes
with it: the decode path was its only reader, and the store in
initialize_color_conversion() was already dead.
Second, the field itself. OutputDtype is resolve_output_dtype(config,
source pixel format) - derived, not configured - and holding it next to the
config it derives from left the struct half-valid until someone resolved.
The two users then resolved it in ways one shared field couldn't express:
SingleStreamDecoder wrote the answer back into its own options once per
stream, while ColorConverter, which resolves per frame because AUTO "can
differ from one frame to the next", fabricated a whole VideoStreamOptions
purely to carry the answer across initialize_color_conversion(). So make
the resolved dtype an argument of that call - the only stage that needs it
- and leave the struct with the config alone. SingleStreamDecoder's
once-per-stream answer moves to StreamInfo, where decoder state belongs.
Worth knowing while reading: the two resolutions take their pixel format
from different places, the stream header in SingleStreamDecoder and the
frame itself in ColorConverter, so they can disagree. Passing the value
explicitly doesn't fix that, but it stops one struct field from implying
they are the same thing.
No behaviour change. initialize_color_conversion() and initialize_video()
both gain a parameter, which out-of-tree device interfaces will need to
follow.
NicolasHug
force-pushed
the
nvdec-surface-depth
branch
from
September 4, 2026 10:18
0d854f0 to
6cbab16
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.
VideoStreamOptions held both the user's
OutputDtypeConfigand theOutputDtyperesolved from it. Two changes, one theme: the config becomes the only dtype on the struct.The NVDEC surface. The decode building block asked for a 16-bit surface by setting
output_dtypeto FLOAT32 — but it does no color conversion, so it has no output dtype. The expression it used to get there,bit_depth > 8 ? FLOAT32 : UINT8, is exactly whatresolve_output_dtype()does with AUTO: it was resolving AUTO by hand so BetaCuda could map the answer back into a surface width. Pass the config and the mapping is direct:UINT8FLOAT32AUTOThe resolved field.
OutputDtypeis derived from the config plus the source pixel format, so storing it beside the config left the struct half-valid until someone resolved.SingleStreamDecoderwrote the answer back into its own options;ColorConverter, which must resolve per frame, fabricated a wholeVideoStreamOptionsjust to carry it intoinitialize_color_conversion(). It is now an argument of that call, and SSD's per-stream answer lives onStreamInfo.No behaviour change.
initialize_color_conversion()andinitialize_video()gain a parameter, so out-of-tree device interfaces will need updating.Replaces #1692, which GitHub auto-closed as merged during a stack reorder — nothing was actually merged. Absorbs #1698.