On 3 January 2018 at 15:25, wm4 <nfx...@googlemail.com> wrote:
> On Wed, 3 Jan 2018 00:42:36 +0000
> Josh de Kock <j...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
