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 <nsivov(a)codeweavers.com>
> ---
>  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.