[FFmpeg-devel] [PATCH] libavfilter: constify filter list

[Mark Thompson]

On 31/01/18 10:37, Muhammad Faiz wrote:
> On Wed, Jan 31, 2018 at 5:26 AM, Mark Thompson <sw at jkqxz.net> wrote:
>> On 30/01/18 18:06, Muhammad Faiz wrote:
>>> On Tue, Jan 30, 2018 at 9:09 PM, Mark Thompson <sw at jkqxz.net> wrote:
>>>> On 30/01/18 07:24, Muhammad Faiz wrote:
>>>>> Move REGISTER_FILTER to FILTER_TABLE in configure.
>>>>> Auto generate filter extern and filter table.
>>>>> Sort filter table, use bsearch on avfilter_get_by_name.
>>>>> Define next pointer at filter extern, no need to initialize
>>>>> next pointer at run time, so AVFilter can be set to const.
>>>>> Make avfilter_register always return error.
>>>>> Target checkasm now depends on EXTRALIBS-avformat.
>>>>>
>>>>> Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
>>>>> ---
>>>>
>>>> I like the idea of this, but I'm not sure about some of the implementation details.
>>>>
>>>> Have you considered dropping the "next" links entirely and having just the array of pointers instead?  I feel like they don't really add anything useful once we are in this form, and result in more boilerplate on every filter and some tricky generation code.
>>>>
>>>> avfilter_next() would be a bit slower (since it would have to search the array, and therefore have runtime linear in the number of filters), but avfilter_get_by_name() is a lot faster because of the binary search (and is the only common use of it) so I don't think that complaint would be a strong one.
>>>
>>> Making avfilter_next() slower (even if it is rarely used) isn't good, I think.
>>
>> I think the slowdown is irrelevant if it is rarely used.
>>
>> On the other side, you get rid of a field in AVFilter and avoid having to put some pointless boilerplate in a lot of places.
> 
> The other solution is to initialize next pointer on
> avfilter_register_all() as before,  add new iterate API, and deprecate
> both avfilter_register_all() and avfilter_next().

I guess having thought about this further my problem is that you appear to be writing a lot of new infrastructure for creating and updating the next links (with special generation code in configure and extra boilerplate on every filter) when the feature does not have any clear value.  Once other functions are properly updated the only place where the next link is used is in avfilter_next(), which is only slowed down a little bit and would never be called in performance-sensitive code anyway.  A new iterate API would be welcome but is mostly orthogonal - you aren't going to call that in performance-sensitive code either.

So, can we just drop the next links completely?

- Mark


(Everything else trimmed.  I'm ok with the huge lists in configure if other people agree to it, I just found it rather inelegant compared to the current per-library information in separate files.)