[PATCH v3 0/6] MR1192: win32u: Improve handling of ppem == 0
Some fonts can end up with too low height for some sizes, resulting in ppem == 0, which can lead to some metrics becoming 0 too, especially tmHeight and tmAveCharWidth. This behavior is also observed on Windows, but on Linux it could lead to division by zero. This was noticed with the font `Emmentaler-Brace`, which comes with TexLive and GNU LilyPond, in the comctl32 status test. This patch set also tries to make the Wine behavior closer to the Windows one by enabling tmAveCharWidth to be 0 and preventing it and tmHeight from causing divisions by zero. This is very much an edge case, and I don't know if it has caused problems in actual programs, so I don't know if it would be worth it to make the behavior match exactly. But this way at least the tests should pass if some incomplete/misconfigured fonts are installed. I mainly limited myself to win32u. I'm not sure if these changes could affect behavior in other parts of the codebase, but the tests seems unchanged. -- v3: win32u: Allow tmAveCharWidth to be 0. win32u: Prevent division by 0 in get_transform_matrices(). win32u: check if tmAveCharWidth is 0 before division. win32u: Do not use SCALE_NTM if TM.tmHeight == 0. win32u: Warn when ppem == 0. win32u: Prevent division by zero in scale_font_metrics(). https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> --- dlls/win32u/font.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index 75189f8ec5a..ca0e52be07f 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -4317,7 +4317,7 @@ static void scale_font_metrics( struct gdi_font *font, TEXTMETRICW *tm ) double scale_x, scale_y; /* Make sure that the font has sane width/height ratio */ - if (font->aveWidth && (font->aveWidth + tm->tmHeight - 1) / tm->tmHeight > 100) + if (font->aveWidth && tm->tmHeight && (font->aveWidth + tm->tmHeight - 1) / tm->tmHeight > 100) { WARN( "Ignoring too large font->aveWidth %d\n", font->aveWidth ); font->aveWidth = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> Windows manages to handle this gracefully, but it may have some unforeseen side-effects. Make it warn when ppem == 0 so it may be logged. I'm not using ERR because compatibility-wise this should work correctly. --- dlls/win32u/freetype.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dlls/win32u/freetype.c b/dlls/win32u/freetype.c index d911039647b..3dfd8c5070f 100644 --- a/dlls/win32u/freetype.c +++ b/dlls/win32u/freetype.c @@ -1951,6 +1951,13 @@ static LONG calc_ppem_for_height(FT_Face ft_face, LONG height) ppem = 1; } + /* + * It's possible for EM to be 0 for some fonts, and it should be handled + * gracefully, but it can cause problems, so warn when it happens. + */ + if(ppem == 0) + WARN("Font family %s with style %s and height %i has EM == 0\n", ft_face->family_name, ft_face->style_name, height); + return ppem; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> --- dlls/win32u/font.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index ca0e52be07f..21a5de17b4b 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -3390,13 +3390,23 @@ static BOOL get_face_enum_data( struct gdi_font_face *face, ENUMLOGFONTEXW *elf, #define SCALE_NTM(value) (muldiv( ntm->ntmTm.tmHeight, (value), TM.tmHeight )) cell_height = TM.tmHeight / ( -lf.lfHeight / font->otm.otmEMSquare ); ntm->ntmTm.tmHeight = muldiv( ntm_ppem, cell_height, font->otm.otmEMSquare ); - ntm->ntmTm.tmAscent = SCALE_NTM( TM.tmAscent ); + if (TM.tmHeight != 0) + { + ntm->ntmTm.tmAscent = SCALE_NTM( TM.tmAscent ); + ntm->ntmTm.tmInternalLeading = SCALE_NTM( TM.tmInternalLeading ); + ntm->ntmTm.tmExternalLeading = SCALE_NTM( TM.tmExternalLeading ); + ntm->ntmTm.tmAveCharWidth = SCALE_NTM( TM.tmAveCharWidth ); + ntm->ntmTm.tmMaxCharWidth = SCALE_NTM( TM.tmMaxCharWidth ); + } + else + { + ntm->ntmTm.tmAscent = 0; + ntm->ntmTm.tmInternalLeading = 0; + ntm->ntmTm.tmExternalLeading = 0; + ntm->ntmTm.tmAveCharWidth = 0; + ntm->ntmTm.tmMaxCharWidth = 0; + } ntm->ntmTm.tmDescent = ntm->ntmTm.tmHeight - ntm->ntmTm.tmAscent; - ntm->ntmTm.tmInternalLeading = SCALE_NTM( TM.tmInternalLeading ); - ntm->ntmTm.tmExternalLeading = SCALE_NTM( TM.tmExternalLeading ); - ntm->ntmTm.tmAveCharWidth = SCALE_NTM( TM.tmAveCharWidth ); - ntm->ntmTm.tmMaxCharWidth = SCALE_NTM( TM.tmMaxCharWidth ); - memcpy((char *)&ntm->ntmTm + offsetof( TEXTMETRICW, tmWeight ), (const char *)&TM + offsetof( TEXTMETRICW, tmWeight ), sizeof(TEXTMETRICW) - offsetof( TEXTMETRICW, tmWeight )); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> tmAveCharWidth and font->aveWidth are apparently unrelated, so both should be checked. --- dlls/win32u/font.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index 21a5de17b4b..10c0c2a6aa8 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -4118,7 +4118,7 @@ static void scale_outline_font_metrics( const struct gdi_font *font, OUTLINETEXT { double scale_x, scale_y; - if (font->aveWidth) + if (font->aveWidth && font->otm.otmTextMetrics.tmAveCharWidth) { scale_x = (double)font->aveWidth; scale_x /= (double)font->otm.otmTextMetrics.tmAveCharWidth; @@ -4333,7 +4333,7 @@ static void scale_font_metrics( struct gdi_font *font, TEXTMETRICW *tm ) font->aveWidth = 0; } - if (font->aveWidth) + if (font->aveWidth && font->otm.otmTextMetrics.tmAveCharWidth) { scale_x = (double)font->aveWidth; scale_x /= (double)font->otm.otmTextMetrics.tmAveCharWidth; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> --- dlls/win32u/freetype.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/win32u/freetype.c b/dlls/win32u/freetype.c index 3dfd8c5070f..26ab8e11cf7 100644 --- a/dlls/win32u/freetype.c +++ b/dlls/win32u/freetype.c @@ -2607,11 +2607,17 @@ static FT_Matrix *get_transform_matrices( struct gdi_font *font, BOOL vertical, matrices[matrix_unrotated] = identity_mat; /* Scaling factor */ - if (font->aveWidth) + if (font->aveWidth && font->otm.otmTextMetrics.tmAveCharWidth) { if (!freetype_set_outline_text_metrics( font )) freetype_set_bitmap_text_metrics( font ); - width_ratio = (double)font->aveWidth; - width_ratio /= (double)font->otm.otmTextMetrics.tmAveCharWidth; + /* Metrics were recalculated, so test again */ + if (font->aveWidth && font->otm.otmTextMetrics.tmAveCharWidth) + { + width_ratio = (double)font->aveWidth; + width_ratio /= (double)font->otm.otmTextMetrics.tmAveCharWidth; + } + else + width_ratio = font->scale_y; } else width_ratio = font->scale_y; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
From: Tarcísio Ladeia de Oliveira<wyrquill(a)gmail.com> This follows behavior observed in tests where ppem == 0. As a consequence, tmMaxCharWidth can also be 0. --- dlls/win32u/freetype.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/win32u/freetype.c b/dlls/win32u/freetype.c index 26ab8e11cf7..21a036f2599 100644 --- a/dlls/win32u/freetype.c +++ b/dlls/win32u/freetype.c @@ -3757,9 +3757,6 @@ static BOOL freetype_set_outline_text_metrics( struct gdi_font *font ) (pHori->Ascender - pHori->Descender)))); TM.tmAveCharWidth = SCALE_X(pOS2->xAvgCharWidth); - if (TM.tmAveCharWidth == 0) { - TM.tmAveCharWidth = 1; - } TM.tmMaxCharWidth = SCALE_X(ft_face->bbox.xMax - ft_face->bbox.xMin); TM.tmWeight = FW_REGULAR; if (font->fake_bold) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1192
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125676 Your paranoid android. === debian11 (32 bit report) === gdi32: font.c:6523: Test failed: expected a positive value, got 1
participants (2)
-
Marvin -
Tarcísio Ladeia de Oliveira