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

[Marton Balint]

On Thu, 14 Nov 2024, Leandro Santiago wrote:

> 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.

Will apply with some cosmetic changes.

Thanks,
Marton

>
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".