[FFmpeg-devel] [PATCH 1/2] avcodec: add avcodec_register_one() to register one codec/parser/bsf/hwaccel by its char* name

[wm4]

On Tue, 30 Sep 2014 02:02:49 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavcodec/allcodecs.c |   51 +++++++++++++++++++++++++++++++++---------------
>  libavcodec/avcodec.h   |    5 +++++
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 7650543..019d5ea 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -29,49 +29,55 @@
>  #include "version.h"
>  
>  #define REGISTER_HWACCEL(X, x)                                          \
> -    {                                                                   \
> +    if (!name || !strcmp(name, AV_STRINGIFY(x##_hwaccel))) {            \
>          extern AVHWAccel ff_##x##_hwaccel;                              \
> -        if (CONFIG_##X##_HWACCEL)                                       \
> +        if (CONFIG_##X##_HWACCEL) {                                     \
>              av_register_hwaccel(&ff_##x##_hwaccel);                     \
> +            found++;                                                    \
> +        }                                                               \
>      }
>  
>  #define REGISTER_ENCODER(X, x)                                          \
> -    {                                                                   \
> +    if (!name || !strcmp(name, AV_STRINGIFY(x##_encoder))) {            \
>          extern AVCodec ff_##x##_encoder;                                \
> -        if (CONFIG_##X##_ENCODER)                                       \
> +        if (CONFIG_##X##_ENCODER) {                                     \
>              avcodec_register(&ff_##x##_encoder);                        \
> +            found++;                                                    \
> +        }                                                               \
>      }
>  
>  #define REGISTER_DECODER(X, x)                                          \
> -    {                                                                   \
> +    if (!name || !strcmp(name, AV_STRINGIFY(x##_decoder))) {            \
>          extern AVCodec ff_##x##_decoder;                                \
> -        if (CONFIG_##X##_DECODER)                                       \
> +        if (CONFIG_##X##_DECODER) {                                     \
>              avcodec_register(&ff_##x##_decoder);                        \
> +            found++;                                                    \
> +        }                                                               \
>      }
>  
>  #define REGISTER_ENCDEC(X, x) REGISTER_ENCODER(X, x); REGISTER_DECODER(X, x)
>  
>  #define REGISTER_PARSER(X, x)                                           \
> -    {                                                                   \
> +    if (!name || !strcmp(name, AV_STRINGIFY(x##_parser))) {             \
>          extern AVCodecParser ff_##x##_parser;                           \
> -        if (CONFIG_##X##_PARSER)                                        \
> +        if (CONFIG_##X##_PARSER) {                                      \
>              av_register_codec_parser(&ff_##x##_parser);                 \
> +            found++;                                                    \
> +        }                                                               \
>      }
>  
>  #define REGISTER_BSF(X, x)                                              \
> -    {                                                                   \
> +    if (!name || !strcmp(name, AV_STRINGIFY(x##_bsf))) {                \
>          extern AVBitStreamFilter ff_##x##_bsf;                          \
> -        if (CONFIG_##X##_BSF)                                           \
> +        if (CONFIG_##X##_BSF) {                                         \
>              av_register_bitstream_filter(&ff_##x##_bsf);                \
> +            found++;                                                    \
> +        }                                                               \
>      }
>  
> -void avcodec_register_all(void)
> +int avcodec_register_one(const char *name)
>  {
> -    static int initialized;
> -
> -    if (initialized)
> -        return;
> -    initialized = 1;
> +    int found = 0;
>  
>      /* hardware accelerators */
>      REGISTER_HWACCEL(H263_VAAPI,        h263_vaapi);
> @@ -588,4 +594,17 @@ void avcodec_register_all(void)
>      REGISTER_BSF(NOISE,                 noise);
>      REGISTER_BSF(REMOVE_EXTRADATA,      remove_extradata);
>      REGISTER_BSF(TEXT2MOVSUB,           text2movsub);
> +
> +    return found;
>  }
> +
> +void avcodec_register_all(void)
> +{
> +    static int initialized;
> +
> +    if (initialized)
> +        return;
> +    initialized = 1;
> +
> +    avcodec_register_one(NULL);
> +}
> \ No newline at end of file
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 94e82f7..de936a5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3472,6 +3472,11 @@ void avcodec_register(AVCodec *codec);
>  void avcodec_register_all(void);
>  
>  /**
> + * @returns the number of newly registered entities
> + */
> +int avcodec_register_one(const char *name);
> +
> +/**
>   * Allocate an AVCodecContext and set its fields to default values. The
>   * resulting struct should be freed with avcodec_free_context().
>   *

What's the purpose? It requires global mutable state, and won't work is
more than one library in a process is using ffmpeg. So this would just
add a fundamentally misdesigned API.

If the purpose is to restrict the codecs that can be used (maybe for
security reasons?), then it should be done in a safe manner with no
global mutable state. I can't see any other purpose; it won't prevent
"uneeded" codecs from being pulled in by the linker.

Even worse, it would make it impossible to fix the thread-safety and
library-safety issues of avcodec_register_all(). With the current API,
you would have the chance to by change the codec list to a static table,
which wouldn't require building a codec list at runtime, and thus would
be safe. With this patch applied it's obviously not possible anymore.