Re: [PATCH] gdi32: Support special 'ttcf' tag in GetFontData(), fix individual collection item offset

[Huw Davies]

On Fri, Aug 12, 2016 at 01:47:39AM +0300, Nikolay Sivov wrote:
> Signed-off-by: Nikolay Sivov
>
> dlls/gdi32/freetype.c | 53 ++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
> index 5ddfc6a..f4aab0b 100644
> --- a/dlls/gdi32/freetype.c
> +++ b/dlls/gdi32/freetype.c
> @@ -394,6 +396,8 @@ struct tagGdiFont {
>      int codepage;
>      BOOL fake_italic;
>      BOOL fake_bold;
> - BOOL is_ttc_item;
> - ULONG ttc_item_offset;

Since ttc_item_offset can never be zero in a ttc, you don't need the
boolean.
>  BYTE underline;
>  BYTE strikeout;
>  INT orientation;
>
> @@ -2189,7 +2193,7 @@ static FT_Face new_ft_face( const char *file, void *font_data_ptr, DWORD font_da
>              !pFT_Get_Sfnt_Table( ft_face, ft_sfnt_hhea ) ||
>              !pFT_Get_Sfnt_Table( ft_face, ft_sfnt_head ))
>          {
> -
>        TRACE("Font %s/%p lacks either an OS2, HHEA or HEAD table.\n"
> -
>        TRACE("Font %s/%p lacks either an OS/2, hhea or head table.\n"
>              "Skipping this font.\n", debugstr_a(file), font_data_ptr);
>        goto fail;
>    }

Unrelated.
> @@ -4535,6 +4539,9 @@ static FT_Face OpenFontFace(GdiFont *font, Face *face, LONG width, LONG height)
>      font->ft_face = ft_face;
>  if(FT_IS_SCALABLE(ft_face)) {
> -
>    FT_ULong len;
> -
>    DWORD header;
> -
>    /* load the VDMX table if we have one */
>    font->ppem = load_VDMX(font, height);
>    if(font->ppem == 0)
>
> @@ -4543,6 +4550,22 @@ static FT_Face OpenFontFace(GdiFont *font, Face *face, LONG width, LONG height)
>      if((err = pFT_Set_Pixel_Sizes(ft_face, 0, font->ppem)) != 0)
>          WARN("FT_Set_Pixel_Sizes %d, %d rets %x\n", 0, font->ppem, err);
> -
> -
>    /* see if it's a TTC */
> -
>    len = sizeof(header);
> -
>    if (!pFT_Load_Sfnt_Table(ft_face, 0, 0, (void*)&header, &len)) {
> -
>        const DWORD ttcf = 0x66637474;

Please define and use the 'ttcf' tag.
> @@ -4638,12 +4661,18 @@ static void free_font(GdiFont *font)
>      HeapFree(GetProcessHeap(), 0, font);
>  }
>
> +#define MS_MAKE_TAG( _x1, _x2, _x3, _x4 ) \
> -
>      ( ( (FT_ULong)_x4 << 24 ) |     \
> -
>        ( (FT_ULong)_x3 << 16 ) |     \
> -
>        ( (FT_ULong)_x2 <<  8 ) |     \
> -
>          (FT_ULong)_x1         )

We already have this define later on, we should probably move it to the top
of the file (under the GET_BE_(D)WORD defines looks like a good place) and
move all the various tag definitions up there too.  This should be done as
a separate, preliminary patch.
> static DWORD get_font_data( GdiFont *font, DWORD table, DWORD offset, LPVOID buf, DWORD cbData)
>  {
>      FT_Face ft_face = font->ft_face;
>      FT_ULong len;
>      FT_Error err;
> -
>
> DWORD tag;
>
> if (!FT_IS_SFNT(ft_face)) return GDI_ERROR;
>
> @@ -4652,7 +4681,17 @@ static DWORD get_font_data( GdiFont *font, DWORD table, DWORD offset, LPVOID buf
>      else
>          len = cbData;
> - table = RtlUlongByteSwap( table );  /* MS tags differ in endianness from FT ones */
> - table = RtlUlongByteSwap( tag = table );  /* MS tags differ in endianness from FT ones */
> -
> - /* if font is a member of TTC, 'ttcf' tag allows reading from beginning of TTC file,
> -
>   0 tag means to read from start of collection member data. */
> - if (font->is_ttc_item)
> - {
> -
>    if (tag == MS_MAKE_TAG('t','t','c','f'))
> -
>        table = 0;
> -
>    else if (tag == 0)
> -
>        offset += font->ttc_item_offset;
> - }

I find the introduction of tag here confusiong.  You should be able to
do this without the second variable.
> return GDI_ERROR;
>      }
>      return len;
> @@ -8246,9 +8282,8 @@ static DWORD freetype_GetFontData( PHYSDEV dev, DWORD table, DWORD offset, LPVOI
>          return dev->funcs->pGetFontData( dev, table, offset, buf, cbData );
>      }
> - TRACE("font=%p, table=%c%c%c%c, offset=0x%x, buf=%p, cbData=0x%x\n",
> -
>      physdev->font, LOBYTE(LOWORD(table)), HIBYTE(LOWORD(table)),
> -
>      LOBYTE(HIWORD(table)), HIBYTE(HIWORD(table)), offset, buf, cbData);
> - TRACE("font=%p, table=%s, offset=0x%x, buf=%p, cbData=0x%x\n",
> -
>      physdev->font, debugstr_an((char*)&table, 4), offset, buf, cbData);

Doesn't this print the tag backwards?

Huw.