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

[James Almer]

On 2/20/2018 1:30 PM, Michael Niedermayer wrote:
> On Tue, Feb 20, 2018 at 10:17:02AM -0300, James Almer wrote:
>> On 2/20/2018 9:21 AM, wm4 wrote:
>>> On Tue, 20 Feb 2018 08:47:51 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>
>>>>> 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?
>>>
>>> That would be nice. But this has been discussed before. No kind of
>>> different registration API could fix this issue either, unless we start
>>> mallocing AVCodec structs and require the user to free them, or
>>> something equally absurd. The proper solution for this specific issue
>>> would be making a new API that somehow replaces AVCodec.pix_fmts.
>>>
>>>>>
>>>>> 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.
>>>
>>> Oh yeah, bikeshed attack by Michael. As it was said a few thousand times
>>> already, public API users could NOT use the registration stuff to
>>> register only specific components at runtime. So they have to use the
>>> register_all variants, which pull in _all_ components even with static
>>> linking.
>>
>> Oh, i assumed it was possible to use avcodec_register() on a manual
>> basis in a proper API usage scenario, but i see now that's not the case.
> 
> of course its possible to use avcodec_register() on a manual basis in a 
> proper API usage scenario, why would it not be ?
> 
> why do you think this function exists or was written ?
> 
> is it the ff_* symbols you are thinking of ?

Yes. You can't register individual AVCodecs using avcodec_register()
without those symbols, and in a normal API usage scenario, be it with
the old or the new iterate one, they are not available.

> This is a problem for an existing API it is not a problem for a new API as
> we can change the symbols if they are intended to be used for individual
> component registration.> The whole discussion is about designing a new API. So any limitation of
> an existing API can be changed.

Hence me also being a proponent of a different, better registration
based API.
I think you misunderstood what i tried to say in the email you replied
to. You reported a drawback of this new API by giving the example of the
fuzzer not being able to discard unused codec symbols since all are
unconditionally referenced now. I assumed that the fuzzer was using the
API properly all this time, but turns out it's not, digging the
internals instead to register single codecs. This therefore can't be
considered a change in behavior compared to the current/old registration
based API.

Which brings me to the next part below.

> 
> There also should be no need to call register_all in the existing API,
> and there cannot be such a problem in a new design because it would be
> a new design you wouldnt design it to "not work".
> 
> 
>>
>> Nevermind then, my argument was focused on preventing losing some link
>> time optimization for static libraries assuming proper API usage.
>>
>>>
>>> And that can't be made with dynamic linking either. If you statically
>>> link anyway, you probably control everything, and you can configure this
>>> stuff at compile time.
>>>
>>> Then I guess there's this very special case where a fuzzer statically
>>> links to libavcodec, and registers only a few components (technically
>>> violating the API and by guessing the component symbol name), and
>>> without calling the register_all functions. This would make the
>>> resulting executable much smaller, which is apparently important
>>> because Google (who runs the fuzzers for their oss-fuzz project) is too
>>> poor (?) to host all the statically linked fuzzers, _or_ the whitelist
>>> stuff is not enough to trick the fuzzer into not trying to fuzz the
>>> wrong code. In addition, it's apparently infeasible to just build
>>> every fuzzer with a separate libavcodec. (Not sure about the details, it
>>> was something like this.)
>>>
>>> Those requirements are really quite nebulous. I don't know why we even
>>> should care - surely whoever does this will find a solution that works
>>> for them. For example they could apply a small patch that makes the
>>> codec_list[] symbol non-static non-const, which would allow them to
>>> overwrite it (i.e. deleting unneeded entries). It's a really simple
>>> solution that took me 5 minutes to come up with.
>>
>> Something like this is probably the best solution for the fuzzer issue, yes.
> 
> This basically says one should fork ffmpeg if one has problems with the new API
> 
> Thats a very chilling response to be honest in a discussion about that APIs
> design. More so as this is at a time we still can change the API.

Again, you misunderstood me. Currently the fuzzer tool is abusing the
API by digging its internal bits in order to register single codecs. wm4
suggestion, which i adhered to, was to change the way the tool digs the
internals to recover the expected unused symbol stripping behavior.

Nothing in my previous email suggested that I'm against a better
registration based API. I'm still in favor of one since it's a lot more
powerful and versatile than this fixed, configure time constant codec
list recently introduced. I simply conceded that the argument about the
fuzzer not stripping symbols with the iterate API was not a valid one to
discredit the latter. It is in fact a good reason/example of why a
proper registration based API should be introduced instead.

> 
> and anyone who wants to only register a subset of components (due to security)
> has to either link to a seperate binary (which is  disallowed in some 
> major distributions which mandate shared libs without exception IIRC so that
> sounds great but simple isnt possible)
> or just not use FFmpeg or fork it or use a fork or just forget about only
> registering a subset.
> 
> IMO, an API that allows registering subsets of components would be wiser
> for FFmpeg.

I agree. I said as much in a previous email. And that we should decide
if we want to keep this iterate API or not, and soon.
At no point i changed my mind about supporting the idea of a better
registration based API.

> 
> Of course we can go with a API that doesnt allow subset registeration but
> then later we need to add an API to seperatly register user componentgs (plugins),
> so 2 APIs and our fuzzer needs to patch that API, and browsers which may only 
> want to register a subset of codecs & formats are screwed.
> 
> Seriously, why should we do it this way ?

We don't have to. But the two people willing to write a new API didn't
want to design it to allow runtime registration.

> An API that allows registering subsets is not very complex
> 
> Thanks
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>