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.rheinha...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
