Re: [ft-devel] PCF: Issues with lazy copy in `ft_bitmap_glyph_init'

[Werner LEMBERG]

>> Here is all the confusing parts about FT_Get_Glyph:
>>
>> - It takes ownership on the bitmap without copying the data. Thus,
>>   it should not be called twice on the same GlyphSlot and should
>>   fail if FT_GLYPH_OWN_BITMAP is not set. The current behavior with
>>   deep copy when FT_GLYPH_OWN_BITMAP is not set is a bug that need
>>   to be fixed.

I've just tried that, see attached patch.  However, this patch makes
BDF files fail with current ftview: Bitmap fonts are no longer
displayed.  Interestingly, PCF files work...

Care to debug?  I won't have time the next few days I fear...

>> - It copies the outline data without modifying original
>>   ownership. So it is the opposite of bitmap glyphs.  To add to the
>>   confusion FT_OUTLINE_OWNER is in a different place.

Sigh.

>> I think we should stick to the lazy copy in both cases because it
>> is usually used once.  The deep copy is a waste because
>> FT_GlyphSlot is usually discarded.  I do not think the deep copy is
>> useful.
> 
> What is the usual use case and why are there two (very) different
> glyph containers in first place (`FT_Glyph' and `FT_GlyphSlot')?

A glyph slot is always tied to a face (which normally contains a
single one).  On the other hand `FT_Glyph' objects are containers for
`glyph objects'.  The idea is to hide the type; either bitmaps or
outlines should be handled identically.

> I would expect that `FT_GlyphSlot' is a wrapper around `FT_Glyph' to
> add more information that is e.g. needed for rendering etc.  But it
> turns out that they are two rather distinct things.

Yes.  It's unfortunate that we have `face->glyph' and not
`face->glyph_slot'...

> I totally agree with whatever the two of you cook up; just trying to
> shed some light from a user's perspective who tries to work with
> FreeType only (!) reading the API docs (not trying to understand any
> of the black magic that goes on in the depths of FT):

This is very helpful.

> The docs surrounding `FT_Get_Glyph' really make it look like I get
> an independent `FT_Glyph' that can be used whatever.

Yeah, bad documentation...

> I would definitely add several warning signs to the docs.  Also, can
> FreeType become the owner of that generated `FT_Glyph'?

No, the ownership of an `FT_Glyph' object can't be taken back.
However, you can use `FT_GlyphSlot_Own_Bitmap' to make a glyph slot
object own a bitmap again – as long as the object exists that took
away the ownership.

> That way the confusion with `FT_Done_Glyph' would vanish & it would
> be clearer that I will always get the same (maybe, but who cares?)
> object from calling `FT_Get_Glyph' -- changes that I make to that
> object might get propageted into `FT_GlyphSlot' (also something that
> should be put into the docs explicitly IMO).

Please provide a documentation patch.

> Reflecting on it, I do really believe that the call to
> `FT_Done_Glyph' is the source of much confusion (at least for me).

Again, please provide a patch that we can work on.


    Werner

2018-09-01  Werner Lemberg  <address@hidden>

        * src/base/ftglyph.c (ft_bitmap_glyph_init): Disallow deep copying.

        Right now, calling `FT_Get_Glyph' more than once would lead to
        double frees if you follow the documentation, cf. the thread
        starting with

          
http://lists.nongnu.org/archive/html/freetype-devel/2018-08/msg00069.html

        This commit fixes the issue and documents it (which is not
        necessarily the best solution).

diff --git a/include/freetype/ftglyph.h b/include/freetype/ftglyph.h
index d570b205c..7e68ec365 100644
--- a/include/freetype/ftglyph.h
+++ b/include/freetype/ftglyph.h
@@ -260,8 +260,11 @@ FT_BEGIN_HEADER
    *   FT_Get_Glyph
    *
    * @description:
-   *   A function used to extract a glyph image from a slot.  Note that the
-   *   created @FT_Glyph object must be released with @FT_Done_Glyph.
+   *   Extract a glyph image from a slot.  Note that the created @FT_Glyph
+   *   object must be released with @FT_Done_Glyph.
+   *
+   *   If `slot` holds a bitmap, this function can be called only once since
+   *   it takes ownership of the bitmap.
    *
    * @input:
    *   slot ::
@@ -278,6 +281,41 @@ FT_BEGIN_HEADER
    *   Because `*aglyph->advance.x` and `*aglyph->advance.y` are 16.16
    *   fixed-point numbers, `slot->advance.x` and `slot->advance.y` (which
    *   are in 26.6 fixed-point format) must be in the range ]-32768;32768[.
+   *
+   *   If you need to create more than a single @FT_Glyph object from
+   *   `slot`, use code similar to the following example (which works
+   *   for both bitmap and outline glyph formats).
+   *
+   *   {
+   *     FT_Glyph  reference, glyph1, glyph2;
+   *
+   *
+   *     FT_Load_Glyph( face, 0, 0 );
+   *     FT_Get_Glyph( face->glyph, &reference );
+   *
+   *     FT_Glyph_Copy( reference, &glyph1 );
+   *     FT_Glyph_Copy( reference, &glyph2 );
+   *     ...
+   *     FT_Done_Glyph( reference );
+   *     FT_Done_Glyph( glyph1 );
+   *     FT_Done_Glyph( glyph2 );
+   *   }
+   *
+   *   An alternative is to make `slot` own the bitmap again (i.e., copying
+   *   back) after it has been taken by `FT_Get_Glyph`.
+   *
+   *   {
+   *     FT_Load_Glyph( face, 0, 0 );
+   *     FT_Get_Glyph( face->glyph, &glyph1 );
+   *
+   *     FT_GlyphSlot_Own_Bitmap( face->glyph );
+   *     FT_Get_Glyph( face->glyph, &glyph2 );
+   *
+   *     FT_Done_Glyph( glyph1 );
+   *     FT_Done_Glyph( glyph2 );
+   *   }
+   *
+   *   Error handling is omitted in both examples for simplicity.
    */
   FT_EXPORT( FT_Error )
   FT_Get_Glyph( FT_GlyphSlot  slot,
diff --git a/src/base/ftglyph.c b/src/base/ftglyph.c
index 27402ecf8..54bd35427 100644
--- a/src/base/ftglyph.c
+++ b/src/base/ftglyph.c
@@ -59,9 +59,8 @@
   ft_bitmap_glyph_init( FT_Glyph      bitmap_glyph,
                         FT_GlyphSlot  slot )
   {
-    FT_BitmapGlyph  glyph   = (FT_BitmapGlyph)bitmap_glyph;
-    FT_Error        error   = FT_Err_Ok;
-    FT_Library      library = FT_GLYPH( glyph )->library;
+    FT_BitmapGlyph  glyph = (FT_BitmapGlyph)bitmap_glyph;
+    FT_Error        error = FT_Err_Ok;
 
 
     if ( slot->format != FT_GLYPH_FORMAT_BITMAP )
@@ -73,17 +72,14 @@
     glyph->left = slot->bitmap_left;
     glyph->top  = slot->bitmap_top;
 
-    /* do lazy copying whenever possible */
+    /* do lazy copying */
     if ( slot->internal->flags & FT_GLYPH_OWN_BITMAP )
     {
       glyph->bitmap          = slot->bitmap;
       slot->internal->flags &= ~FT_GLYPH_OWN_BITMAP;
     }
     else
-    {
-      FT_Bitmap_Init( &glyph->bitmap );
-      error = FT_Bitmap_Copy( library, &slot->bitmap, &glyph->bitmap );
-    }
+      error = FT_THROW( Invalid_Argument );
 
   Exit:
     return error;