[FFmpeg-devel] [PATCH v2] libavfi/vf_drawtext: fix memory management when destroying font face

[Leandro Santiago]

Just as a ping, to know whether anyone is able to have a look at this patch :-)

Please let me know if you folks need extra context or info.

31 Oct 2024 21:48:31 Leandro Santiago <leandrosansilva at gmail.com>:

> Ref https://trac.ffmpeg.org/ticket/11152
> 
> According to harfbuzz docs, hb_ft_font_set_funcs() does not need to be
> called, as, quoted:
> 
> ```
> An #hb_font_t object created with hb_ft_font_create()
> is preconfigured for FreeType font functions and does not
> require this function to be used.
> ```
> 
> Using this function seems to cause memory management issues between
> harfbuzz and freetype, and could be eliminated.
> 
> This commit also call hb_ft_font_changed() when the underlying FC_Face
> changes size, as stated on hardbuzz:
> 
> ```
> HarfBuzz also provides a utility function called hb_ft_font_changed() that you should call
> whenever you have altered the properties of your underlying FT_Face, as well as a hb_ft_get_face()
> that you can call on an hb_font_t font object to fetch its underlying FT_Face.
> ```
> 
> Finally, the execution order between hb_font_destroy() and
> hb_buffer_destroy() is flipped to match the order of creation of
> the respective objects.
> 
> Signed-off-by: Leandro Santiago <leandrosansilva at gmail.com>
> ---
> libavfilter/vf_drawtext.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 2b0a21a4b4..cf99b4e59e 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -445,6 +445,7 @@ static int glyph_cmp(const void *key, const void *b)
> static av_cold int set_fontsize(AVFilterContext *ctx, unsigned int fontsize)
> {
>      int err;
> +    int line;
>      DrawTextContext *s = ctx->priv;
> 
>      if ((err = FT_Set_Pixel_Sizes(s->face, 0, fontsize))) {
> @@ -453,6 +454,12 @@ static av_cold int set_fontsize(AVFilterContext *ctx, unsigned int fontsize)
>          return AVERROR(EINVAL);
>      }
> 
> +    // Whenever the underlying FT_Face changes, harfbuzz has to be notified of the change.
> +    for (line = 0; line < s->line_count; line++) {
> +      TextLine *cur_line = &s->lines[line];
> +      hb_ft_font_changed(cur_line->hb_data.font);
> +    }
> +
>      s->fontsize = fontsize;
> 
>      return 0;
> @@ -1365,15 +1372,17 @@ static int shape_text_hb(DrawTextContext *s, HarfbuzzData* hb, const char* text,
>      if(!hb_buffer_allocation_successful(hb->buf)) {
>          return AVERROR(ENOMEM);
>      }
> +
>      hb_buffer_set_direction(hb->buf, HB_DIRECTION_LTR);
>      hb_buffer_set_script(hb->buf, HB_SCRIPT_LATIN);
>      hb_buffer_set_language(hb->buf, hb_language_from_string("en", -1));
>      hb_buffer_guess_segment_properties(hb->buf);
> -    hb->font = hb_ft_font_create(s->face, NULL);
> +
> +    hb->font = hb_ft_font_create_referenced(s->face);
>      if(hb->font == NULL) {
>          return AVERROR(ENOMEM);
>      }
> -    hb_ft_font_set_funcs(hb->font);
> +
>      hb_buffer_add_utf8(hb->buf, text, textLen, 0, -1);
>      hb_shape(hb->font, hb->buf, NULL, 0);
>      hb->glyph_info = hb_buffer_get_glyph_infos(hb->buf, &hb->glyph_count);
> @@ -1384,8 +1393,8 @@ static int shape_text_hb(DrawTextContext *s, HarfbuzzData* hb, const char* text,
> 
> static void hb_destroy(HarfbuzzData *hb)
> {
> -    hb_buffer_destroy(hb->buf);
>      hb_font_destroy(hb->font);
> +    hb_buffer_destroy(hb->buf);
>      hb->buf = NULL;
>      hb->font = NULL;
>      hb->glyph_info = NULL;
> -- 
> 2.46.1