Enable local builds with FFmpeg8 #935
Conversation
meta-cla
bot
added
the
CLA Signed
There was a problem hiding this comment.
Thanks for the PR @Dan-Flores !
I am looking at https://lists.ffmpeg.org/pipermail/ffmpeg-cvslog/2024-September/145629.html which deprecated av_opt_set_int_list and it says:
lavu: deprecate av_opt_set_int_list() and related infrastructure
It has no more users and is replaced by array-type options.
Looking at https://ffmpeg.org/doxygen/8.0/group__opt__set__funcs.html I think the "array-type" options refer to av_opt_set_array()
and something like this compiles fine on my side:
diff --git a/src/torchcodec/_core/FilterGraph.cpp b/src/torchcodec/_core/FilterGraph.cpp
index 70bc2b5..333b19c 100644
--- a/src/torchcodec/_core/FilterGraph.cpp
+++ b/src/torchcodec/_core/FilterGraph.cpp
@@ -100,15 +100,16 @@ FilterGraph::FilterGraph(
"Failed to create filter graph: ",
getFFMPEGErrorStringFromErrorCode(status));
- enum AVPixelFormat pix_fmts[] = {
- filtersContext.outputFormat, AV_PIX_FMT_NONE};
+ AVPixelFormat formats[] = {filtersContext.outputFormat, AV_PIX_FMT_NONE};
- status = av_opt_set_int_list(
+ status = av_opt_set_array(
sinkContext_,
- "pix_fmts",
- pix_fmts,
- AV_PIX_FMT_NONE,
- AV_OPT_SEARCH_CHILDREN);
+ "pixel_formats",
+ AV_OPT_SEARCH_CHILDREN,
+ 0, // start_elem
+ 2, // nb_elems
+ AV_OPT_TYPE_INT,
+ formats);
TORCH_CHECK(
status >= 0,
"Failed to set output pixel formats: ",If this works, it might be simpler alternative to using av_opt_set, since av_opt_set forces us to take a detour with format names. Do you mind trying that out and let me know if it properly works?
Thanks for this suggestion! I've updated my PR to use |
| if (!sinkContext) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Try to TORCH_CHECK early here (and elsewhere) instead of returning a nullptr, it'll make it easier to figure out where the error happens.
| if (status < 0) { | ||
| return nullptr; | ||
| } | ||
| status = avfilter_init_str(sinkContext, nullptr); |
There was a problem hiding this comment.
Do we need this? In #935 (review) the only thing we had to change was to replace av_opt_set_int_list with av_opt_set_array, but I think we can still rely on avfilter_graph_create_filter for FFmpeg 8 ? Same above with avfilter_graph_alloc_filter which may not be needed?
There was a problem hiding this comment.
Unfortunately the new flow of allocation -> set flags -> init is required to set the pixel_formats flag. If we try to retain the previous pattern, this error appears:
Option 'pixel_formats' is not a runtime option and so cannot be set after the object has been initialized
There was a problem hiding this comment.
For my own ref, claude is telling me that the way you've done it above is the actual new recommended way:
● Based on my research, here's the answer to your question about FFmpeg 8 changes and simpler alternatives:
Why avfilter_init_str() is Required in FFmpeg 8
The use of avfilter_init_str() is unavoidable and actually represents the correct, simplified approach for FFmpeg
8. Here's why:
The API Change
FFmpeg 8 deprecated av_opt_set_int_list() and replaced it with array-type options that require a two-step
initialization process:
1. Allocate filter context: avfilter_graph_alloc_filter()
2. Set array options: av_opt_set_array()
3. Initialize filter: avfilter_init_str() or avfilter_init_dict()
Why This is Actually Simpler
Your current implementation is the simplified approach. The alternative would be more complex:
Alternative 1: String-based (less flexible)
// Would require converting enum to string manually
char pix_fmt_str[32];
av_get_pix_fmt_string(pix_fmt_str, sizeof(pix_fmt_str), outputFormat);
avfilter_graph_create_filter(&sinkContext, buffersink, name,
"pixel_formats=yuv420p", nullptr, filterGraph);
Alternative 2: Dictionary-based (more verbose)
AVDictionary* opts = nullptr;
char pix_fmt_str[32];
av_get_pix_fmt_string(pix_fmt_str, sizeof(pix_fmt_str), outputFormat);
av_dict_set(&opts, "pixel_formats", pix_fmt_str, 0);
sinkContext = avfilter_graph_alloc_filter(filterGraph, buffersink, name);
avfilter_init_dict(sinkContext, &opts);
av_dict_free(&opts);
Your Current Implementation is Optimal
The pattern you have is the recommended FFmpeg 8 approach:
sinkContext = avfilter_graph_alloc_filter(filterGraph, buffersink, name);
av_opt_set_array(sinkContext, "pixel_formats", AV_OPT_SEARCH_CHILDREN,
0, 1, AV_OPT_TYPE_PIXEL_FMT, &outputFormat);
avfilter_init_str(sinkContext, nullptr);
This is simpler because:
- Type-safe: Uses AV_OPT_TYPE_PIXEL_FMT directly
- No string conversion: Avoids enum-to-string conversion
- Clean separation: Options set before initialization
- Minimal code: Fewer lines than alternatives
Recommendation
Keep your current implementation. The avfilter_init_str() call is not a workaround—it's the intended FFmpeg 8 API
design. The separation of allocation, option-setting, and initialization provides better error handling and
flexibility than the older combined approach.
The FFmpeg 8 changes actually made the API more consistent, even if it requires one additional function call.
| AVFilterContext* sinkContext = nullptr; | ||
| int status; | ||
|
|
||
| // av_opt_set_int_list was replaced by av_opt_set() in FFmpeg 8. |
There was a problem hiding this comment.
| // av_opt_set_int_list was replaced by av_opt_set() in FFmpeg 8. | |
| // av_opt_set_int_list was replaced by av_opt_set_array() in FFmpeg 8. |
| 0, // start_elem | ||
| 1, // nb_elems | ||
| AV_OPT_TYPE_PIXEL_FMT, | ||
| &outputFormat); |
There was a problem hiding this comment.
Should we pass enum AVPixelFormat pix_fmts[] = {outputFormat, AV_PIX_FMT_NONE}; like we were doing it below instead of just passing outputFormat?
Unless there's a good reason not to, in doubt, I think we should do the same.
|
|
||
| AVFilterContext* createBuffersinkFilter( | ||
| AVFilterGraph* filterGraph, | ||
| const char* name, |
There was a problem hiding this comment.
we're always passing "out" here, so let's just remove this parameter and hard-code "out" below. If we ever need another name, we can add the parameter then.
| if (status < 0) { | ||
| return nullptr; | ||
| } | ||
| status = avfilter_init_str(sinkContext, nullptr); |
There was a problem hiding this comment.
For my own ref, claude is telling me that the way you've done it above is the actual new recommended way:
● Based on my research, here's the answer to your question about FFmpeg 8 changes and simpler alternatives:
Why avfilter_init_str() is Required in FFmpeg 8
The use of avfilter_init_str() is unavoidable and actually represents the correct, simplified approach for FFmpeg
8. Here's why:
The API Change
FFmpeg 8 deprecated av_opt_set_int_list() and replaced it with array-type options that require a two-step
initialization process:
1. Allocate filter context: avfilter_graph_alloc_filter()
2. Set array options: av_opt_set_array()
3. Initialize filter: avfilter_init_str() or avfilter_init_dict()
Why This is Actually Simpler
Your current implementation is the simplified approach. The alternative would be more complex:
Alternative 1: String-based (less flexible)
// Would require converting enum to string manually
char pix_fmt_str[32];
av_get_pix_fmt_string(pix_fmt_str, sizeof(pix_fmt_str), outputFormat);
avfilter_graph_create_filter(&sinkContext, buffersink, name,
"pixel_formats=yuv420p", nullptr, filterGraph);
Alternative 2: Dictionary-based (more verbose)
AVDictionary* opts = nullptr;
char pix_fmt_str[32];
av_get_pix_fmt_string(pix_fmt_str, sizeof(pix_fmt_str), outputFormat);
av_dict_set(&opts, "pixel_formats", pix_fmt_str, 0);
sinkContext = avfilter_graph_alloc_filter(filterGraph, buffersink, name);
avfilter_init_dict(sinkContext, &opts);
av_dict_free(&opts);
Your Current Implementation is Optimal
The pattern you have is the recommended FFmpeg 8 approach:
sinkContext = avfilter_graph_alloc_filter(filterGraph, buffersink, name);
av_opt_set_array(sinkContext, "pixel_formats", AV_OPT_SEARCH_CHILDREN,
0, 1, AV_OPT_TYPE_PIXEL_FMT, &outputFormat);
avfilter_init_str(sinkContext, nullptr);
This is simpler because:
- Type-safe: Uses AV_OPT_TYPE_PIXEL_FMT directly
- No string conversion: Avoids enum-to-string conversion
- Clean separation: Options set before initialization
- Minimal code: Fewer lines than alternatives
Recommendation
Keep your current implementation. The avfilter_init_str() call is not a workaround—it's the intended FFmpeg 8 API
design. The separation of allocation, option-setting, and initialization provides better error handling and
flexibility than the older combined approach.
The FFmpeg 8 changes actually made the API more consistent, even if it requires one additional function call.
| "pixel_formats", | ||
| AV_OPT_SEARCH_CHILDREN, | ||
| 0, // start_elem | ||
| 1, // nb_elems |
There was a problem hiding this comment.
I think this needs to be set to 2 now
| 1, // nb_elems | |
| 2, // nb_elems |
There was a problem hiding this comment.
This is actually ok at 1, we do not want to set AV_PIX_FMT_NONE, only the first real pixel format. It seems AV_PIX_FMT_NONE is used to indicate the end of the array, as per claude:
FFmpeg <= 7: av_opt_set_int_list()
enum AVPixelFormat pix_fmts[] = {outputFormat, AV_PIX_FMT_NONE};
av_opt_set_int_list(..., pix_fmts, AV_PIX_FMT_NONE, ...);
- Uses sentinel-terminated arrays
- AV_PIX_FMT_NONE (-1) is the terminator, not a value to set
- Only sets 1 pixel format (stops when it hits the terminator)
FFmpeg 8: av_opt_set_array()
av_opt_set_array(..., nb_elems=2, ..., &outputFormat);
- Uses count-based arrays (no sentinel)
- Reads exactly nb_elems values from memory
- With nb_elems=2, it reads:
a. Element 0: outputFormat ✓
b. Element 1: Memory after outputFormat (could be -1 or garbage)
If element 1 is -1 (AV_PIX_FMT_NONE), FFmpeg 8's validation rejects it as out of range [0 - 2.14748e+09], because -1 is now treated as an invalid value, not a
terminator.
Why the Error
FFmpeg 8 validates array elements more strictly. Negative values like AV_PIX_FMT_NONE (-1) are rejected, causing:
Cannot set array element 1 for parameter 'pixel_formats': value -1.000000 out of range
Solution
Set nb_elems=1 to only pass the single valid pixel format, not the terminator.
There was a problem hiding this comment.
OK that sounds good, let's just add a comment to that effect
This PR adds 8 as an option in
load_torchcodec_shared_libraries()and_core/CMakeLists.txtto enable FFmpeg8 builds.In addition, to enable local builds with FFmpeg8, we need to update our pixel_format setting approach.
Old flow (<= FFmpeg7) :
avfilter_graph_create_filter()av_opt_set_int_list.New flow (>= FFmpeg8) :
avfilter_graph_alloc_filterav_opt_set_arrayavfilter_init_str.These changes are encapsulated
FFmpegCommon.hincreateBuffersinkFilter().