[FFmpeg-devel,09/10] avfilter/formats: Don't set samplerate or channel count on video links

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.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.
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.
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).)

 libavfilter/formats.c | 16 ++++++++++------
 libavfilter/formats.h |  6 +++---
 2 files changed, 13 insertions(+), 9 deletions(-)

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@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:
> 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?
> 

AFAIK no filter that is not audio/video-only uses ff_set_common_formats
directly. But lots use it indirectly, because a call to
ff_set_common_formats() happens after every successful call to a
query_formats function. But as I see it, all such filters already set
all the formats, so said ff_set_common_formats() does nothing.

But I don't agree with you that ff_set_common_samplerates/channel_counts
should be disallowed: Now that they only do something for for audio
links (and are documented to do so) the existence of video links should
not hinder us from simplifying query_formats functions by using them.
E.g. it seems that all avf_* filters except concat can use
ff_set_common_all_samplerates(). The only reason I haven't already done
so is because it would lead to merge conflicts with your patch.

> 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.
> 

I'll implement this as soon as I have found one instance where we set
pix fmts for audio or sample fmts for video.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.

Is there a way I can force forgetting the channel 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.
> 

I don't agree that flags are more complex and harder to maintain or that
they would lead to a similar amount of code: Even complex filters often
have quite a lot of boilerplate code (the most obvious example is
ff_set_all_samplerates) which could be removed if one could rely on it
being called generically after the query_formats callback. Furthermore,
with flags one can extend the logic as-is to make the majority of video
query_formats callbacks superfluous: The majority of said callbacks
simply call ff_set_common_formats_from_list() and this can be replaced
by putting a pointer to the list in the AVFilter instead of a pointer to
query_formats and these cases can be distinguished by a flag.

> 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.
> 

vf_palettegen.c and the VAAPI filters (ff_vaapi_vpp_query_formats does
this). Probably even more than that.

>>
>>  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.
> 

Changed to media_type (I didn't use it to avoid an overlong line) and
applied it.

- Andreas

Andreas Rheinhardt (12021-08-21):
> AFAIK no filter that is not audio/video-only uses ff_set_common_formats
> directly. But lots use it indirectly, because a call to
> ff_set_common_formats() happens after every successful call to a
> query_formats function. But as I see it, all such filters already set
> all the formats, so said ff_set_common_formats() does nothing.
> 
> But I don't agree with you that ff_set_common_samplerates/channel_counts
> should be disallowed: Now that they only do something for for audio
> links (and are documented to do so) the existence of video links should
> not hinder us from simplifying query_formats functions by using them.

Fair enough.

> E.g. it seems that all avf_* filters except concat can use
> ff_set_common_all_samplerates(). The only reason I haven't already done
> so is because it would lead to merge conflicts with your patch.

Thank you. I will try to rush applying it.

> Is there a way I can force forgetting the channel layout?

I am afraid not. This is something that would need investigating and
fixing.

> I don't agree that flags are more complex and harder to maintain or that
> they would lead to a similar amount of code: Even complex filters often
> have quite a lot of boilerplate code (the most obvious example is
> ff_set_all_samplerates) which could be removed if one could rely on it
> being called generically after the query_formats callback. Furthermore,
> with flags one can extend the logic as-is to make the majority of video
> query_formats callbacks superfluous: The majority of said callbacks
> simply call ff_set_common_formats_from_list() and this can be replaced
> by putting a pointer to the list in the AVFilter instead of a pointer to
> query_formats and these cases can be distinguished by a flag.

You are neglecting the complexity of flags when it comes to
understanding the code. Flags constitute a state engine, a new language
to express the constraints. FFmpeg is written in C, we are fluent in C.
If somebody else than you were to use the system, they would need to
learn the subtleties of the flags system to use it correctly, and the
same goes for whoever would review their patches.

Furthermore, I posit that for review and maintenance, it is better to
have five contiguous lines of code than to have only three lines of code
if they are apart but interact.

In this instance, helper functions and macros that can reduce the
boilerplate code seem a better solution.

It could look like this:

static int query_formats(AVFilterContext *ctx)
{
    same_audio_formats(fmt_list(AV_SAMPLE_FMT_DBLP),
                       all_layouts,
		       all_framerates);
}

This is compact and readable, and does not depend on complex behavior by
the framework.

When I reimplemented the scheduling from the recursive implementation to
using the activate callback, the thing that was hardest and required the
most work was to reproduce every tiny quirk of the framework because
some filter used them. It would have been much more convenient if they
had been explicit like that.

Regards,