Consolidate VideoDecoder creation with python wrapper#1250
Consolidate VideoDecoder creation with python wrapper#1250NicolasHug merged 6 commits intometa-pytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1250
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 1 PendingAs of commit 024ac78 with merge base 068e58a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
zy1git
force-pushed
the
consolidate-python-version-video
branch
from
c42ad34 to
4391528
Compare
| """Get and validate video stream metadata from a decoder. | ||
|
|
||
| Args: | ||
| decoder: The decoder tensor. | ||
| stream_index: The stream index to use, or None to use the best stream. | ||
|
|
||
| Returns: | ||
| A tuple of (metadata, stream_index, begin_stream_seconds, | ||
| end_stream_seconds, num_frames). | ||
| """ |
There was a problem hiding this comment.
This helper didn't have docs, I feel like it's not really necessary to add them. The interface is pretty straightforward, this isn't user-facing, and any modification to the underlying logic would mean we'd have to keep the docs up to date consequently. So I'd suggest to just remove the docstrings and keep it they it was.
There was a problem hiding this comment.
Removed in the new commit.
| if device is None: | ||
| device = str(torch.get_default_device()) | ||
| elif isinstance(device, torch_device): | ||
| device = str(device) |
There was a problem hiding this comment.
This is now done twice, here and in __init__. Let's just do it once and let this function accept the string as input.
There was a problem hiding this comment.
Done in the new commit.
| begin_stream_seconds, | ||
| end_stream_seconds, | ||
| num_frames, |
There was a problem hiding this comment.
I realize this was already the case in main, but let's use this PR as opportunity to clean this up too: all of begin_stream_seconds, begin_stream_seconds and num_frames are just fields of metadata, so let's not return them. We should just use the info from metadata whenever relevant.
We can keep the existing fields like self._begin_stream_seconds on the VideoDecoder` however.
There was a problem hiding this comment.
Done in the new commit.
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the progress. Can you look into the mypy lint issue?
| """Create a video decoder and add a video stream. | ||
|
|
||
| This function consolidates the creation of a decoder and adding a video stream | ||
| into a single operation, performing all necessary validation. | ||
|
|
||
| Args: | ||
| source: The source of the video. | ||
| seek_mode: The seek mode for the decoder. | ||
| stream_index: The stream index to decode, or None to use the best stream. | ||
| dimension_order: The dimension order for decoded frames. | ||
| num_ffmpeg_threads: Number of FFmpeg threads for CPU decoding. | ||
| device: The device for decoding (as a string). | ||
| device_variant: The CUDA backend variant to use ("ffmpeg" or "beta"). | ||
| transforms: Optional sequence of transforms to apply. | ||
| custom_frame_mappings: Optional pre-processed frame mappings data. | ||
|
|
||
| Returns: | ||
| A tuple of (decoder, metadata, stream_index). | ||
| """ |
There was a problem hiding this comment.
Let's not add a docstring here. Same reason as in #1250 (comment)
There was a problem hiding this comment.
done in the new commit.
| decoder, | ||
| metadata, | ||
| stream_index, | ||
| ) |
There was a problem hiding this comment.
Let's keep the return order consistent with create_audio_decoder which is returning decoder, stream_index, metadata.
There was a problem hiding this comment.
Also, as Nit, here and in other places, just make the return fit in one line instead of 3 if possible.
There was a problem hiding this comment.
done in the new commit
| return (decoder, stream_index, metadata) | ||
|
|
||
|
|
||
| def _get_and_validate_stream_metadata( |
There was a problem hiding this comment.
Let's name this _get_and_validate_video_stream_metadata since this is video only now.
There was a problem hiding this comment.
done in the new commit
zy1git
force-pushed
the
consolidate-python-version-video
branch
from
ad903bb to
024ac78
Compare
2 participants
This PR introduces a
create_video_decoder()wrapper function that consolidates the two-step video decoder creation process (create decoder + add video stream) into a single function call.This is the Python-only phase of the video decoder consolidation for issue #1064.