Re: [Devel] pcfdriver.c memory leak

[Detlef Würkner]

address@hidden (Francesco Zappa Nardelli) wrote:

> DT> Done, as well as the patch managing the "AVERAGE_WIDTH"
> DT> property.. _Please_ test..
> 
> Humpf... I was talking of the second patch, not of the first one.
> 
> When I wrote the PCF driver I was extremely reluctant to use all the
> information provided in the properties because of some bizarre
> informations contained there...

Just looked at the sfnt and winfnt driver source and found that
the whole bitmap width/height problem seems to be a misunderstanding.

include/freetype/freetype.h says
----8<----
  /*    FT_Bitmap_Size                                                     */
  /*                                                                       */
  /* <Description>                                                         */
  /*    An extremely simple structure used to model the size of a bitmap   */
  /*    strike (i.e., a bitmap instance of the font for a given            */
  /*    resolution) in a fixed-size font face.  This is used for the       */
  /*    `available_sizes' field of the FT_Face_Properties structure.       */
  /*                                                                       */
  /* <Fields>                                                              */
  /*    height :: The character height in pixels.                          */
  /*                                                                       */
  /*    width  :: The character width in pixels.                           */
----8<----

and src/winfonts/winfnt.c says
----8<----
        FT_Bitmap_Size*  size = root->available_sizes;


        for ( cur = fonts; cur < limit; cur++, size++ )
        {
          size->width  = cur->header.pixel_width;
          size->height = cur->header.pixel_height;
        }
----8<----

which IMHO both means the width and height are in pixels.
But the sfnt driver says
----8<----
        for ( n = 0 ; n < face->num_sbit_strikes ; n++ )
        {
          root->available_sizes[n].width =
            face->sbit_strikes[n].x_ppem;

          root->available_sizes[n].height =
            face->sbit_strikes[n].y_ppem;
        }
----8<----

which IMHO means the width and height are in (something like)
points, which maybe is the reason the .ttf/.ttc fonts I have
here all have "cubic" bitmaps (width equal height, which means
resolution 1:1)?

The original PCF driver you created used point units for width
and height, similar to the sfnt driver. If I remember it right, 
my first patch some time ago (the PIXEL_SIZE / POINT_SIZE /
RESOLUTION_X / RESOLUTION_Y patch) changed its behaviour to
report different sizes for 75dpi and 100dpi fonts,
which is wrong if the size is in points, but was also not
correct if the size is in pixels.

The next patch (AVERAGE_WIDTH) was for reporting the width
in pixels. OK, it could have more error checking...

The last patch (accel->maxbounds) finally changes width and
height from points to pixels.

If the size shall be in points, I would suggest to undo
all three patches and just add using POINT_SIZE when
PIXEL_SIZE, RESOLUTION_X and RESOLUTION_Y cant be found.

If the size shall be in pixels, I suggest applying all
patches.

When the size shall be in points, it is IMHO necessary to
add sufficient metrics to the FT_Face object to be able
to compute the pixel size from the point size. Currently,
bbox.xMin, bbox.yMin, bbox.xMax, bbox.yMax, units_per_EM,
ascender, descender, height, max_advance_width and max_advance_height
are all zero.


Maybe it would be best if a consistent handling method for
all (currently 3) bitmap font formats supported by FreeType2
can be found and documented. What do the experts think?


> DW> Sorry, found some fonts with negative AVERAGE_WIDTH...
> 
> That is...  Right now I cannot test last Detlef's patch, but from the
> top of my mind I remeber that not all pcf fonts are required to have
> the accelerator table.  In such case you should trust the properties
> derived from the XLFD information.  Thus, I do not think it should be
> applied (and the AVERAGE_WIDTH patch should be undoed).

Your source code IMHO ensures that an accelerator table is present:
----8<----
    if ( !hasBDFAccelerators )
    {
      error = pcf_get_accel( stream, face, PCF_ACCELERATORS );
      if ( error )
        goto Bail;
    }
[...]
    if ( hasBDFAccelerators )
    {
      error = pcf_get_accel( stream, face, PCF_BDF_ACCELERATORS );
      if ( error )
        goto Bail;
    }
[...]
      if ( face->accel.constantWidth )
        root->face_flags |= FT_FACE_FLAG_FIXED_WIDTH;
[...]
  Bail:
    PCF_Done_Face( face );
    return PCF_Err_Invalid_File_Format;
----8<----

> DW> The XFree86 base fonts all have usable sizes with it, hopefully
> DW> its now foolproof enough...
> 
> Unfortunately, not all PCF fonts have been compiled with XFree86.
> 
> By the way, please check whether the original BDF font (or the
> original XLFD) has a negative AVERAGE_WIDTH property or not.

Debugging output said there exist XFree86 fonts with negative
values in either accel->maxbounds.characterWidth|ascent|descent or
AVERAGE_WIDTH (but not in both). The last patch checks all these
values and uses a fallback mechanism that should produce reasonable
(pixel) sizes for hopefully all cases, even when accel values and
AVERAGE_WIDTH are negative.

I have downloaded the XFree86 BDF fonts and currently examine them.
The AVERAGE_WIDTH problem seems to be that some fonts contain this
value as a string, not as an integer:
lubB19.bdf:AVERAGE_WIDTH "114"
lubB24.bdf:AVERAGE_WIDTH 143

A workaround for the PCF driver would be something like

prop = find_property( face, "AVERAGE_WIDTH" );
if ( prop != NULL )
{
  if ( prop->isString )
    width = ( atoi( prop->value.atom ) + 5 ) / 10;
  else
    width = ( prop->value.integer + 5 ) / 10;
}
(introducing correct rounding...)

The accel values are negative sometimes, more in the next mail.

Ciao, Detlef
-- 
_ // address@hidden
\X/  Detlef Wuerkner, Langgoens/Germany