Support rotation on beta cuda#1235
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1235
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 298c9ef with merge base 2d1b5c6 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| } | ||
| // Apply rotation using torch::rot90 on the H and W dims of our HWC tensor. | ||
| // torch::rot90 returns a view, so we need to make it contiguous. | ||
| frameOutput.data = torch::rot90(frameOutput.data, k, {0, 1}).contiguous(); |
There was a problem hiding this comment.
The code structure makes it a bit hard to follow what preAllocatedOutputTensor is storing, perhaps we could change it to be more explicit when changes to the preallocated tensor are needed:
if rotation_ == Rotation::NONE:
convertNV12FrameToRGB() # regular call
else:
convertNV12FrameToRGB() # call without preallocated tensor
applyRotation()Ideally we could handle the rotation switch statement and call to torch::rot90 in a separate function.
There might be nuances to the logic I am missing that make this structure difficult, please double check me on this.
There was a problem hiding this comment.
Agreed it might help to have an applyRotation() helper.
I'm not sure about having convertNV12FrameToRGB() within if/else blocks, it might be tricky since there is also validatePreAllocatedTensorShape which depends on preAllocatedOutputTensor having the correct size (i.e. we'd still need the savedPreAllocatedOutputTensor hack above, I think).
There was a problem hiding this comment.
I'm still a little hesitant on this approach - when the hack sets preAllocatedOutputTensor = std::nullopt, then validatePreAllocatedTensorShape does not actually do any validation. Excluding the validation call in the else block would be more explicit about that.
This is minor though, and could be addressed when we implement the less hacky solution which I believe will use the NPP functions?
There was a problem hiding this comment.
Thanks for the discussion! I agree that it is clearer to be explicit in excluding the validation call and to reduce the number of variables
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @mollyxu , this LGTM, I'll let @Dan-Flores make another pass as well
Add rotation support to
BetaCudaDeviceInterfaceusingtorch::rot90Parametrize over
cpuandcuda:betaintest_rotation_applied_to_frames(note: since we are planning to move to beta cuda eventually, we've decided to currently not support this on cuda with ffmpeg backend)