[FFmpeg-devel] [PATCH] lavc: add new API for iterating codecs and codec parsers

[wm4]

On Sun, 24 Dec 2017 12:43:27 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sun, Dec 24, 2017 at 02:31:16AM +0100, wm4 wrote:
> > On Sun, 24 Dec 2017 02:06:40 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > If you and others agree we can also easily maintain support for user apps
> > > to register codecs. The only thing needed is to make the array bigger and
> > > add codecs which arent ours in there as long as there is space.
> > > doing this would be very easy, a atomic integer pointing to the begin of
> > > the free space which is atomically increased for each register is all that
> > > is needed  
> > 
> > Not sure how often I repeated this (specifically for you), but:  
> 
> why are you so upset ? We have different oppinions here or we misunderstand
> each other.

Not necessarily upset, but this is tiring. Every time you bring this
up, I have to repeat the arguments I made, and next time you seem to
have forgotten about it. And then it always degenerates in a flame war.
I don't need this.

On the other hand, you seem to be upset about the removal of codec
registering. I don't know why. As I've said, it didn't work anyway,
unless you've used internal API/ABI. You seem to be raising a bit of
a stink about it.

In summary:

- before this commit: you could not register user codecs, unless you've
  used unstable internal API and used ABI unstable internal fields,
  effectively binding to one FFmpeg version
- after this commit: you can not register user codecs, even if you're
  willing to violate the API

Not a big loss I think. Also you could still pass customs AVCodec to
the codec context creation.

> > 
> > - users can't provide AVCodec, because it would require accessing
> >   internal API and private ABI unstable fields  
> 
> That is expected without there being an effort to design a public API
> for external codecs.
> 
> Point being, this doesnt matter.
> One can still maintain an external codec register it and use it.
> It would need to be rebuild for each libavcodec version and would
> occasionally need to be changed like any internal codec.
> But it would not be harder than maintaining a codec inside ffmpeg.

You're just as well off by patching the libavcodec to add your
non-upstreamed codec. (Before this patch, you obviously had to have
control over the libavcodec used, and now you can use your control over
it to patch it in.)

> Especially if one targets a release like 3.4.x it would be close to 0
> work to have a external codec that is registered compared to having it
> internal
> 
> 
> > - if we provide the ability to add user codecs (which I'm not
> >   fundamentally against), then it should not modify global state.
> >   Having named user codecs in a process wide list would definitely lead
> >   to name clashes or other conflicts  
> 
> I dont see how you could do this without modifying global state.
> You would need some kind of context for all codec "lookup" functions
> 
> Such context could be added and iam not against this at all, but i feel

Yes. And this would require new API. Actually, I think you would just
not register the codec at all, but pass it directly to AVCodecContext
initialization (and for other components, anything that actually needs
to access such lists).

In fact you can still do this after this patch. avcodec_alloc_context3()
takes a AVCodec pointer. You can pass a custom struct if you're willing
to violate the public API.

> this is fixing a problem noone has ever had. registering codecs was
> supported since forever and ive never heared of a conflict like you describe

I've never heard of any API user using custom registration. When I
inquired about this in my previous mail, you couldn't name verifiable
examples either.

I know a bunch of programs that broke because libavformat removed
custom protocol registration (and forced you to use a custom AVIO
context instead), but apparently that hasn't been a problem for FFmpeg.

> 
> > - also, we would have to provide stable/public API for implementing
> >   codecs in the first place (all AVCodec callback fields are private),
> >   and we're nowhere near the state of adding it  
> 
> I would love to see such a API and i would certainly contribute but

> as explained above, we dont have it and we dont really need it for
> registering user codec to be usefull in practice IMO

Are you seriously arguing for API users to access internal API?

And I bet some time later you could argue for avoiding internal API
changes because some API users rely on it... This is not how it works.
You're just setting us up for a maintenance nightmare. The whole point
of separation FFmpeg into public and private API is to set a boundary
so we can do anything at all without immediately breaking API users.

> 
> > - dropping avcodec_register() is definitely not the worst blocker for
> >   user codecs - it's very, very far from it, because once we have
> >   fixed the things above, we can just add a new public API for
> >   registering (which would have to have a different function signature
> >   to avoid global mutable lists). So I don't know why you complain.  
> 
> Like elaborated above, it is the main problem.
> Currently external codecs are possible for all i know. And i have
> seen it used a few years ago, so it did work at some point in real
> applications.
> 
> 
> > - these points are technical, not ideological
> > 
> > Can you point out any user application which registers its own codecs
> > (and this violates the API)?  
> 
> I do not remember which application it was but i did a few years ago review
> all user apps using libavcodec / libavformat for things we could merge.
> I do remember seeing some that added codecs.
> It was some code that was very specific to the app or platform IIRC

These sound like they're fine with patching ffmpeg, or using the
method I described above.

Besides, "someone might have done something that violated the API a
decade ago" isn't a very good argument to consider and respect
hypothetical invalid API use now. If the code you mentioned still
exists, we've probably broken it a dozen times by now.

> There are more usecases. For example codec optimizations for niche
> codecs where rejected on ffmpeg-devel.
> I find it unfriendly from us if we reject improvments because they
> arent in a area most of us care and then also drop the possibility
> for externally maintaining and registering code without doing a
> full fork (which is much more work to maintain)

What? We accept tons of obscure features and optimizations.

The only thing I remember that got consistently rejected were the HEVC
optimizations - not because we didn't want them, but because they used
intrinsics in x86 (instead of external asm), which we reject on
principle. We (apparently) had bad experiences with them in the past,
compiler support tends to be flaky, and performance of the generated
code can depend much on used compiler and is not very predictable. You
know very well that we have a policy to reject intrinsics and that we
follow it for good reasons.

I'm not sure why you feel the need to distort the facts that much just
to make some sort of passive aggressive sounding point about general
dev/project practices.

> >   
> > > > +AVCodec *av_codec_next(const AVCodec *c)
> > > > +{
> > > > +    pthread_once(&av_codec_next_init, av_codec_init_next);
> > > > +
> > > > +    if (c)
> > > > +        return c->next;    
> > > 
> > > AVCodec->next should be removed as it makes the structs non constant  
> > 
> > That has to happen after a deprecation phase, unless you have an idea
> > how to make av_codec_next() not O(n^2) without this.  
> 
> That can be done and there are many ways. Not saying we want to do it
> first possibility, one can add a int "constant" to each AVCodec.
> configure would have to choose these or there would be holes in the array.

That sounds complicated, how would configure modify each AVCodec struct
(which are spread into hundreds of source files)?

> a hashtable with a mutex is an option too but that seems not worth the
> complexity / work
> the 3rd obvious option is to make "next" a pointer to a int which is
> initialized in each codec to some static int. Which can then be set
> to the array index, the main struct can then remain in read only memory
> iam pretty sure there are many more options

Not sure how these are better than just initializing the next pointer
for now, until deprecation removes it.

> PS: merry Xmas!

Same.