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

[Nicolas George]

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,

-- 
  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/20210821/8988644b/attachment.sig>