[FFmpeg-devel] [PATCH 09/10] avfilter/formats: Don't set samplerate or channel count on video links
Nicolas George
george at nsup.org
Fri Aug 20 13:03:19 EEST 2021
Andreas Rheinhardt (12021-08-15):
> This currently happens by accident in a few filters that use
> ff_set_common_(samplerates|channel_layouts) like afir (if the response
> option is set) or agraphmonitor (due to the default code in
> avfiltergraph.c). So change those functions to make sure it does no
> longer happen.
I think it would be better to fix these filters to not rely on
ff_set_common_ functions. ff_set_common_formats is especially a problem
since it cannot check the media type as is. How many are problematic?
Or we should replace ff_set_common_formats() with explicit
ff_set_common_pix_fmts() and ff_set_common_sample_fmts().
sed -i s/ff_set_common_formats/ff_set_common_pix_fmts/ libavfilter/vf_*.c
sed -i s/ff_set_common_formats/ff_set_common_sample_fmts/ libavfilter/af_*.c
would probably do most of the job correctly and a few asserts would
catch the potential mistakes.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> 1. Contrary to ff_default_query_formats() the default code in
> avfiltergraph.c still uses ff_all_channel_layouts, not
> ff_all_channel_counts. I believe that this doesn't make sense
> and intend to change it after having inspected all filters for
> compatibility with the change. After it is changed,
> ff_default_query_formats() can be used directly.
It was a choice I made when we had frequent merges from the fork, where
unknown channel layouts were not supported.
If you or somebody else can make all filters support channel counts or
be explicit about not supporting them, then it is all the better.
Supporting unknown layouts is still something that needs testing. AFAIR,
we mostly do not have FATE tests for it because early parts of the code
insist on guessing a layout.
> 2. These defaults can be used to e.g. remove lots of
> ff_set_common_all_samplerates() and calls. I will also look into this.
> 3. I don't like the way it is decided whether the audio components
> get initialized in ff_default_query_formats() and in the default code
> in avfiltergraph.c: In many cases this will lead to an unnecessary
> allocation (if the query_formats function has already set everything)
> and furthermore I don't think one should only look at the first input
> or (lacking that) the first output. Using internal per-filter flags
> seems more reasonable for this.
The rationale is that if a filter is more complex than inputs and
outputs related and with the same format, then it must implement a
query_formats() callback and set the lists. Only the very simple filters
where the logic as implemented works can dispense with a complete
query_formats() callback.
I think it is a good principle. A system of flags would require a
similar amount of code, but be more complex and harder to maintain.
We could do with a little more consistency checks, though.
> 4. Just a quick question: If several links are set to individual
> lists that each contain exactly one element that coincides for all of
> these lists, then one could just as well use a shared list for all these
> links!? After all, in both cases the format negotiation will lead to the
> same result: The given format will be choosen for all these links.
> (E.g. vf_yadif_cuda.c can be simplified if the answer turns out to be
> yes (as I expect).)
I think you are right. Do you remember which other filters have this
issue? It is entirely possible the authors did not understand the
negotiation process in all its details.
>
> libavfilter/formats.c | 16 ++++++++++------
> libavfilter/formats.h | 6 +++---
> 2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index cc73c5abcb..9ceb32255b 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -622,14 +622,16 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
> FORMATS_CHANGEREF(oldref, newref);
> }
>
> -#define SET_COMMON_FORMATS(ctx, fmts, ref_fn, unref_fn) \
> +#define SET_COMMON_FORMATS(ctx, fmts, _type, ref_fn, unref_fn) \
I think mtype or media_type would be better than _type: identifiers
starting with an underscore are supposed to be allowed, but they might
cause issues with obscure implementations. More importantly, I find it
catches the eye and distract from what is going on.
> int i; \
> \
> if (!fmts) \
> return AVERROR(ENOMEM); \
> \
> for (i = 0; i < ctx->nb_inputs; i++) { \
> - if (ctx->inputs[i] && !ctx->inputs[i]->outcfg.fmts) { \
> + AVFilterLink *const link = ctx->inputs[i]; \
> + if (link && !link->outcfg.fmts && \
> + (_type == AVMEDIA_TYPE_UNKNOWN || link->type == _type)) { \
> int ret = ref_fn(fmts, &ctx->inputs[i]->outcfg.fmts); \
> if (ret < 0) { \
> return ret; \
> @@ -637,7 +639,9 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
> } \
> } \
> for (i = 0; i < ctx->nb_outputs; i++) { \
> - if (ctx->outputs[i] && !ctx->outputs[i]->incfg.fmts) { \
> + AVFilterLink *const link = ctx->outputs[i]; \
> + if (link && !link->incfg.fmts && \
> + (_type == AVMEDIA_TYPE_UNKNOWN || link->type == _type)) { \
> int ret = ref_fn(fmts, &ctx->outputs[i]->incfg.fmts); \
> if (ret < 0) { \
> return ret; \
> @@ -653,7 +657,7 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref)
> int ff_set_common_channel_layouts(AVFilterContext *ctx,
> AVFilterChannelLayouts *channel_layouts)
> {
> - SET_COMMON_FORMATS(ctx, channel_layouts,
> + SET_COMMON_FORMATS(ctx, channel_layouts, AVMEDIA_TYPE_AUDIO,
> ff_channel_layouts_ref, ff_channel_layouts_unref);
> }
>
> @@ -671,7 +675,7 @@ int ff_set_common_all_channel_counts(AVFilterContext *ctx)
> int ff_set_common_samplerates(AVFilterContext *ctx,
> AVFilterFormats *samplerates)
> {
> - SET_COMMON_FORMATS(ctx, samplerates,
> + SET_COMMON_FORMATS(ctx, samplerates, AVMEDIA_TYPE_AUDIO,
> ff_formats_ref, ff_formats_unref);
> }
>
> @@ -693,7 +697,7 @@ int ff_set_common_all_samplerates(AVFilterContext *ctx)
> */
> int ff_set_common_formats(AVFilterContext *ctx, AVFilterFormats *formats)
> {
> - SET_COMMON_FORMATS(ctx, formats,
> + SET_COMMON_FORMATS(ctx, formats, AVMEDIA_TYPE_UNKNOWN,
> ff_formats_ref, ff_formats_unref);
> }
>
> diff --git a/libavfilter/formats.h b/libavfilter/formats.h
> index ed513c265a..bef6557578 100644
> --- a/libavfilter/formats.h
> +++ b/libavfilter/formats.h
> @@ -144,9 +144,9 @@ av_warn_unused_result
> AVFilterChannelLayouts *ff_make_format64_list(const int64_t *fmts);
>
> /**
> - * A helper for query_formats() which sets all links to the same list of channel
> - * layouts/sample rates. If there are no links hooked to this filter, the list
> - * is freed.
> + * Helpers for query_formats() which set all free audio links to the same list
> + * of channel layouts/sample rates. If there are no links hooked to this list,
> + * the list is freed.
> */
> av_warn_unused_result
> int ff_set_common_channel_layouts(AVFilterContext *ctx,
Code looks good to me.
I do not maintain the other files in this patchset, but you can probably
go ahead and push most of it.
Thanks.
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210820/cb1f776e/attachment.sig>
More information about the ffmpeg-devel
mailing list