-
Notifications
You must be signed in to change notification settings - Fork 64
Enable local builds with FFmpeg8 #935
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
Changes from all commits
a132fe5
9e31132
fddf741
c4d0df8
4d41abb
c5fa1e1
7f96f98
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 |
---|---|---|
|
@@ -8,6 +8,11 @@ | |
|
||
#include <c10/util/Exception.h> | ||
|
||
extern "C" { | ||
#include <libavfilter/avfilter.h> | ||
#include <libavfilter/buffersink.h> | ||
} | ||
|
||
namespace facebook::torchcodec { | ||
|
||
AutoAVPacket::AutoAVPacket() : avPacket_(av_packet_alloc()) { | ||
|
@@ -374,6 +379,70 @@ SwrContext* createSwrContext( | |
return swrContext; | ||
} | ||
|
||
AVFilterContext* createBuffersinkFilter( | ||
AVFilterGraph* filterGraph, | ||
enum AVPixelFormat outputFormat) { | ||
const AVFilter* buffersink = avfilter_get_by_name("buffersink"); | ||
TORCH_CHECK(buffersink != nullptr, "Failed to get buffersink filter."); | ||
|
||
AVFilterContext* sinkContext = nullptr; | ||
int status; | ||
const char* filterName = "out"; | ||
|
||
enum AVPixelFormat pix_fmts[] = {outputFormat, AV_PIX_FMT_NONE}; | ||
|
||
// av_opt_set_int_list was replaced by av_opt_set_array() in FFmpeg 8. | ||
#if LIBAVUTIL_VERSION_MAJOR >= 60 // FFmpeg >= 8 | ||
// Output options like pixel_formats must be set before filter init | ||
sinkContext = | ||
avfilter_graph_alloc_filter(filterGraph, buffersink, filterName); | ||
TORCH_CHECK( | ||
sinkContext != nullptr, "Failed to allocate buffersink filter context."); | ||
|
||
// When setting pix_fmts, only the first element is used, so nb_elems = 1 | ||
// AV_PIX_FMT_NONE acts as a terminator for the array in av_opt_set_int_list | ||
status = av_opt_set_array( | ||
sinkContext, | ||
"pixel_formats", | ||
AV_OPT_SEARCH_CHILDREN, | ||
0, // start_elem | ||
1, // nb_elems | ||
AV_OPT_TYPE_PIXEL_FMT, | ||
pix_fmts); | ||
TORCH_CHECK( | ||
status >= 0, | ||
"Failed to set pixel format for buffersink filter: ", | ||
getFFMPEGErrorStringFromErrorCode(status)); | ||
|
||
status = avfilter_init_str(sinkContext, nullptr); | ||
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. Do we need this? In #935 (review) the only thing we had to change was to replace 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. Unfortunately the new flow of allocation -> set flags -> init is required to set the 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. For my own ref,
|
||
TORCH_CHECK( | ||
status >= 0, | ||
"Failed to initialize buffersink filter: ", | ||
getFFMPEGErrorStringFromErrorCode(status)); | ||
#else // FFmpeg <= 7 | ||
// For older FFmpeg versions, create filter and then set options | ||
status = avfilter_graph_create_filter( | ||
&sinkContext, buffersink, filterName, nullptr, nullptr, filterGraph); | ||
TORCH_CHECK( | ||
status >= 0, | ||
"Failed to create buffersink filter: ", | ||
getFFMPEGErrorStringFromErrorCode(status)); | ||
|
||
status = av_opt_set_int_list( | ||
sinkContext, | ||
"pix_fmts", | ||
pix_fmts, | ||
AV_PIX_FMT_NONE, | ||
AV_OPT_SEARCH_CHILDREN); | ||
TORCH_CHECK( | ||
status >= 0, | ||
"Failed to set pixel formats for buffersink filter: ", | ||
getFFMPEGErrorStringFromErrorCode(status)); | ||
#endif | ||
|
||
return sinkContext; | ||
} | ||
|
||
UniqueAVFrame convertAudioAVFrameSamples( | ||
const UniqueSwrContext& swrContext, | ||
const UniqueAVFrame& srcAVFrame, | ||
|
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.
I think this needs to be set to 2 now
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.
This is actually ok at 1, we do not want to set
AV_PIX_FMT_NONE
, only the first real pixel format. It seemsAV_PIX_FMT_NONE
is used to indicate the end of the array, as perclaude
: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, ...);
FFmpeg 8: av_opt_set_array()
av_opt_set_array(..., nb_elems=2, ..., &outputFormat);
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that sounds good, let's just add a comment to that effect