Add pixel_format to VideoEncoder API

[Dan-Flores]

This PR updates VideoEncoder API to accept an optional pixel_format.

• Remove avcodec_find_best_pix_fmt_of_list, instead validate and use provided pixel format.
  • If no pixel_format provided, use default pixel format. Often this is yuv420p, but not always (ex. gif codec).
• Error message appears similar to FFmpeg's internal logged error to help users:

       RuntimeError: Unknown pixel format: invalid_pix_fmt
       Supported pixel formats for libx264: yuv420p yuvj420p yuv422p yuvj422p yuv444p yuvj444p nv12 nv16 nv21 yuv420p10le yuv422p10le yuv444p10le nv20le gray gray10le

[Dan-Flores]

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.

[NicolasHug]

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

	// 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;

[Dan-Flores]

gif only supports rgb pixel formats, this test is now focused on the more common yuv formats.

[NicolasHug]

Nicely done @Dan-Flores , thank you!

[NicolasHug]

Good job on making this a keyword only params 👍

[NicolasHug]

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

	// 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;

[NicolasHug]

Before we were using avcodec_find_best_pix_fmt_of_list, now our heuristic is to return the first format in the list. Do I understand correctly that the reason for this change is that you have empirically observe that the first in the list is often yuv420p, which is a good default?

I think it makes sense, just confirming my understanding. IT might be worth making that very explicit through a comment explaining that yuv420p is often the first entry

[Dan-Flores]

Yes, yuv420p is often the first in the list, and FFmpeg's avcodec_default_get_format uses the same heuristic.

I'll add a comment here to mention this as well

[scotts]

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

...

[Dan-Flores]

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

	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.

[scotts]

Missing = None?

[Dan-Flores]

Perhaps - I'm not sure if they are needed for the @register_fake annotated functions. I'll add them in case.

[scotts]

Missing = None?