[FFmpeg-devel] [PATCH 0/3] Finish new iteration APIs

[James Almer]

On 2/20/2018 3:24 AM, wm4 wrote:
> On Mon, 19 Feb 2018 23:30:24 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 2/19/2018 11:16 PM, Michael Niedermayer wrote:
>>> On Mon, Feb 19, 2018 at 09:57:35PM +0100, Hendrik Leppkes wrote:  
>>>> On Mon, Feb 19, 2018 at 8:30 PM, Michael Niedermayer
>>>> <michael at niedermayer.cc> wrote:  
>>>>> On Sun, Feb 18, 2018 at 05:58:16PM +0000, Josh de Kock wrote:  
>>>>>> This should be the last of the major API changes. I'm not entirely
>>>>>> sure if I missed anything.  
>>>>>
>>>>> Moving from a register based system where a user app can register
>>>>> any subset to a system which registers all via an array will
>>>>> increase the size of statically linked binaries for users only
>>>>> using a subset.
>>>>>  
>>>>
>>>> User apps did not have the ability to register a subset. How would
>>>> they do that? They can't access the internals (ie. the ff_ references
>>>> to those components)  
>>>
>>> I think you are mistaken here.
>>>
>>> What you are thinking of here, i think is, that ff_* symbols are not
>>> accessable to user apps. This is true with shared libs but the issue
>>> above is primarely an issue with static linking. There the ff_* symbols
>>> are available.
>>>
>>> But much more important, we are designing a new API here, it doesnt matter
>>> all that much what was possible, what matters is that it IS possible
>>> and its IMHO not a obscure use case to want to only "register" parts that are
>>> actually needed. Every security concious application that deals with
>>> some standarized input from the net, like a browser, would IMHO want to
>>> limit the parts that are enabled by as many and as hard ways as possible.
>>>
>>>   
>>>> That was only something some internal tools used, and you can probably
>>>> find different options for dealing with those.  
>>>
>>> I can imagine some ways to hack around it, for the fuzzer yes, but a
>>> clean way though proper public API (no ff_*, no #ifdefs around the array)
>>> would seem better
>>>
>>> So, yeah, i would prefer if a new API would allow registering subsets.
>>>
>>> Without this and if the fuzzer runs out of diskspace someone will probably
>>> need to hack around the new API so the arrays with all the pointers to
>>> every part arent linked in. I may be missing some solution but this
>>> sounds like a bunch of ugly code ...  
>>
>> Afaik, the objective of this new API was to make the modules const and
>> not mutable during init/registration by the requirement of setting the
>> *next pointer.
>> Admittedly, by keeping the init_static feature that can also set fields
>> like pix_fmt or change reported capabilities, the benefits from this new
>> API are more or less nullified.
> 
> That doesn't even affect filters. The pix_fmt thing affects only less
> than 5 codecs.
> 
>> So i agree with you that, seeing the drawbacks this new API introduced
>> without having actually achieved its objective, a different, better one
>> that allows "registration", the modules to be const while setting at
>> least some subset of capabilities based on the runtime environment
>> (things like enabled pix_fmts, codec capabilities and such) should be
>> written instead.
> 
> It has fully achieved its objectives. There's no more visible global
> mutable state, and the actual global mutable state has been reduced to
> less than 5 codec entries.

It's true that after the deprecation period they will effectively be the
only ones still mutable, but shouldn't the objective be to cover them all?

> 
> Why are we discussing this _again_?

Because a drawback has been found, namely that link time optimization
using static libraries can't remove "non registered" modules anymore,
and now depends fully on what's enabled at configure time.
Ideally, a better registration based API that follows what i described
above should be designed with care.

> 
>> Whatever is done, in any case, should be decided fast. The current new
>> API is in the tree and should be removed without delay if we decide to
>> not use it in the end, even if a proper replacement is not written in
>> the short term.
> 
> What needs to be done is testing and applying these patches.

What makes me uneasy is that if this API remains is place, introducing a
new better one would be annoying for downstream users if it's added too
close to the end of the old API's deprecation period, be it before or
after. Having to migrate a second time in a short time frame is undesirable.

In any case, just my two cents. Don't take this as a blocker for this
set, but as a request to consider better alternatives in the short or
long term.