-
Notifications
You must be signed in to change notification settings - Fork 67
Add pixel_format to VideoEncoder API #1027
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
base: main
Are you sure you want to change the base?
Changes from all commits
d75f0eb
4ff25f6
7ef8a8f
dc86a8c
884f4dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| from pathlib import Path | ||
| from typing import Union | ||
| from typing import Optional, Union | ||
|
|
||
| import torch | ||
| from torch import Tensor | ||
|
|
@@ -35,29 +35,38 @@ def __init__(self, frames: Tensor, *, frame_rate: int): | |
| def to_file( | ||
| self, | ||
| dest: Union[str, Path], | ||
| *, | ||
| pixel_format: Optional[str] = None, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job on making this a keyword only params 👍 |
||
| ) -> None: | ||
| """Encode frames into a file. | ||
|
|
||
| Args: | ||
| dest (str or ``pathlib.Path``): The path to the output file, e.g. | ||
| ``video.mp4``. The extension of the file determines the video | ||
| container format. | ||
| pixel_format (str, optional): The pixel format for encoding (e.g., | ||
| "yuv420p", "yuv444p"). If not specified, uses codec's default format. | ||
| """ | ||
| _core.encode_video_to_file( | ||
| frames=self._frames, | ||
| frame_rate=self._frame_rate, | ||
| filename=str(dest), | ||
| pixel_format=pixel_format, | ||
| ) | ||
|
|
||
| def to_tensor( | ||
| self, | ||
| format: str, | ||
| *, | ||
| pixel_format: Optional[str] = None, | ||
| ) -> Tensor: | ||
| """Encode frames into raw bytes, as a 1D uint8 Tensor. | ||
|
|
||
| Args: | ||
| format (str): The container format of the encoded frames, e.g. "mp4", "mov", | ||
| "mkv", "avi", "webm", "flv", or "gif" | ||
| pixel_format (str, optional): The pixel format to encode frames into (e.g., | ||
| "yuv420p", "yuv444p"). If not specified, uses codec's default format. | ||
|
|
||
| Returns: | ||
| Tensor: The raw encoded bytes as 4D uint8 Tensor. | ||
|
|
@@ -66,12 +75,15 @@ def to_tensor( | |
| frames=self._frames, | ||
| frame_rate=self._frame_rate, | ||
| format=format, | ||
| pixel_format=pixel_format, | ||
| ) | ||
|
|
||
| def to_file_like( | ||
| self, | ||
| file_like, | ||
| format: str, | ||
| *, | ||
| pixel_format: Optional[str] = None, | ||
| ) -> None: | ||
| """Encode frames into a file-like object. | ||
|
|
||
|
|
@@ -83,10 +95,13 @@ def to_file_like( | |
| int = 0) -> int``. | ||
| format (str): The container format of the encoded frames, e.g. "mp4", "mov", | ||
| "mkv", "avi", "webm", "flv", or "gif". | ||
| pixel_format (str, optional): The pixel format for encoding (e.g., | ||
| "yuv420p", "yuv444p"). If not specified, uses codec's default format. | ||
| """ | ||
| _core.encode_video_to_file_like( | ||
| frames=self._frames, | ||
| frame_rate=self._frame_rate, | ||
| format=format, | ||
| file_like=file_like, | ||
| pixel_format=pixel_format, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -629,6 +629,30 @@ def test_bad_input(self, tmp_path): | |||||
| ): | ||||||
| encoder.to_tensor(format="bad_format") | ||||||
|
|
||||||
| @pytest.mark.parametrize("method", ("to_file", "to_tensor", "to_file_like")) | ||||||
| def test_pixel_format_errors(self, method, tmp_path): | ||||||
| frames = torch.zeros((5, 3, 64, 64), dtype=torch.uint8) | ||||||
| encoder = VideoEncoder(frames, frame_rate=30) | ||||||
|
|
||||||
| if method == "to_file": | ||||||
| valid_params = dict(dest=str(tmp_path / "output.mp4")) | ||||||
| elif method == "to_tensor": | ||||||
| valid_params = dict(format="mp4") | ||||||
| elif method == "to_file_like": | ||||||
| valid_params = dict(file_like=io.BytesIO(), format="mp4") | ||||||
|
|
||||||
| with pytest.raises( | ||||||
| RuntimeError, | ||||||
| match=r"Unknown pixel format: invalid_pix_fmt[\s\S]*Supported pixel formats.*yuv420p", | ||||||
| ): | ||||||
| getattr(encoder, method)(**valid_params, pixel_format="invalid_pix_fmt") | ||||||
|
|
||||||
| with pytest.raises( | ||||||
| RuntimeError, | ||||||
| match=r"Specified pixel format rgb24 is not supported[\s\S]*Supported pixel formats.*yuv420p", | ||||||
| ): | ||||||
| getattr(encoder, method)(**valid_params, pixel_format="rgb24") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's personal taste, but I think this test might be simpler and more clear if we did the more basic thing: with pytest.raises(
RuntimeError,
match=r"Unknown pixel format: invalid_pix_fmt[\s\S]*Supported pixel formats.*yuv420p",
):
encoder.to_file(str(tmp_path / "output.mp4", pixel_format="invalid_pix_fmt")
with pytest.raises(
RuntimeError,
match=r"Unknown pixel format: invalid_pix_fmt[\s\S]*Supported pixel formats.*yuv420p",
):
encoder.to_tensor(format="mp4", pixel_format="invalid_pix_fmt")
with pytest.raises(
RuntimeError,
match=r"Unknown pixel format: invalid_pix_fmt[\s\S]*Supported pixel formats.*yuv420p",
):
encoder.to_file_like(file_like=io.BytesIO(), format="mp4", pixel_format="invalid_pix_fmt")
...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this is a bit confusing, but I reused it as the AudioEncoder tests also use this pattern to test across encoding methods. torchcodec/test/test_encoders.py Lines 193 to 194 in dc86a8c
In terms of taste, I would prefer to use this pattern and reduce code duplication. Alternatively, I don't think this test needs to be parametrized across encoding methods, since the error will always be hit in |
||||||
|
|
||||||
| @pytest.mark.parametrize("method", ("to_file", "to_tensor", "to_file_like")) | ||||||
| def test_contiguity(self, method, tmp_path): | ||||||
| # Ensure that 2 sets of video frames with the same pixel values are encoded | ||||||
|
|
||||||
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.
getSupportedPixelFormatsis not guaranteed to return any formats. If the user does not specify a format and we find none, I think we should try to use the broadly supportedyuv420p, rather than error out.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.
Agreed, this makes sense and that's similar to what we do for the audio encoder when we can't validate:
torchcodec/src/torchcodec/_core/Encoder.cpp
Lines 85 to 87 in 8e615e3