[FFmpeg-devel] [PATCH] asrc_flite: do not crash on multiple instances.

[Stefano Sabatini]

On date Saturday 2012-07-28 12:09:46 +0200, Nicolas George encoded:
> The voice register functions return the same voice structure
> upon multiple registration. It causes us two problems:
> 
> If we delete a voice without deregistering it, it leaves
> a dangling pointer inside the library.
> 
> If we delete or unregister a voice at uninit, it may still
> be in use by another instance of the filter.
> 

> The second is problem is solved by keeping an usage counter inside
             ^^

> asrc_flite. This is not thread-safe, but neither is flite itself.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/filters.texi         |    2 ++
>  libavfilter/asrc_flite.c |   38 +++++++++++++++++++++++++++-----------
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 37d9e40..dd5b32c 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -1033,6 +1033,8 @@ Synthesize a voice utterance using the libflite library.
>  To enable compilation of this filter you need to configure FFmpeg with
>  @code{--enable-libflite}.
>  
> +Note that the flite library is not thread-safe.
> +
>  The source accepts parameters as a list of @var{key}=@var{value} pairs,
>  separated by ":".
>  
> diff --git a/libavfilter/asrc_flite.c b/libavfilter/asrc_flite.c
> index 9226399..2d68296 100644
> --- a/libavfilter/asrc_flite.c
> +++ b/libavfilter/asrc_flite.c
> @@ -42,6 +42,7 @@ typedef struct {
>      int      wave_nb_samples;
>      int list_voices;

>      cst_voice *voice;

Now redundant? Could be dropped in favor of voice_entry.

> +    struct voice_entry *voice_entry;
>      int64_t pts;
>      int frame_nb_samples; ///< number of samples per frame
>  } FliteContext;
> @@ -64,7 +65,9 @@ AVFILTER_DEFINE_CLASS(flite);
>  static volatile int flite_inited = 0;
>  
>  /* declare functions for all the supported voices */
> -#define DECLARE_REGISTER_VOICE_FN(name) cst_voice *register_cmu_us_## name(const char *)
> +#define DECLARE_REGISTER_VOICE_FN(name) \
> +    cst_voice *register_cmu_us_## name(const char *); \
> +    void     unregister_cmu_us_## name(cst_voice *);
>  DECLARE_REGISTER_VOICE_FN(awb);
>  DECLARE_REGISTER_VOICE_FN(kal);
>  DECLARE_REGISTER_VOICE_FN(kal16);
> @@ -74,14 +77,21 @@ DECLARE_REGISTER_VOICE_FN(slt);
>  struct voice_entry {
>      const char *name;
>      cst_voice * (*register_fn)(const char *);
> +    void (*unregister_fn)(cst_voice *);
> +    cst_voice *voice;
> +    unsigned usage_count;
>  } voice_entry;
>  
> +#define MAKE_VOICE_STRUCTURE(voice_name)                   \
> +    { .name          =                      #voice_name,   \
> +      .register_fn   =   register_cmu_us_ ## voice_name,   \
> +      .unregister_fn = unregister_cmu_us_ ## voice_name, }

Nit++:
#define MAKE_VOICE_STRUCTURE(voice_name) {                 \
      .name          =                      #voice_name,   \
      .register_fn   =   register_cmu_us_ ## voice_name,   \
      .unregister_fn = unregister_cmu_us_ ## voice_name,   \
}

>  static struct voice_entry voice_entries[] = {
> -    { "awb",   register_cmu_us_awb },
> -    { "kal",   register_cmu_us_kal },
> -    { "kal16", register_cmu_us_kal16 },
> -    { "rms",   register_cmu_us_rms },
> -    { "slt",   register_cmu_us_slt },
> +    MAKE_VOICE_STRUCTURE(awb),
> +    MAKE_VOICE_STRUCTURE(kal),
> +    MAKE_VOICE_STRUCTURE(kal16),
> +    MAKE_VOICE_STRUCTURE(rms),
> +    MAKE_VOICE_STRUCTURE(slt),
>  };
>  
>  static void list_voices(void *log_ctx, const char *sep)
> @@ -92,19 +102,22 @@ static void list_voices(void *log_ctx, const char *sep)
>                 voice_entries[i].name, i < (n-1) ? sep : "\n");
>  }
>  
> -static int select_voice(cst_voice **voice, const char *voice_name, void *log_ctx)
> +static int select_voice(struct voice_entry **entry_ret, const char *voice_name, void *log_ctx)
>  {
>      int i;
>  
>      for (i = 0; i < FF_ARRAY_ELEMS(voice_entries); i++) {
>          struct voice_entry *entry = &voice_entries[i];
>          if (!strcmp(entry->name, voice_name)) {
> -            *voice = entry->register_fn(NULL);
> -            if (!*voice) {
> +            if (!entry->voice)
> +                entry->voice = entry->register_fn(NULL);
> +            if (!entry->voice) {
>                  av_log(log_ctx, AV_LOG_ERROR,
>                         "Could not register voice '%s'\n", voice_name);
>                  return AVERROR_UNKNOWN;
>              }
> +            entry->usage_count++;
> +            *entry_ret = entry;
>              return 0;
>          }
>      }
> @@ -142,8 +155,9 @@ static av_cold int init(AVFilterContext *ctx, const char *args)
>          flite_inited++;
>      }
>  
> -    if ((ret = select_voice(&flite->voice, flite->voice_str, ctx)) < 0)
> +    if ((ret = select_voice(&flite->voice_entry, flite->voice_str, ctx)) < 0)
>          return ret;
> +    flite->voice = flite->voice_entry->voice;
>  
>      if (flite->textfile && flite->text) {
>          av_log(ctx, AV_LOG_ERROR,
> @@ -188,8 +202,10 @@ static av_cold void uninit(AVFilterContext *ctx)
>  
>      av_opt_free(flite);
>  
> -    delete_voice(flite->voice);
> +    if (!--flite->voice_entry->usage_count)
> +        flite->voice_entry->unregister_fn(flite->voice);
>      flite->voice = NULL;
> +    flite->voice_entry = NULL;
>      delete_wave(flite->wave);
>      flite->wave = NULL;
>  }

Sounds OK otherwise, thank you.
-- 
FFmpeg = Frenzy & Fabulous Multipurpose Political Earthshaking Gadget