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

[Rostislav Pehlivanov]

On 3 January 2018 at 15:25, wm4 <nfxjfg at googlemail.com> wrote:

> On Wed, 3 Jan 2018 00:42:36 +0000
> Josh de Kock <josh at itanimul.li> wrote:
>
> > Also replace linked list with an array.
> > ---
> >  configure              |   12 +-
> >  doc/APIchanges         |    4 +
> >  libavcodec/allcodecs.c | 1473 ++++++++++++++++++++++++++++--
> ------------------
> >  libavcodec/avcodec.h   |   31 +
> >  libavcodec/parser.c    |   87 ++-
> >  libavcodec/utils.c     |  105 ----
> >  libavcodec/version.h   |    3 +
> >  7 files changed, 974 insertions(+), 741 deletions(-)
> >
>
> > +#if CONFIG_ME_CMP
> > +#include "me_cmp.h"
> > +#endif
> > +
> > +pthread_once_t av_codec_static_init = PTHREAD_ONCE_INIT;
> > +static void av_codec_init_static(void)
> > +{
> > +#if CONFIG_ME_CMP
> > +    ff_me_cmp_init_static();
> > +#endif
>
> (This should probably be moved away, since it doesn't really have to do
> with any of this - it's just a global init in the wrong place. But I
> won't insist that this gets cleaned up now. Someone else can do it
> later.)
>
> > +    for (int i = 0; codec_list[i]; i++) {
> > +        if (codec_list[i]->init_static_data)
> > +            codec_list[i]->init_static_data((AVCodec*)codec_list[i]);
> >      }
> > +}
> > +
> > +const AVCodec *av_codec_iterate(void **opaque)
> > +{
> > +    uintptr_t i = (uintptr_t)*opaque;
> > +    const AVCodec *c = codec_list[i];
> > +
> > +    pthread_once(&av_codec_static_init, av_codec_init_static);
>
> One issue: you need to use ff_thread_once() everywhere (it uses a
> different initializer type too). The reason is because if it's built
> without threads, pthread_once() will not be defined.
>
> Same for all other uses of this.
>
> >
> > -#define REGISTER_ENCDEC(X, x) REGISTER_ENCODER(X, x);
> REGISTER_DECODER(X, x)
> > +    if (c)
> > +        *opaque = (void*)(i + 1);
> >
> > -#define REGISTER_PARSER(X, x)
>  \
> > -    {
>  \
> > -        extern AVCodecParser ff_##x##_parser;
>  \
> > -        if (CONFIG_##X##_PARSER)
> \
> > -            av_register_codec_parser(&ff_##x##_parser);
>  \
> > +    return c;
> > +}
> > +
> > +#if FF_API_NEXT
> > +pthread_once_t av_codec_next_init = PTHREAD_ONCE_INIT;
> > +
> > +static void av_codec_init_next(void)
> > +{
> > +    AVCodec *prev = NULL, *p;
> > +    void *i = 0;
> > +    while ((p = (AVCodec*)av_codec_iterate(&i))) {
> > +        if (prev)
> > +            prev->next = p;
> > +        prev = p;
> >      }
>
> > +    if (prev)
> > +        prev->next = NULL;
>
> What's this for? Isn't it already initialized to NULL?
>
> > +}
> > +
> >
> > -static void register_all(void)
> > +
> > +av_cold void avcodec_register(AVCodec *codec)
> >  {
> > -    /* video codecs */
> > -    REGISTER_ENCODER(A64MULTI,          a64multi);
>
> > +    pthread_once(&av_codec_next_init, av_codec_init_next);
> > +}
>
> I don't see much of a point in this call. Could be dropped, so the body
> is empty.
>
> > +AVCodec *av_codec_next(const AVCodec *c)
> > +{
> > +    pthread_once(&av_codec_next_init, av_codec_init_next);
> > +
> > +    if (c)
> > +        return c->next;
> > +    else
> > +        return (AVCodec*)codec_list[0];
> >  }
> >
> >  void avcodec_register_all(void)
> >  {
> > -    static AVOnce control = AV_ONCE_INIT;
> > +    pthread_once(&av_codec_next_init, av_codec_init_next);
> > +}
>
> So that means if you use the legacy API, you still need to call
> the register function. Strange choice, but I'm fine with it.
>
> > +#endif
> > +
> > +static enum AVCodecID remap_deprecated_codec_id(enum AVCodecID id)
> > +{
> > +    switch(id){
> > +        //This is for future deprecatec codec ids, its empty since
> > +        //last major bump but will fill up again over time, please
> don't remove it
> > +        default                                         : return id;
> > +    }
> > +}
> > +
> > +static AVCodec *find_codec(enum AVCodecID id, int (*x)(const AVCodec *))
> > +{
> > +    const AVCodec *p, *experimental = NULL;
> > +    void *i = 0;
> > +
> > +    id = remap_deprecated_codec_id(id);
> > +
> > +    while ((p = av_codec_iterate(&i))) {
> > +        if (!x(p))
> > +            continue;
> > +        if (p->id == id) {
> > +            if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL &&
> !experimental) {
> > +                experimental = p;
> > +            } else
> > +                return (AVCodec*)p;
> > +        }
> > +    }
> >
> > -    ff_thread_once(&control, register_all);
> > +    return (AVCodec*)experimental;
> > +}
> > +
> > +AVCodec *avcodec_find_encoder(enum AVCodecID id)
> > +{
> > +    return find_codec(id, av_codec_is_encoder);
> > +}
> > +
> > +AVCodec *avcodec_find_decoder(enum AVCodecID id)
> > +{
> > +    return find_codec(id, av_codec_is_decoder);
> > +}
> > +
> > +static AVCodec *find_codec_by_name(const char *name, int (*x)(const
> AVCodec *))
> > +{
> > +    void *i = 0;
> > +    const AVCodec *p;
> > +
> > +    if (!name)
> > +        return NULL;
> > +
> > +    while ((p = av_codec_iterate(&i))) {
> > +        if (!x(p))
> > +            continue;
> > +        if (strcmp(name, p->name) == 0)
> > +            return (AVCodec*)p;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +AVCodec *avcodec_find_encoder_by_name(const char *name)
> > +{
> > +    return find_codec_by_name(name, av_codec_is_encoder);
> > +}
> > +
> > +AVCodec *avcodec_find_decoder_by_name(const char *name)
> > +{
> > +    return find_codec_by_name(name, av_codec_is_decoder);
> >  }
>
> Personally I'd argue a bool parameter or so to select encoders vs.
> decoders would be less roudnabout (like it was before), but I have no
> strong opinion. Could be considered an improvement as well.
>
>
I think separate functions would be better in this case.