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

Tue Feb 20 20:20:43 EET 2018

On Tue, Feb 20, 2018 at 02:02:14PM -0300, James Almer wrote:
> 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. 

You certainly have a point here
though most strictly looking at it, the fuzzer is internal in our tree and
thus it doesnt really violate the API any more than other internal
components do.

> This therefore can't be
> considered a change in behavior compared to the current/old registration
> based API.

Well, the fuzzer binaries are like several times bigger than before that is a
difference caused by the patches. Not by the puiblic API, no
but iam not sure this way of looking at it is really helpfull to any side of
this arguemnt

> 
> 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.

ok, it seems i misunderstood you

> 
> > 
> > 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.

Yes but this was before some of the disadvantages of that where fully understood
(at least by me, as i was a bit caught by surprise that the fuzzer binaries grew
 though its very logic in retrospect)

Are the people working on this still against runtime registration?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180220/6f1e0b62/attachment.sig>