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

[wm4]

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.

> +static void av_parser_init_next(void)
> +{
> +    AVCodecParser *prev = NULL, *p;
> +    int i = 0;
> +    while ((p = (AVCodecParser*)parser_list[i++])) {

(Personally I'd prefer less than 2 side effects in conditional
expressions, but maybe it doesn't matter.)

> +        if (prev)
> +            prev->next = p;
> +        prev = p;
> +    }

> +    if (prev)
> +        prev->next = NULL;

Probably redundant assignment again.

Still missing: APIchanges entry.