Niklas Haas via ffmpeg-devel (HE12025-11-06): > PR #20851 opened by Niklas Haas (haasn) > URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20851 > Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20851.patch > > The current code only ever printed the pixel format; even when the > negotiation error was in a completely different format.
Looks useful. Note: AVWriter would have made the code more elegant. > > > >From c1717cb6669f986a8bd04c8bfc5e37927c12769d Mon Sep 17 00:00:00 2001 > From: Niklas Haas <[email protected]> > Date: Thu, 6 Nov 2025 17:26:19 +0100 > Subject: [PATCH 1/5] avfilter/format: add print_list() to > AVFilterFormatsMerger > > So that the generic code can correctly print format lists for failing mergers. > --- > libavfilter/formats.c | 41 +++++++++++++++++++++++++++++++++++++++++ > libavfilter/formats.h | 3 +++ > 2 files changed, 44 insertions(+) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index 847199dda1..b9487fe0cf 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -20,6 +20,7 @@ > */ > > #include "libavutil/avassert.h" > +#include "libavutil/bprint.h" > #include "libavutil/channel_layout.h" > #include "libavutil/common.h" > #include "libavutil/mem.h" > @@ -345,6 +346,39 @@ static int merge_generic(void *a, void *b) > return merge_generic_internal(a, b, 0); > } > > +#define PRINT_NAME(type, type_name) > \ > +static void print_##type_name(AVBPrint *bp, const void *fmtsp) > \ > +{ > \ > + const AVFilterFormats *fmts = fmtsp; > \ > + for (int i = 0; i < fmts->nb_formats; i++) { > \ > + const char *name = av_##type_name(fmts->formats[i]); > \ > + av_bprint_chars(bp, ' ', i ? 1 : 0); > \ > + av_bprint_append_data(bp, name, name ? strlen(name) : 0); > \ I think some kind of placeholder, maybe just '?', would be better. > + } > \ > +} > + > +PRINT_NAME(enum AVSampleFormat, get_sample_fmt_name) > +PRINT_NAME(enum AVPixelFormat, get_pix_fmt_name) > +PRINT_NAME(enum AVColorSpace, color_space_name) > +PRINT_NAME(enum AVColorRange, color_range_name) > +PRINT_NAME(enum AVAlphaMode, alpha_mode_name) > + > +static void print_channel_layout_desc(AVBPrint *bp, const void *layoutsp) > +{ > + const AVFilterChannelLayouts *layouts = layoutsp; > + for (int i = 0; i < layouts->nb_channel_layouts; i++) { > + av_bprint_chars(bp, ' ', i ? 1 : 0); > + av_channel_layout_describe_bprint(&layouts->channel_layouts[i], bp); > + } > +} > + > +static void print_sample_rate(AVBPrint *bp, const void *ratesp) > +{ > + const AVFilterFormats *rates = ratesp; > + for (int i = 0; i < rates->nb_formats; i++) > + av_bprintf(bp, "%s%d", i ? " " : "", rates->formats[i]); > +} > + > #define CONVERSION_FILTER_SWSCALE \ > .conversion_filter = "scale", \ > .conversion_opts_offset = offsetof(AVFilterGraph, scale_sws_opts), > @@ -358,24 +392,28 @@ static const AVFilterFormatsMerger mergers_video[] = { > .offset = offsetof(AVFilterFormatsConfig, formats), > .merge = merge_pix_fmts, > .can_merge = can_merge_pix_fmts, > + .print_list = print_get_pix_fmt_name, > CONVERSION_FILTER_SWSCALE > }, > { > .offset = offsetof(AVFilterFormatsConfig, color_spaces), > .merge = merge_generic, > .can_merge = can_merge_generic, > + .print_list = print_color_space_name, > CONVERSION_FILTER_SWSCALE > }, > { > .offset = offsetof(AVFilterFormatsConfig, color_ranges), > .merge = merge_generic, > .can_merge = can_merge_generic, > + .print_list = print_color_range_name, > CONVERSION_FILTER_SWSCALE > }, > { > .offset = offsetof(AVFilterFormatsConfig, alpha_modes), > .merge = merge_generic, > .can_merge = can_merge_generic, > + .print_list = print_alpha_mode_name, > .conversion_filter = "premultiply_dynamic", > }, > }; > @@ -385,18 +423,21 @@ static const AVFilterFormatsMerger mergers_audio[] = { > .offset = offsetof(AVFilterFormatsConfig, channel_layouts), > .merge = merge_channel_layouts, > .can_merge = can_merge_channel_layouts, > + .print_list = print_channel_layout_desc, > CONVERSION_FILTER_ARESAMPLE > }, > { > .offset = offsetof(AVFilterFormatsConfig, samplerates), > .merge = merge_samplerates, > .can_merge = can_merge_samplerates, > + .print_list = print_sample_rate, > CONVERSION_FILTER_ARESAMPLE > }, > { > .offset = offsetof(AVFilterFormatsConfig, formats), > .merge = merge_sample_fmts, > .can_merge = can_merge_sample_fmts, > + .print_list = print_get_sample_fmt_name, > CONVERSION_FILTER_ARESAMPLE > }, > }; > diff --git a/libavfilter/formats.h b/libavfilter/formats.h > index 0c92ecad3f..d0b2a0b96a 100644 > --- a/libavfilter/formats.h > +++ b/libavfilter/formats.h > @@ -513,10 +513,13 @@ int ff_formats_check_color_ranges(void *log, const > AVFilterFormats *fmts); > */ > int ff_formats_check_alpha_modes(void *log, const AVFilterFormats *fmts); > > +struct AVBPrint; > + > typedef struct AVFilterFormatMerger { > unsigned offset; > int (*merge)(void *a, void *b); > int (*can_merge)(const void *a, const void *b); > + void (*print_list)(struct AVBPrint *bp, const void *fmts); > const char *conversion_filter; > unsigned conversion_opts_offset; > } AVFilterFormatsMerger; > -- > 2.49.1 > > > >From f5bc9704edb3254e65c108509052f19eb77622c1 Mon Sep 17 00:00:00 2001 > From: Niklas Haas <[email protected]> > Date: Thu, 6 Nov 2025 17:48:32 +0100 > Subject: [PATCH 2/5] avfilter/formats: constify ff_filter_get_negotiation > > --- > libavfilter/formats.c | 2 +- > libavfilter/formats.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index b9487fe0cf..36e45c53c1 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -452,7 +452,7 @@ static const AVFilterNegotiation negotiate_audio = { > .mergers = mergers_audio, > }; > > -const AVFilterNegotiation *ff_filter_get_negotiation(AVFilterLink *link) > +const AVFilterNegotiation *ff_filter_get_negotiation(const AVFilterLink > *link) > { > switch (link->type) { > case AVMEDIA_TYPE_VIDEO: return &negotiate_video; > diff --git a/libavfilter/formats.h b/libavfilter/formats.h > index d0b2a0b96a..4fe96187ba 100644 > --- a/libavfilter/formats.h > +++ b/libavfilter/formats.h > @@ -614,6 +614,6 @@ typedef struct AVFilterNegotiation { > const AVFilterFormatsMerger *mergers; > } AVFilterNegotiation; > > -const AVFilterNegotiation *ff_filter_get_negotiation(AVFilterLink *link); > +const AVFilterNegotiation *ff_filter_get_negotiation(const AVFilterLink > *link); > > #endif /* AVFILTER_FORMATS_H */ > -- > 2.49.1 > > > >From ad5b151f88aed49b4c36e764186214c13e0788ac Mon Sep 17 00:00:00 2001 > From: Niklas Haas <[email protected]> > Date: Thu, 6 Nov 2025 17:58:10 +0100 > Subject: [PATCH 3/5] avfilter/formats: add name field to AVFilterFormatMerger > > Needed to properly print format lists on format configuration failure. > --- > libavfilter/formats.c | 7 +++++++ > libavfilter/formats.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index 36e45c53c1..5ba961f059 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -389,6 +389,7 @@ static void print_sample_rate(AVBPrint *bp, const void > *ratesp) > > static const AVFilterFormatsMerger mergers_video[] = { > { > + .name = "Pixel formats", > .offset = offsetof(AVFilterFormatsConfig, formats), > .merge = merge_pix_fmts, > .can_merge = can_merge_pix_fmts, > @@ -396,6 +397,7 @@ static const AVFilterFormatsMerger mergers_video[] = { > CONVERSION_FILTER_SWSCALE > }, > { > + .name = "Color spaces", > .offset = offsetof(AVFilterFormatsConfig, color_spaces), > .merge = merge_generic, > .can_merge = can_merge_generic, > @@ -403,6 +405,7 @@ static const AVFilterFormatsMerger mergers_video[] = { > CONVERSION_FILTER_SWSCALE > }, > { > + .name = "Color ranges", > .offset = offsetof(AVFilterFormatsConfig, color_ranges), > .merge = merge_generic, > .can_merge = can_merge_generic, > @@ -410,6 +413,7 @@ static const AVFilterFormatsMerger mergers_video[] = { > CONVERSION_FILTER_SWSCALE > }, > { > + .name = "Alpha modes", > .offset = offsetof(AVFilterFormatsConfig, alpha_modes), > .merge = merge_generic, > .can_merge = can_merge_generic, > @@ -420,6 +424,7 @@ static const AVFilterFormatsMerger mergers_video[] = { > > static const AVFilterFormatsMerger mergers_audio[] = { > { > + .name = "Channel layouts", > .offset = offsetof(AVFilterFormatsConfig, channel_layouts), > .merge = merge_channel_layouts, > .can_merge = can_merge_channel_layouts, > @@ -427,6 +432,7 @@ static const AVFilterFormatsMerger mergers_audio[] = { > CONVERSION_FILTER_ARESAMPLE > }, > { > + .name = "Sample rates", > .offset = offsetof(AVFilterFormatsConfig, samplerates), > .merge = merge_samplerates, > .can_merge = can_merge_samplerates, > @@ -434,6 +440,7 @@ static const AVFilterFormatsMerger mergers_audio[] = { > CONVERSION_FILTER_ARESAMPLE > }, > { > + .name = "Sample formats", > .offset = offsetof(AVFilterFormatsConfig, formats), > .merge = merge_sample_fmts, > .can_merge = can_merge_sample_fmts, > diff --git a/libavfilter/formats.h b/libavfilter/formats.h > index 4fe96187ba..969bb230f1 100644 > --- a/libavfilter/formats.h > +++ b/libavfilter/formats.h > @@ -516,6 +516,7 @@ int ff_formats_check_alpha_modes(void *log, const > AVFilterFormats *fmts); > struct AVBPrint; > > typedef struct AVFilterFormatMerger { > + const char *name; > unsigned offset; > int (*merge)(void *a, void *b); > int (*can_merge)(const void *a, const void *b); > -- > 2.49.1 > > > >From 7b564e2efc295ca205642c5c2ddfe5d8e77ecf08 Mon Sep 17 00:00:00 2001 > From: Niklas Haas <[email protected]> > Date: Thu, 6 Nov 2025 18:01:37 +0100 > Subject: [PATCH 4/5] avfilter/avfiltergraph: print all format lists on config > failure > > Instead of just printing the pixel/sample formats. > --- > libavfilter/avfiltergraph.c | 87 +++++++++++++++++++++---------------- > 1 file changed, 50 insertions(+), 37 deletions(-) > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > index 4c80204f01..15978fc7a8 100644 > --- a/libavfilter/avfiltergraph.c > +++ b/libavfilter/avfiltergraph.c > @@ -443,44 +443,33 @@ static int formats_declared(AVFilterContext *f) > return 1; > } > > -static void print_formats(void *log_ctx, int level, enum AVMediaType type, > - const AVFilterFormats *formats) > -{ > - AVBPrint bp; > - av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); > - > - switch (type) { > - case AVMEDIA_TYPE_VIDEO: > - for (unsigned i = 0; i < formats->nb_formats; i++) > - av_bprintf(&bp, "%s%s", bp.len ? " " : "", > av_get_pix_fmt_name(formats->formats[i])); > - break; > - case AVMEDIA_TYPE_AUDIO: > - for (unsigned i = 0; i < formats->nb_formats; i++) > - av_bprintf(&bp, "%s%s", bp.len ? " " : "", > av_get_sample_fmt_name(formats->formats[i])); > - break; > - default: > - av_bprintf(&bp, "(unknown)"); > - break; > - } > - > - if (av_bprint_is_complete(&bp)) { > - av_log(log_ctx, level, "%s\n", bp.str); > - } else { > - av_log(log_ctx, level, "(out of memory)\n"); > - } > - av_bprint_finalize(&bp, NULL); > -} > - > static void print_link_formats(void *log_ctx, int level, const AVFilterLink > *l) > { > if (av_log_get_level() < level) > return; > > - av_log(log_ctx, level, "Link '%s.%s' -> '%s.%s':\n" > - " src: ", l->src->name, l->srcpad->name, > l->dst->name, l->dstpad->name); > - print_formats(log_ctx, level, l->type, l->incfg.formats); > - av_log(log_ctx, level, " dst: "); > - print_formats(log_ctx, level, l->type, l->outcfg.formats); > + AVBPrint bp; > + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); > + > + av_log(log_ctx, level, "Link '%s.%s' -> '%s.%s':\n", > + l->src->name, l->srcpad->name, l->dst->name, l->dstpad->name); > + > + const AVFilterNegotiation *neg = ff_filter_get_negotiation(l); > + for (unsigned i = 0; i < neg->nb_mergers; i++) { > + const AVFilterFormatsMerger *m = &neg->mergers[i]; > + av_log(log_ctx, level, " %s:\n", m->name); > + m->print_list(&bp, FF_FIELD_AT(void *, m->offset, l->incfg)); > + if (av_bprint_is_complete(&bp)) > + av_log(log_ctx, level, " src: %s\n", bp.str); Do not silently skip this if allocation failed. Same below. > + av_bprint_clear(&bp); > + > + m->print_list(&bp, FF_FIELD_AT(void *, m->offset, l->outcfg)); > + if (av_bprint_is_complete(&bp)) > + av_log(log_ctx, level, " dst: %s\n", bp.str); > + av_bprint_clear(&bp); > + } > + > + av_bprint_finalize(&bp, NULL); > } > > static void print_filter_formats(void *log_ctx, int level, const > AVFilterContext *f) > @@ -488,15 +477,39 @@ static void print_filter_formats(void *log_ctx, int > level, const AVFilterContext > if (av_log_get_level() < level) > return; > > + AVBPrint bp; > + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED); > + > av_log(log_ctx, level, "Filter '%s' formats:\n", f->name); > for (int i = 0; i < f->nb_inputs; i++) { > - av_log(log_ctx, level, " in[%d] '%s': ", i, f->input_pads[i].name); > - print_formats(log_ctx, level, f->inputs[i]->type, > f->inputs[i]->outcfg.formats); > + const AVFilterLink *in = f->inputs[i]; > + const AVFilterNegotiation *neg = ff_filter_get_negotiation(in); > + av_log(log_ctx, level, " in[%d] '%s':", i, f->input_pads[i].name); > + > + for (unsigned i = 0; i < neg->nb_mergers; i++) { > + const AVFilterFormatsMerger *m = &neg->mergers[i]; > + m->print_list(&bp, FF_FIELD_AT(void *, m->offset, in->outcfg)); > + if (av_bprint_is_complete(&bp)) > + av_log(log_ctx, level, " %s: %s", m->name, bp.str); > + av_bprint_clear(&bp); > + } > } > + > for (int i = 0; i < f->nb_outputs; i++) { > - av_log(log_ctx, level, " out[%d] '%s': ", i, > f->output_pads[i].name); > - print_formats(log_ctx, level, f->outputs[i]->type, > f->outputs[i]->incfg.formats); > + const AVFilterLink *out = f->outputs[i]; > + const AVFilterNegotiation *neg = ff_filter_get_negotiation(out); > + av_log(log_ctx, level, " out[%d] '%s':", i, f->output_pads[i].name); > + > + for (unsigned i = 0; i < neg->nb_mergers; i++) { > + const AVFilterFormatsMerger *m = &neg->mergers[i]; > + m->print_list(&bp, FF_FIELD_AT(void *, m->offset, out->incfg)); > + if (av_bprint_is_complete(&bp)) > + av_log(log_ctx, level, " %s: %s", m->name, bp.str); > + av_bprint_clear(&bp); > + } Can these two blocks be merged more? > } > + > + av_bprint_finalize(&bp, NULL); > } > > /** > -- > 2.49.1 > > > >From 6c3a63112be927526b08c0154b2df786fa7bce3d Mon Sep 17 00:00:00 2001 > From: Niklas Haas <[email protected]> > Date: Thu, 6 Nov 2025 18:09:36 +0100 > Subject: [PATCH 5/5] avfilter/avfiltergraph: only print format lists for > failing mergers > > Instead of printing all format lists on a link negotiation error, just print > the relevant/failing format lists. > --- > libavfilter/avfiltergraph.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > index 15978fc7a8..285a701d09 100644 > --- a/libavfilter/avfiltergraph.c > +++ b/libavfilter/avfiltergraph.c > @@ -443,7 +443,9 @@ static int formats_declared(AVFilterContext *f) > return 1; > } > > -static void print_link_formats(void *log_ctx, int level, const AVFilterLink > *l) > +static void print_link_formats(void *log_ctx, int level, const AVFilterLink > *l, > + const AVFilterFormatsMerger *mergers[], > + int nb_mergers) > { > if (av_log_get_level() < level) > return; > @@ -454,9 +456,8 @@ static void print_link_formats(void *log_ctx, int level, > const AVFilterLink *l) > av_log(log_ctx, level, "Link '%s.%s' -> '%s.%s':\n", > l->src->name, l->srcpad->name, l->dst->name, l->dstpad->name); > > - const AVFilterNegotiation *neg = ff_filter_get_negotiation(l); > - for (unsigned i = 0; i < neg->nb_mergers; i++) { > - const AVFilterFormatsMerger *m = &neg->mergers[i]; > + for (unsigned i = 0; i < nb_mergers; i++) { > + const AVFilterFormatsMerger *m = mergers[i]; > av_log(log_ctx, level, " %s:\n", m->name); > m->print_list(&bp, FF_FIELD_AT(void *, m->offset, l->incfg)); > if (av_bprint_is_complete(&bp)) > @@ -554,8 +555,9 @@ retry: > AVFilterLink *link = filter->inputs[j]; > const AVFilterNegotiation *neg; > AVFilterContext *conv[4]; > + const AVFilterFormatsMerger *mergers[4]; /* triggered mergers */ > const char *conv_filters[4], *conv_opts[4] = {0}; It looks like some code was pushed while I was not looking. Please fix these duplicately-hard-coded sizes. (I will need to check and fix what else might have been pushed.) > - unsigned neg_step, num_conv = 0; > + unsigned neg_step, num_conv = 0, num_mergers = 0; > > if (!link) > continue; > @@ -578,6 +580,8 @@ retry: > conv_opts[num_conv] = FF_FIELD_AT(char *, > m->conversion_opts_offset, *graph); > num_conv++; > } > + av_assert1(num_mergers < FF_ARRAY_ELEMS(mergers)); > + mergers[num_mergers++] = m; > } > } > for (neg_step = 0; neg_step < neg->nb_mergers; neg_step++) { > @@ -594,6 +598,7 @@ retry: > if (ret < 0) > return ret; > if (!ret) { > + mergers[num_mergers++] = m; > conv_filters[num_conv] = m->conversion_filter; > if (m->conversion_opts_offset) > conv_opts[num_conv] = FF_FIELD_AT(char *, > m->conversion_opts_offset, *graph); > @@ -616,7 +621,7 @@ retry: > "The filters '%s' and '%s' do not have a common > format " > "and automatic conversion is disabled.\n", > link->src->name, link->dst->name); > - print_link_formats(log_ctx, AV_LOG_ERROR, link); > + print_link_formats(log_ctx, AV_LOG_ERROR, link, mergers, > num_mergers); > return AVERROR(EINVAL); > } > > @@ -624,7 +629,7 @@ retry: > av_log(log_ctx, AV_LOG_ERROR, > "'%s' filter not present, cannot convert > formats.\n", > conv_filters[k]); > - print_link_formats(log_ctx, AV_LOG_ERROR, link); > + print_link_formats(log_ctx, AV_LOG_ERROR, link, mergers, > num_mergers); > return AVERROR(EINVAL); > } > snprintf(inst_name, sizeof(inst_name), "auto_%s_%d", > @@ -687,7 +692,7 @@ retry: > av_log(log_ctx, AV_LOG_ERROR, > "Impossible to convert between the formats > supported by the filter " > "'%s' and the filter '%s'\n", > link->src->name, link->dst->name); > - print_link_formats(log_ctx, AV_LOG_ERROR, link); > + print_link_formats(log_ctx, AV_LOG_ERROR, link, &m, > 1); > return AVERROR(ENOSYS); > } else { > count_merged += 2; Regards, -- Nicolas George _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
