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

[James Almer]

On 12/24/2017 8:43 AM, Michael Niedermayer 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.
> 
> 
>>
>> - 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.

Every project that requires some custom addition to what ffmpeg offers
will and should implement them into their own public fork of ffmpeg as
lgpl expects them to, and right now that includes custom modules like
codecs, muxers and such. Said changes can then be upstreamed if
needed/useful.

The API does not allow them to register their own codecs. We could right
now move all the "private" AVCodec fields into an internal opaque struct
while keeping the codec register API untouched and it wouldn't be an API
break of any kind, yet it would prevent people from abusing it and
registering their own Frankencodecs.
So yes, it's literally a side effect of us putting internal fields in
public headers, an awful practice that no one else does, probably
because of reasons like the one we're discussing.

Furthermore, we did what this patch is doing with hwaccels a month or so
ago, and it wasn't an issue. Same with bsfs like a year ago. This is
simply the next step.

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

"People in the past misused the API. What if they still do?" is not a
good reason to prevent development and improvements to our codebase.
People will always misuse the API when they can, and it should never be
a reason to condition development. Just look at how things were with
mplayer for the longest time.

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

No, it's not easier to maintain than in a fork. They are still forced to
ship their own libavcodec instead of using the system one since they are
using private, non fixed ABI fields to register their codecs, they still
need to adapt to any non offset related changes to said callback fields,
etc.

If we eventually add the possibility to register user codecs, then it
should be one similar to AVIO, where the callback functions are provided
by the user at the moment of allocating an AVCodec before it can be
registered. But right now, registering user codecs is so fragile that
it's a matter of making the private AVCodec fields truly private for it
to stop being possible.

> 
>>
>>>> +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.
> 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
> 
> PS: merry Xmas!

Merry Christmas :)

> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>