[cairo] Memory leak with font variations

Sun Jun 14 20:34:05 UTC 2020

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/94faddb6/attachment.htm>