Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions src/torchcodec/_core/Encoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
#include "Encoder.h"
#include "torch/types.h"

extern "C" {
#include <libavutil/pixdesc.h>
}

namespace facebook::torchcodec {

namespace {
Expand Down Expand Up @@ -534,6 +538,36 @@ torch::Tensor validateFrames(const torch::Tensor& frames) {
return frames.contiguous();
}

AVPixelFormat validatePixelFormat(
const AVCodec& avCodec,
const std::string& targetPixelFormat) {
AVPixelFormat pixelFormat = av_get_pix_fmt(targetPixelFormat.c_str());

// Validate that the encoder supports this pixel format
const AVPixelFormat* supportedFormats = getSupportedPixelFormats(avCodec);
if (supportedFormats != nullptr) {
for (int i = 0; supportedFormats[i] != AV_PIX_FMT_NONE; ++i) {
if (supportedFormats[i] == pixelFormat) {
return pixelFormat;
}
}
}

std::stringstream errorMsg;
// av_get_pix_fmt failed to find a pix_fmt
if (pixelFormat == AV_PIX_FMT_NONE) {
errorMsg << "Unknown pixel format: " << targetPixelFormat;
} else {
errorMsg << "Specified pixel format " << targetPixelFormat
<< " is not supported by the " << avCodec.name << " encoder.";
}
// Build error message, similar to FFmpeg's error log
errorMsg << "\nSupported pixel formats for " << avCodec.name << ":";
for (int i = 0; supportedFormats[i] != AV_PIX_FMT_NONE; ++i) {
errorMsg << " " << av_get_pix_fmt_name(supportedFormats[i]);
}
TORCH_CHECK(false, errorMsg.str());
}
} // namespace

VideoEncoder::~VideoEncoder() {
Expand Down Expand Up @@ -635,15 +669,19 @@ void VideoEncoder::initializeEncoder(
outWidth_ = inWidth_;
outHeight_ = inHeight_;

// TODO-VideoEncoder: Enable other pixel formats
// Let FFmpeg choose best pixel format to minimize loss
outPixelFormat_ = avcodec_find_best_pix_fmt_of_list(
getSupportedPixelFormats(*avCodec), // List of supported formats
AV_PIX_FMT_GBRP, // We reorder input to GBRP currently
0, // No alpha channel
nullptr // Discard conversion loss information
);
TORCH_CHECK(outPixelFormat_ != -1, "Failed to find best pix fmt")
if (videoStreamOptions.pixelFormat.has_value()) {
outPixelFormat_ =
validatePixelFormat(*avCodec, videoStreamOptions.pixelFormat.value());
} else {
const AVPixelFormat* formats = getSupportedPixelFormats(*avCodec);
// Use first listed pixel format as default (often yuv420p).
// This is similar to FFmpeg's logic:
// https://www.ffmpeg.org/doxygen/4.0/decode_8c_source.html#l01087
// If pixel formats are undefined for some reason, try yuv420p
outPixelFormat_ = (formats && formats[0] != AV_PIX_FMT_NONE)
? formats[0]
: AV_PIX_FMT_YUV420P;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSupportedPixelFormats is 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 supported yuv420p, rather than error out.

Copy link
Contributor

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:

// Can't really validate anything in this case, best we can do is hope that
// FLTP is supported by the encoder. If not, FFmpeg will raise.
return AV_SAMPLE_FMT_FLTP;


// Configure codec parameters
avCodecContext_->codec_id = avCodec->id;
Expand Down
4 changes: 4 additions & 0 deletions src/torchcodec/_core/StreamOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ struct VideoStreamOptions {
// TODO-VideoEncoder: Consider adding other optional fields here
// (bit rate, gop size, max b frames, preset)
std::optional<int> crf;

// Optional pixel format for video encoding (e.g., "yuv420p", "yuv444p")
// If not specified, uses codec's default format.
std::optional<std::string> pixelFormat;
};

struct AudioStreamOptions {
Expand Down
12 changes: 9 additions & 3 deletions src/torchcodec/_core/custom_ops.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ TORCH_LIBRARY(torchcodec_ns, m) {
m.def(
"_encode_audio_to_file_like(Tensor samples, int sample_rate, str format, int file_like_context, int? bit_rate=None, int? num_channels=None, int? desired_sample_rate=None) -> ()");
m.def(
"encode_video_to_file(Tensor frames, int frame_rate, str filename, int? crf=None) -> ()");
"encode_video_to_file(Tensor frames, int frame_rate, str filename, str? pixel_format=None, int? crf=None) -> ()");
m.def(
"encode_video_to_tensor(Tensor frames, int frame_rate, str format, int? crf=None) -> Tensor");
"encode_video_to_tensor(Tensor frames, int frame_rate, str format, str? pixel_format=None, int? crf=None) -> Tensor");
m.def(
"_encode_video_to_file_like(Tensor frames, int frame_rate, str format, int file_like_context, int? crf=None) -> ()");
"_encode_video_to_file_like(Tensor frames, int frame_rate, str format, int file_like_context, str? pixel_format=None, int? crf=None) -> ()");
m.def(
"create_from_tensor(Tensor video_tensor, str? seek_mode=None) -> Tensor");
m.def(
Expand Down Expand Up @@ -603,8 +603,10 @@ void encode_video_to_file(
const at::Tensor& frames,
int64_t frame_rate,
std::string_view file_name,
std::optional<std::string> pixel_format = std::nullopt,
std::optional<int64_t> crf = std::nullopt) {
VideoStreamOptions videoStreamOptions;
videoStreamOptions.pixelFormat = pixel_format;
videoStreamOptions.crf = crf;
VideoEncoder(
frames,
Expand All @@ -618,9 +620,11 @@ at::Tensor encode_video_to_tensor(
const at::Tensor& frames,
int64_t frame_rate,
std::string_view format,
std::optional<std::string> pixel_format = std::nullopt,
std::optional<int64_t> crf = std::nullopt) {
auto avioContextHolder = std::make_unique<AVIOToTensorContext>();
VideoStreamOptions videoStreamOptions;
videoStreamOptions.pixelFormat = pixel_format;
videoStreamOptions.crf = crf;
return VideoEncoder(
frames,
Expand All @@ -636,6 +640,7 @@ void _encode_video_to_file_like(
int64_t frame_rate,
std::string_view format,
int64_t file_like_context,
std::optional<std::string> pixel_format = std::nullopt,
std::optional<int64_t> crf = std::nullopt) {
auto fileLikeContext =
reinterpret_cast<AVIOFileLikeContext*>(file_like_context);
Expand All @@ -644,6 +649,7 @@ void _encode_video_to_file_like(
std::unique_ptr<AVIOFileLikeContext> avioContextHolder(fileLikeContext);

VideoStreamOptions videoStreamOptions;
videoStreamOptions.pixelFormat = pixel_format;
videoStreamOptions.crf = crf;

VideoEncoder encoder(
Expand Down
10 changes: 8 additions & 2 deletions src/torchcodec/_core/ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ def encode_video_to_file_like(
format: str,
file_like: Union[io.RawIOBase, io.BufferedIOBase],
crf: Optional[int] = None,
pixel_format: Optional[str] = None,
) -> None:
"""Encode video frames to a file-like object.

Expand All @@ -222,6 +223,7 @@ def encode_video_to_file_like(
format: Video format (e.g., "mp4", "mov", "mkv")
file_like: File-like object that supports write() and seek() methods
crf: Optional constant rate factor for encoding quality
pixel_format: Optional pixel format (e.g., "yuv420p", "yuv444p")
"""
assert _pybind_ops is not None

Expand All @@ -230,6 +232,7 @@ def encode_video_to_file_like(
frame_rate,
format,
_pybind_ops.create_file_like_context(file_like, True), # True means for writing
pixel_format,
crf,
)

Expand Down Expand Up @@ -318,7 +321,8 @@ def encode_video_to_file_abstract(
frames: torch.Tensor,
frame_rate: int,
filename: str,
crf: Optional[int],
crf: Optional[int] = None,
pixel_format: Optional[str] = None,
) -> None:
return

Expand All @@ -328,7 +332,8 @@ def encode_video_to_tensor_abstract(
frames: torch.Tensor,
frame_rate: int,
format: str,
crf: Optional[int],
crf: Optional[int] = None,
pixel_format: Optional[str] = None,
) -> torch.Tensor:
return torch.empty([], dtype=torch.long)

Expand All @@ -340,6 +345,7 @@ def _encode_video_to_file_like_abstract(
format: str,
file_like_context: int,
crf: Optional[int] = None,
pixel_format: Optional[str] = None,
) -> None:
return

Expand Down
17 changes: 16 additions & 1 deletion src/torchcodec/encoders/_video_encoder.py
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
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Expand All @@ -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.

Expand All @@ -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,
)
24 changes: 24 additions & 0 deletions test/test_encoders.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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")

...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

with pytest.raises(RuntimeError, match="bit_rate=-1 must be >= 0"):
getattr(decoder, method)(**valid_params, bit_rate=-1)

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 VideoEncoder::initializeEncoder.


@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
Expand Down
Loading
Loading