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

[Andreas Rheinhardt]

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

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