[cairo] Memory leak with font variations

Sun Jun 14 20:34:39 UTC 2020

Oops, sorry, wrong thread. :)

On Sun, Jun 14, 2020 at 10:34 PM Tobias Fleischer (reduxFX) <
tobias.fleischer at reduxfx.com> wrote:

> If the new smoothing parameter is set to "Color", it should apply roughly
> the same anti-aliasing technique as AE itself, at least it looked really
> similar when putting a text layer next to the plugins rendered output.
>
> On Sat, Jun 13, 2020 at 11:03 PM Tobias Fleischer (reduxFX) <
> tobias.fleischer at reduxfx.com> wrote:
>
>> Thanks!
>> I will try to test it soon.
>> As I needed a quick and working solution for a bigger project I am
>> working on, for now I simply replaced in my personal Cairo branch all
>> re-assignments of the font variation string with a direct copy of the
>> pointer variable, as the pointer to the string will always exist in my
>> program. But I will check if your patch solves the problem also for the
>> dynamic assignment case.
>> Thanks again,
>> Toby
>>
>>
>> On Sat, Jun 13, 2020 at 10:44 PM Behdad Esfahbod <behdad at behdad.org>
>> wrote:
>>
>>> You are absolutely right.  It's a mess.
>>>
>>> cairo_font_options_t used to be a plain old struct that didn't own
>>> anything.  The ->get_font_options() unfortunately was designed to take a
>>> *options that it assumed didn't need cleanup and just plain wrote to it.
>>>
>>> When we added variations, all those assumptions broke.  I've given it a
>>> try to clean up:
>>>
>>>
>>> https://github.com/behdad/cairo/commit/53cffff1578e4bbd1fa4d8f730c6a963617572bb
>>>
>>> I think I got it all right but am not completely sure.  I think what I
>>> did is the best approach given the mess we are in.
>>>
>>> Please review and test.  It compiles but I haven't tested at all.  I
>>> also wrote the patch on a very old checkout of mine then rebase on master.
>>> I missed any code in between that would need the same treatment but can't
>>> do another review right now.
>>>
>>> Hope someone can take it from there.
>>>
>>> Thanks,
>>> behdad
>>>
>>> PS. I also found this mirror is stalled and confusing:
>>>
>>>   https://github.com/freedesktop/cairo/commits/master
>>>
>>> On Wed, Jun 10, 2020 at 10:39 PM Tobias Fleischer (reduxFX) <
>>> tobias.fleischer at reduxfx.com> wrote:
>>>
>>>> Unfortunately, I just found out it is not the only location where this
>>>> is happening :(
>>>> For example cairo_scaled_font_create() also calls
>>>> _cairo_font_options_init_copy(), which then leads to a leak.
>>>> I tried to add these lines to _cairo_scaled_font_fini_internal():
>>>> _cairo_font_options_fini (&scaled_font->options);
>>>> scaled_font->options.variations = NULL;
>>>>
>>>> But that didn't help, I still get leaks because of unmatched
>>>> malloc/free blocks. Maybe it has to do with the fontmap caching/release
>>>> count here?
>>>>
>>>> Or maybe we shouldn't try to duplicate the variation option string at
>>>> all, but just copy the pointer and say it is up to the calling application
>>>> to guarantee its existence?
>>>> A fixed char array could also be an option but that does sound very
>>>> hacky.
>>>>
>>>> On Thu, Jun 11, 2020 at 7:30 AM Behdad Esfahbod <behdad at behdad.org>
>>>> wrote:
>>>>
>>>>> Thanks Tobias. This definitely makes sense. Thanks for debugging. I'll
>>>>> try to take a look on computer and fix. Unless, I hope, someone beats me to
>>>>> that.
>>>>>
>>>>> On Wed, Jun 10, 2020, 10:25 PM Tobias Fleischer (reduxFX) <
>>>>> tobias.fleischer at reduxfx.com> wrote:
>>>>>
>>>>>> I think I found a bug concerning non-released memory when using font
>>>>>> variations.
>>>>>> I tested against cairo-1.17.2.
>>>>>>
>>>>>> The internal function _cairo_gstate_init_copy() is supposed to make a
>>>>>> deep copy of the fields from one instance/state to another, used for
>>>>>> example by cairo_save(). It does however call
>>>>>> _cairo_font_options_init_copy(), which has this line in it:
>>>>>> options->variations = other->variations ? strdup (other->variations)
>>>>>> : NULL;
>>>>>>
>>>>>> This means that if a font variation string has been set, instead of a
>>>>>> copy, it will always allocate and use a copy of the string (via strdup),
>>>>>> which will then never be freed.
>>>>>> This leads to memory leaks as for example just by calling
>>>>>> cairo_save(), with each call an additional pointer is created that is never
>>>>>> released.
>>>>>>
>>>>>> Simple sample code to reproduce:
>>>>>> cairo_surface_t* surface =
>>>>>> cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 1920, 1080);
>>>>>> cairo_t* cr = cairo_create(surface);
>>>>>> cairo_font_options_t* t = cairo_font_options_create();
>>>>>> cairo_get_font_options(cr, t);
>>>>>> cairo_font_options_set_variations(t, "wght=400");
>>>>>> cairo_set_font_options(cr, t);
>>>>>> cairo_font_options_destroy(t);
>>>>>> cairo_save(cr);
>>>>>> cairo_restore(cr);
>>>>>> cairo_destroy(cr);
>>>>>> cairo_surface_destroy(surface);
>>>>>>
>>>>>> I think what is missing is a matching free-and-null call in
>>>>>> _cairo_gstate_fini().
>>>>>> If I add the following two lines at the beginning
>>>>>> of_cairo_gstate_fini(), it seems to fix this issue, as every allocated copy
>>>>>> gets freed again:
>>>>>> _cairo_font_options_fini (&gstate->font_options);
>>>>>> gstate->font_options.variations = NULL;
>>>>>>
>>>>>> Let me know if this makes sense.
>>>>>> Cheers,
>>>>>> Toby
>>>>>> --
>>>>>> cairo mailing list
>>>>>> cairo at cairographics.org
>>>>>> https://lists.cairographics.org/mailman/listinfo/cairo
>>>>>>
>>>>> --
>>>> cairo mailing list
>>>> cairo at cairographics.org
>>>> https://lists.cairographics.org/mailman/listinfo/cairo
>>>>
>>>
>>>
>>> --
>>> behdad
>>> http://behdad.org/
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.cairographics.org/archives/cairo/attachments/20200614/92533240/attachment-0001.htm>