-
Notifications
You must be signed in to change notification settings - Fork 64
Initial C++ implementation of transforms #902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 36 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
0620470
First pass on transforms. Committing to switch branches
scotts ad1631d
Merge branch 'main' of github.com:pytorch/torchcodec into transform_core
scotts c59de36
Initial C++ implementaiton of transforms
scotts 890e2b4
Ha, "maybe unsued".
scotts d07f7d8
Update C++ tests
scotts cc4e2ec
Remove C++ test that we no longer need
scotts f471776
Virtual classes need virtual destructors
scotts c06aa94
Cuda device convert frames function
scotts 781f956
Fix cuda
scotts 8e7072f
Handle swscale correctly
scotts 1d0c275
Variable names matter
scotts 30622a7
Timebase
scotts 7a41bfd
Removes width and height from StreamOptions
scotts 8e55bd4
More cuda error checking
scotts a032cb7
Don't pass pre-allocated GPU tensor to CPU decoding
scotts 9aa85c2
Lint
scotts 4e6c6f8
Remove prints from test
scotts aa54a02
Merge branch 'main' of github.com:pytorch/torchcodec into transform_core
scotts 3737099
Lint
scotts 139e4ff
Refactor NV12 stuff; test if we need format for FFmpeg 4
scotts 6668f4b
Specify hwdownload format as rgb24
scotts 9f357c7
Do all nv12 conversions on GPU
scotts 3dc20b8
Wrong output format
scotts 7f88e60
Back to RGB24
scotts dda2649
CUDA and CPU refactoring regarding NV12.
scotts fc5468e
Test to ensure transforms are not used with non-CPU
scotts 48e3ea3
Better comments; refactor toTensor
scotts 7813005
Deal with variable resolution and lying metadata - again
scotts 23ec35f
Better comment
scotts fb06f87
Proper frame dims handling in CUDA
scotts 3626854
Make swscale and filtergraph look more similar
scotts 1a07828
Better comment formatting
scotts ee3b9b7
Apply reviewer suggestions
scotts d2e9bde
Refactor device interface, again.
scotts 343ed3e
Merge branch 'main' of github.com:pytorch/torchcodec into transform_core
scotts db2ea07
Clean up comment
scotts 1753f9c
Name change
scotts 4b9f4c9
Merge branch 'main' of github.com:pytorch/torchcodec into transform_core
scotts 9efb767
Stragglers
scotts File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment on my understanding of
CpuDeviceInterface::initialize
andCpuDeviceInterface::initializeVideo
:initialize
(previouslyinitializeContext
) now does 2 seemingly orthogonal things:timeBase_
fieldinitializeVideo
(new) initializes other fields of the interface.Was there a particular reason to set
timeBase_
within initialize? If possible I think it would help to separate concerns here. For example we could have one method to initialize thecodecContext
itself (and nothing else), and one separate method to initialize all the interface and its fields (timeBase_
and the rest). In https://github.com/meta-pytorch/torchcodec/pull/910/files#diff-e4fe2b8c629954b1dac78a122bd2b33664844f022aa0bf3abc72b5938a14f4e2R465 I did something very similar to your changes, where:initializeContext
is unchanged and only initializes the codecContextinitialize
is a new method that initializes fields on the interface itself, including thetimeBase_
which I needed too. It's analogous to yourinitializeVideo()
.Here you're calling your
initializeVideo()
later than where I call myinitialize()
, but I think still would still work fine with your initialization order.(I also noticed you moved the cuda context creation to the constructor, which I think is a good thing!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasHug, my rationale here is:
SpecificDeviceInterface::SpecificDeviceInterface()
, as in, the actual implementation's default constructor, should initialize as much as it can given that it will be initialized without any input.DeviceInterface::initialize()
is for things that are generic to decoding in general. Video and audio decoding will need whatever goes here. That's whatcodecContext_
andtimeBase_
have in common.DeviceInterface::initializeVideo()
is obviously just for things specific to video. At the moment, our transforms are very video specific, although I do think there's a point where we'll want to generalize them to audio and then transforms will move toinitialize()
.DeviceInterface::initializeAudio()
is something we'll eventually implement when we move audio decoding into a device interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For posterity, @NicolasHug's point is about direction of state change. We want to differentiate between the device interface being initialized and the device interface changing the state of something else.