[PATCH v4 0/3] MR3876: Draft: Zero-width character handling in GetTextExtentExPoint[AW]
Certain characters are considered by GetTextExtentExPointA/W to have no width, even if they have a non-zero width and a shape in the font. This MR attempts to bring Wine's implementation of these functions closer to native behavior, though I was not able to get it anywhere near perfect. -- v4: win32u: Handle glyphs that are marked by the font as zero-width in GetTextExtentExPoint. win32u: Handle characters that are hard-coded to have no width in GetTextExtentExPoint. gdi32/tests: Add test for width of carriage return and line feed. https://gitlab.winehq.org/wine/wine/-/merge_requests/3876
From: Torge Matthies <tmatthies(a)codeweavers.com> Signed-off-by: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/gdi32/tests/font.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/gdi32/tests/font.c b/dlls/gdi32/tests/font.c index 711c9ca4a32..2388b91b5e6 100644 --- a/dlls/gdi32/tests/font.c +++ b/dlls/gdi32/tests/font.c @@ -1399,7 +1399,7 @@ static void test_GetCharABCWidths(void) static void test_text_extents(void) { - static const WCHAR wt[] = L"One\ntwo 3"; + static const WCHAR wt[] = L"One\r\ntwo 3\nfour"; LPINT extents; INT i, len, fit1, fit2, extents2[3]; LOGFONTA lf; @@ -1462,11 +1462,20 @@ static void test_text_extents(void) ok(extents[len-1] == sz1.cx, "GetTextExtentExPointW extents and size don't match\n"); ok(0 <= fit1 && fit1 <= len, "GetTextExtentExPointW generated illegal value %d for fit\n", fit1); ok(0 < fit1, "GetTextExtentExPointW says we can't even fit one letter in 32767 logical units\n"); + todo_wine + ok(extents[2] == extents[3], "Carriage return has width: %d != %d\n", extents[2], extents[3]); + todo_wine + ok(extents[3] == extents[4], "Line feed has width: %d != %d\n", extents[3], extents[4]); + todo_wine + ok(extents[9] == extents[10], "Unix-style newline has width: %d != %d\n", extents[3], extents[4]); GetTextExtentExPointW(hdc, wt, len, extents[2], &fit2, NULL, &sz2); ok(sz1.cx == sz2.cx && sz1.cy == sz2.cy, "GetTextExtentExPointW returned different sizes for the same string\n"); ok(fit2 == 3, "GetTextExtentExPointW extents isn't consistent with fit\n"); GetTextExtentExPointW(hdc, wt, len, extents[2]-1, &fit2, NULL, &sz2); ok(fit2 == 2, "GetTextExtentExPointW extents isn't consistent with fit\n"); + GetTextExtentExPointW(hdc, wt, len, extents[2]+1, &fit2, NULL, &sz2); + todo_wine + ok(fit2 == 5, "GetTextExtentExPointW newline doesn't fit in 1-pixel space\n"); GetTextExtentExPointW(hdc, wt, 2, 0, NULL, extents + 2, &sz2); ok(extents[0] == extents[2] && extents[1] == extents[3], "GetTextExtentExPointW with lpnFit == NULL returns incorrect results\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3876
From: Torge Matthies <tmatthies(a)codeweavers.com> These rules only apply when the glyph is passed by unicode code point, not by glyph index in the font (GGO_GLYPH_INDEX). Signed-off-by: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/gdi32/tests/font.c | 4 -- dlls/gdiplus/tests/graphics.c | 2 +- dlls/win32u/font.c | 108 ++++++++++++++++++++++++++++++---- 3 files changed, 99 insertions(+), 15 deletions(-) diff --git a/dlls/gdi32/tests/font.c b/dlls/gdi32/tests/font.c index 2388b91b5e6..65e8064491c 100644 --- a/dlls/gdi32/tests/font.c +++ b/dlls/gdi32/tests/font.c @@ -1462,11 +1462,8 @@ static void test_text_extents(void) ok(extents[len-1] == sz1.cx, "GetTextExtentExPointW extents and size don't match\n"); ok(0 <= fit1 && fit1 <= len, "GetTextExtentExPointW generated illegal value %d for fit\n", fit1); ok(0 < fit1, "GetTextExtentExPointW says we can't even fit one letter in 32767 logical units\n"); - todo_wine ok(extents[2] == extents[3], "Carriage return has width: %d != %d\n", extents[2], extents[3]); - todo_wine ok(extents[3] == extents[4], "Line feed has width: %d != %d\n", extents[3], extents[4]); - todo_wine ok(extents[9] == extents[10], "Unix-style newline has width: %d != %d\n", extents[3], extents[4]); GetTextExtentExPointW(hdc, wt, len, extents[2], &fit2, NULL, &sz2); ok(sz1.cx == sz2.cx && sz1.cy == sz2.cy, "GetTextExtentExPointW returned different sizes for the same string\n"); @@ -1474,7 +1471,6 @@ static void test_text_extents(void) GetTextExtentExPointW(hdc, wt, len, extents[2]-1, &fit2, NULL, &sz2); ok(fit2 == 2, "GetTextExtentExPointW extents isn't consistent with fit\n"); GetTextExtentExPointW(hdc, wt, len, extents[2]+1, &fit2, NULL, &sz2); - todo_wine ok(fit2 == 5, "GetTextExtentExPointW newline doesn't fit in 1-pixel space\n"); GetTextExtentExPointW(hdc, wt, 2, 0, NULL, extents + 2, &sz2); ok(extents[0] == extents[2] && extents[1] == extents[3], diff --git a/dlls/gdiplus/tests/graphics.c b/dlls/gdiplus/tests/graphics.c index 89baff64fa4..309a1b1e8ed 100644 --- a/dlls/gdiplus/tests/graphics.c +++ b/dlls/gdiplus/tests/graphics.c @@ -3358,7 +3358,7 @@ static void test_string_functions(void) expectf_(char_bounds.Width, bounds.Width, 0.01); expectf_(char_bounds.Height + char_height * 3, bounds.Height, 0.05); expect(6, codepointsfitted); - todo_wine expect(4, linesfilled); + expect(4, linesfilled); for (i = 0; i < 4; i++) regions[i] = (GpRegion *)0xdeadbeef; diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index c21d87c1fbc..977f079285b 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -3830,15 +3830,72 @@ static UINT get_glyph_index_linked( struct gdi_font **font, UINT glyph ) return 0; } +struct zero_width_unicode_range +{ + WORD start; + WORD count; +} zw_ranges[] = { +#define X1(c) { (c), 1 } +#define X2(start, end) { (start), (end) + 1 - (start) } + + X2(0x001c, 0x001f), /* information separators */ + X2(0x0080, 0x009f), /* C1 controls */ + X1(0x034f), /* COMBINING GRAPHEME JOINER */ /* squishing */ + X1(0x0605), /* ARABIC NUMBER MARK ABOVE */ + X1(0x061c), /* ARABIC LETTER MARK */ + X1(0x0674), /* ARABIC LETTER HIGH HAMZA */ + X2(0x06e5, 0x06e6), /* ARABIC SMALL WAW, ARABIC SMALL YEH */ + X1(0x09fe), /* BENGALI SANDHI MARK */ + X1(0x0a51), /* GURMUKHI SIGN UDAAT */ + X1(0x0a75), /* GURMUKHI SIGN YAKASH */ + X2(0x0afa, 0x0aff), /* GUJARATI SIGN SUKUN - GUJARATI SIGN TWO-CIRCLE NUKTA ABOVE */ + X1(0x0c04), /* TELUGU SIGN COMBINING ANUSVARA ABOVE */ + X1(0x0d00), /* MALAYALAM SIGN COMBINING ANUSVARA ABOVE */ + X2(0x0d3b, 0x0d3c), /* MALAYALAM SIGN VERTICAL BAR VIRAMA, MALAYALAM SIGN CIRCULAR VIRAMA */ + X1(0x0e31), /* THAI CHARACTER MAI HAN-AKAT */ + X1(0x0e33), /* THAI CHARACTER SARA AM */ /* squishing */ + X2(0x0e34, 0x0e3a), /* THAI CHARACTER SARA I - THAI CHARACTER PHINTHU */ + X2(0x0e47, 0x0e4e), /* THAI CHARACTER MAITAIKHU - THAI CHARACTER YAMAKKAN */ + X1(0x180e), /* MONGOLIAN VOWEL SEPARATOR */ + X1(0x1878), /* MONGOLIAN LETTER CHA WITH TWO DOTS */ + X1(0x1ce1), /* VEDIC TONE ATHARVAVEDIC INDEPENDENT SVARITA */ + X1(0x1cf7), /* VEDIC SIGN ATIKRAMA */ + X2(0x200b, 0x200f), /* ZWSP, ZWNJ, ZWJ, LRM, RLM */ + X2(0x2029, 0x202e), /* Paragraph separator, LRE, RLE, PDF, LRO, RLO */ + X2(0x2061, 0x2064), /* invisible operators */ + X2(0x206a, 0x206f), /* Deprecated control characters */ + X2(0x302a, 0x302d), /* IDEOGRAPHIC LEVEL TONE MARK - IDEOGRAPHIC ENTERING TONE MARK */ + X2(0x3099, 0x309a), /* COMBINING KATAKANA-HIRAGANA VOICED SOUND MARK, + COMBINING KATAKANA-HIRAGANA SEMI-VOICED SOUND MARK */ + X1(0xa8fa), /* DEVANAGARI CARET */ + X1(0xa8ff), /* DEVANAGARI VOWEL SIGN AY */ + X2(0xfbb2, 0xfbbb), /* ARABIC SYMBOL DOT ABOVE - ARABIC SYMBOL FOUR DOTS BELOW */ + X2(0xfbc0, 0xfbc1), /* ARABIC SYMBOL SMALL TAH ABOVE, ARABIC SYMBOL SMALL TAH BELOW */ + X1(0xfeff), /* ZWNBSP */ + +#undef X1 +#undef X2 +}; + +static inline BOOL is_zero_width_utf16( WORD c ) +{ + size_t i; + for (i = 0; i < ARRAY_SIZE(zw_ranges) && c < zw_ranges[i].start; i++) + if (c - zw_ranges[i].start < zw_ranges[i].count) + return TRUE; + return FALSE; +} + static DWORD get_glyph_outline( struct gdi_font *font, UINT glyph, UINT format, GLYPHMETRICS *gm_ret, ABC *abc_ret, DWORD buflen, void *buf, - const MAT2 *mat ) + const MAT2 *mat, BOOL *zero_width ) { GLYPHMETRICS gm; ABC abc; DWORD ret = 1; UINT index = glyph; BOOL tategaki = (*get_gdi_font_name( font ) == '@'); + UINT orig_format = format; if (format & GGO_GLYPH_INDEX) { @@ -3863,6 +3920,21 @@ static DWORD get_glyph_outline( struct gdi_font *font, UINT glyph, UINT format, if (mat && !memcmp( mat, &identity, sizeof(*mat) )) mat = NULL; + if (zero_width) + { + *zero_width = FALSE; + if (!(orig_format & GGO_GLYPH_INDEX)) + { + /* Windows seems to have a fallback font that marks these characters as zero-width. + Wine does not have such a font. Since applications are likely to depend on the + dimensions of these characters, explicitly make them zero-width here. */ + if (glyph == 0x0009 || glyph == 0x000A || glyph == 0x000D) + *zero_width = TRUE; + else + *zero_width = is_zero_width_utf16( glyph ); + } + } + if (format == GGO_METRICS && !mat && get_gdi_font_glyph_metrics( font, index, &gm, &abc )) goto done; @@ -3873,6 +3945,7 @@ static DWORD get_glyph_outline( struct gdi_font *font, UINT glyph, UINT format, set_gdi_font_glyph_metrics( font, index, &gm, &abc ); done: + if (zero_width && (abc.abcA + abc.abcB + abc.abcC) == 0) *zero_width = TRUE; if (gm_ret) *gm_ret = gm; if (abc_ret) *abc_ret = abc; return ret; @@ -3915,7 +3988,7 @@ static BOOL font_GetCharABCWidths( PHYSDEV dev, UINT first, UINT count, WCHAR *c for (i = 0; i < count; i++) { c = chars ? chars[i] : first + i; - get_glyph_outline( physdev->font, c, GGO_METRICS, NULL, &buffer[i], 0, NULL, NULL ); + get_glyph_outline( physdev->font, c, GGO_METRICS, NULL, &buffer[i], 0, NULL, NULL, NULL ); } pthread_mutex_unlock( &font_lock ); return TRUE; @@ -3941,7 +4014,7 @@ static BOOL font_GetCharABCWidthsI( PHYSDEV dev, UINT first, UINT count, WORD *g pthread_mutex_lock( &font_lock ); for (c = 0; c < count; c++, buffer++) get_glyph_outline( physdev->font, gi ? gi[c] : first + c, GGO_METRICS | GGO_GLYPH_INDEX, - NULL, buffer, 0, NULL, NULL ); + NULL, buffer, 0, NULL, NULL, NULL ); pthread_mutex_unlock( &font_lock ); return TRUE; } @@ -3968,7 +4041,7 @@ static BOOL font_GetCharWidth( PHYSDEV dev, UINT first, UINT count, const WCHAR for (i = 0; i < count; i++) { c = chars ? chars[i] : i + first; - if (get_glyph_outline( physdev->font, c, GGO_METRICS, NULL, &abc, 0, NULL, NULL ) == GDI_ERROR) + if (get_glyph_outline( physdev->font, c, GGO_METRICS, NULL, &abc, 0, NULL, NULL, NULL ) == GDI_ERROR) buffer[i] = 0; else buffer[i] = abc.abcA + abc.abcB + abc.abcC; @@ -4147,7 +4220,7 @@ static DWORD font_GetGlyphOutline( PHYSDEV dev, UINT glyph, UINT format, return dev->funcs->pGetGlyphOutline( dev, glyph, format, gm, buflen, buf, mat ); } pthread_mutex_lock( &font_lock ); - ret = get_glyph_outline( physdev->font, glyph, format, gm, NULL, buflen, buf, mat ); + ret = get_glyph_outline( physdev->font, glyph, format, gm, NULL, buflen, buf, mat, NULL ); pthread_mutex_unlock( &font_lock ); return ret; } @@ -4327,8 +4400,10 @@ static BOOL font_GetTextExtentExPoint( PHYSDEV dev, const WCHAR *str, INT count, pthread_mutex_lock( &font_lock ); for (i = pos = 0; i < count; i++) { - get_glyph_outline( physdev->font, str[i], GGO_METRICS, NULL, &abc, 0, NULL, NULL ); - pos += abc.abcA + abc.abcB + abc.abcC; + BOOL zero_width = FALSE; + get_glyph_outline( physdev->font, str[i], GGO_METRICS, NULL, &abc, 0, NULL, NULL, &zero_width ); + if (!zero_width) + pos += abc.abcA + abc.abcB + abc.abcC; dxs[i] = pos; } pthread_mutex_unlock( &font_lock ); @@ -4356,9 +4431,11 @@ static BOOL font_GetTextExtentExPointI( PHYSDEV dev, const WORD *indices, INT co pthread_mutex_lock( &font_lock ); for (i = pos = 0; i < count; i++) { + BOOL zero_width = FALSE; get_glyph_outline( physdev->font, indices[i], GGO_METRICS | GGO_GLYPH_INDEX, - NULL, &abc, 0, NULL, NULL ); - pos += abc.abcA + abc.abcB + abc.abcC; + NULL, &abc, 0, NULL, NULL, &zero_width ); + if (!zero_width) + pos += abc.abcA + abc.abcB + abc.abcC; dxs[i] = pos; } pthread_mutex_unlock( &font_lock ); @@ -5347,7 +5424,18 @@ BOOL WINAPI NtGdiGetTextExtentExW( HDC hdc, const WCHAR *str, INT count, INT max { unsigned int dx = abs( INTERNAL_XDSTOWS( dc, pos[i] )) + (i + 1) * dc->attr->char_extra; - if (nfit && dx > (unsigned int)max_ext) break; + if (nfit) + { + unsigned int dx2 = dx; + if (i >= 1) + { + unsigned int prev_dx = abs( INTERNAL_XDSTOWS( dc, pos[i - 1] )) + + i * dc->attr->char_extra; + if (dx == prev_dx) + dx2 += 1; + } + if (dx2 > (unsigned int)max_ext) break; + } if (dxs) dxs[i] = dx; } if (nfit) *nfit = i; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3876
From: Torge Matthies <tmatthies(a)codeweavers.com> This always applies, whether the glyph is passed by unicode code point or by glyph index. Signed-off-by: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/win32u/font.c | 131 +++++++++++++++++++++++++++++++++++- dlls/win32u/ntgdi_private.h | 2 + 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index 977f079285b..d9d43cdab85 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -2426,6 +2426,7 @@ static void free_gdi_font( struct gdi_font *font ) free( font->gm ); free( font->kern_pairs ); free( font->gsub_table ); + free( font->gdef_table ); free( font ); } @@ -2833,6 +2834,126 @@ static UINT get_GSUB_vert_glyph( struct gdi_font *font, UINT glyph ) return GSUB_apply_feature( font->gsub_table, font->vert_feature, glyph ); } +/* structures copied from dlls/gdi32/uniscribe/opentype.c */ + +enum {BaseGlyph=1, LigatureGlyph, MarkGlyph, ComponentGlyph}; + +typedef struct { + DWORD Version; + WORD GlyphClassDef; + WORD AttachList; + WORD LigCaretList; + WORD MarkAttachClassDef; +} GDEF_Header; + +typedef struct { + WORD ClassFormat; + WORD StartGlyph; + WORD GlyphCount; + WORD ClassValueArray[1]; +} OT_ClassDefFormat1; + +typedef struct { + WORD Start; + WORD End; + WORD Class; +} OT_ClassRangeRecord; + +typedef struct { + WORD ClassFormat; + WORD ClassRangeCount; + OT_ClassRangeRecord ClassRangeRecord[1]; +} OT_ClassDefFormat2; + +static void *load_GDEF( struct gdi_font *font ) +{ + WORD format, offset, count; + GDEF_Header hdr; + UINT result; + size_t size; + void *data; + + result = font_funcs->get_font_data( font, MS_GDEF_TAG, 0, &hdr, sizeof(hdr) ); + if (result == GDI_ERROR) /* no GDEF table present */ + { + TRACE("No GDEF table found\n"); + return NULL; + } + + offset = GET_BE_WORD(hdr.GlyphClassDef); + if (!offset) + return NULL; + + result = font_funcs->get_font_data( font, MS_GDEF_TAG, offset, &format, sizeof(format) ); + format = GET_BE_WORD(format); + if (result == GDI_ERROR || format < 1 || format > 2) /* GDEF table invalid or too short */ + { + WARN("Invalid GDEF table\n"); + return NULL; + } + + if (format == 1) + { + result = font_funcs->get_font_data( font, MS_GDEF_TAG, offset + 4, &count, sizeof(count) ); + size = FIELD_OFFSET(OT_ClassDefFormat1, ClassValueArray[GET_BE_WORD(count)]); + } + else + { + result = font_funcs->get_font_data( font, MS_GDEF_TAG, offset + 2, &count, sizeof(count) ); + size = FIELD_OFFSET(OT_ClassDefFormat2, ClassRangeRecord[GET_BE_WORD(count)]); + } + if (result == GDI_ERROR) + return NULL; + + data = malloc( size ); + if (!data) + return NULL; + + result = font_funcs->get_font_data( font, MS_GDEF_TAG, offset, data, size ); + if (result != GDI_ERROR) + { + TRACE( "Loaded GDEF table of %zu bytes\n", size ); + return data; + } + else + free( data ); + return NULL; +} + +static WORD get_GDEF_glyph_class( struct gdi_font *font, WORD glyph ) +{ + OT_ClassDefFormat1 *cf1 = font->gdef_table; + WORD class = 0; + + if (!cf1) + return 0; + + if (cf1->ClassFormat == 1) + { + if (glyph >= GET_BE_WORD(cf1->StartGlyph)) + { + int index = glyph - GET_BE_WORD(cf1->StartGlyph); + if (index < GET_BE_WORD(cf1->GlyphCount)) + class = GET_BE_WORD(cf1->ClassValueArray[index]); + } + } + else + { + OT_ClassDefFormat2 *cf2 = (OT_ClassDefFormat2 *)cf1; + int i, top = GET_BE_WORD(cf2->ClassRangeCount); + for (i = 0; i < top; i++) + { + if (glyph >= GET_BE_WORD(cf2->ClassRangeRecord[i].Start) && + glyph <= GET_BE_WORD(cf2->ClassRangeRecord[i].End)) + { + class = GET_BE_WORD(cf2->ClassRangeRecord[i].Class); + break; + } + } + } + return class; +} + static void add_child_font( struct gdi_font *font, const WCHAR *family_name ) { FONTSIGNATURE fs = {{0}}; @@ -3936,16 +4057,22 @@ static DWORD get_glyph_outline( struct gdi_font *font, UINT glyph, UINT format, } if (format == GGO_METRICS && !mat && get_gdi_font_glyph_metrics( font, index, &gm, &abc )) + { + if (zero_width && (abc.abcA + abc.abcB + abc.abcC == 0)) + *zero_width = get_GDEF_glyph_class( font, index ); goto done; + } ret = font_funcs->get_glyph_outline( font, index, format, &gm, &abc, buflen, buf, mat, tategaki ); if (ret == GDI_ERROR) return ret; + if (zero_width && (abc.abcA + abc.abcB + abc.abcC == 0)) + *zero_width = get_GDEF_glyph_class( font, index ); + if (format == GGO_METRICS && !mat) set_gdi_font_glyph_metrics( font, index, &gm, &abc ); done: - if (zero_width && (abc.abcA + abc.abcB + abc.abcC) == 0) *zero_width = TRUE; if (gm_ret) *gm_ret = gm; if (abc_ret) *abc_ret = abc; return ret; @@ -4663,6 +4790,8 @@ static struct gdi_font *select_font( LOGFONTW *lf, FMAT2 dcmat, BOOL can_use_bit if (face->flags & ADDFONT_VERTICAL_FONT) /* We need to try to load the GSUB table */ font->vert_feature = get_GSUB_vert_feature( font ); + font->gdef_table = load_GDEF( font ); + create_child_font_list( font ); TRACE( "caching: gdiFont=%p\n", font ); diff --git a/dlls/win32u/ntgdi_private.h b/dlls/win32u/ntgdi_private.h index ad4c709b865..de0fa071817 100644 --- a/dlls/win32u/ntgdi_private.h +++ b/dlls/win32u/ntgdi_private.h @@ -288,6 +288,7 @@ struct gdi_font struct gdi_font *base_font; void *gsub_table; void *vert_feature; + void *gdef_table; void *data_ptr; SIZE_T data_size; FILETIME writetime; @@ -299,6 +300,7 @@ struct gdi_font #define MS_GASP_TAG MS_MAKE_TAG('g', 'a', 's', 'p') #define MS_GSUB_TAG MS_MAKE_TAG('G', 'S', 'U', 'B') +#define MS_GDEF_TAG MS_MAKE_TAG('G', 'D', 'E', 'F') #define MS_KERN_TAG MS_MAKE_TAG('k', 'e', 'r', 'n') #define MS_TTCF_TAG MS_MAKE_TAG('t', 't', 'c', 'f') #define MS_VDMX_TAG MS_MAKE_TAG('V', 'D', 'M', 'X') -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3876
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=138286 Your paranoid android. === debian11b (64 bit WoW report) === d3dx9_36: core.c:830: Test succeeded inside todo block: Got unexpected height 12.
Test failures are just the usual `test-linux-32` flakiness btw -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47652
Nikolay Sivov (@nsivov) commented about dlls/win32u/font.c:
+ else + { + OT_ClassDefFormat2 *cf2 = (OT_ClassDefFormat2 *)cf1; + int i, top = GET_BE_WORD(cf2->ClassRangeCount); + for (i = 0; i < top; i++) + { + if (glyph >= GET_BE_WORD(cf2->ClassRangeRecord[i].Start) && + glyph <= GET_BE_WORD(cf2->ClassRangeRecord[i].End)) + { + class = GET_BE_WORD(cf2->ClassRangeRecord[i].Class); + break; + } + } + } + return class; +} This does not do any validation regarding reading past the end of the table. Also format 2 should be using binary search, and not a loop.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47689
Nikolay Sivov (@nsivov) commented about dlls/win32u/font.c:
}
if (format == GGO_METRICS && !mat && get_gdi_font_glyph_metrics( font, index, &gm, &abc )) + { + if (zero_width && (abc.abcA + abc.abcB + abc.abcC == 0)) + *zero_width = get_GDEF_glyph_class( font, index ); goto done; + }
ret = font_funcs->get_glyph_outline( font, index, format, &gm, &abc, buflen, buf, mat, tategaki ); if (ret == GDI_ERROR) return ret;
+ if (zero_width && (abc.abcA + abc.abcB + abc.abcC == 0)) + *zero_width = get_GDEF_glyph_class( font, index ); +
Why would any non-zero class mean zero_width? Out of four currently defined classes only marks are documented as non-spacing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47690
Even if this works this way, glyph class test looks suspicious to me, I still think it's misplaced. This most likely should be a part of larger shaping handling, we need to figure out the extent of it in legacy shaping system of gdi32, and do something similar. For example. does GetTextExtentExPoint() work for Arabic text at all? A simple test would be to test lam-alef ligature with some font that's using a single glyph for it, and see what's returned for extents. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47691
On Thu Oct 5 13:11:00 2023 +0000, Nikolay Sivov wrote:
Even if this works this way, glyph class test looks suspicious to me, I still think it's misplaced. This most likely should be a part of larger shaping handling, we need to figure out the extent of it in legacy shaping system of gdi32, and do something similar. For example. does GetTextExtentExPoint() work for Arabic text at all? A simple test would be to test lam-alef ligature with some font that's using a single glyph for it, and see what's returned for extents. This was all reverse-engineered by listing which characters fit into a 1-pixel space after an "a" character according to `GetTextExtentExPoint` with the DejaVu font, on Windows and on Wine, and working towards getting Wine to match the Windows result. This is all just for fixing a game crash on Wine. I don't see why this should be blocked because it doesn't yet implement everything, it just makes Wine's implementation "less wrong/incomplete".
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47692
On Thu Oct 5 12:56:23 2023 +0000, Nikolay Sivov wrote:
Why would any non-zero class mean zero_width? Out of four currently defined classes only marks are documented as non-spacing. This was a bug introduced while I was refactoring, sorry, it should be `get_GDEF_glyph_class( font, index ) == MarkGlyph;`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47693
On Thu Oct 5 12:56:22 2023 +0000, Nikolay Sivov wrote:
This does not do any validation regarding reading past the end of the table. Also format 2 should be using binary search, and not a loop. I don't see how this could read past the end of the table, the size of the allocated memory is determined by `ClassRangeCount` in `load_GDEF` so all of this memory is allocated. I'm assuming that if the font section was too small, that `font_funcs->get_font_data` would return an error and thus `font->gdef_table` would be `NULL`, which is handled at the top of this function.
Regardless, this should use a bsearch, I think I did it this way because I was initially copying some code for parsing a different table which used a for loop. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47694
On Thu Oct 5 13:17:53 2023 +0000, Torge Matthies wrote:
I don't see how this could read past the end of the table, the size of the allocated memory is determined by `ClassRangeCount` in `load_GDEF` so all of this memory is allocated. I'm assuming that if the font section was too small, that `font_funcs->get_font_data` would return an error and thus `font->gdef_table` would be `NULL`, which is handled at the top of this function. Regardless, this should use a bsearch, I think I did it this way because I was initially copying some code for parsing a different table which used a for loop. Right, it's allocated, so probably fine. Ultimately it does not have to be allocated, if we map the font already.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47696
On Thu Oct 5 13:12:03 2023 +0000, Torge Matthies wrote:
This was a bug introduced while I was refactoring, sorry, it should be `get_GDEF_glyph_class( font, index ) == MarkGlyph;` If abc == 0 already, why do we need to set zero_width?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47697
On Thu Oct 5 13:29:21 2023 +0000, Nikolay Sivov wrote:
If abc == 0 already, why do we need to set zero_width? Huh, you're right. I'm pretty sure I added the check for `abc.abcA + abc.abcB + abc.abcC == 0` to fix a single mismatch between the listing of DejaVu zero-width characters on Wine and Windows, but now that I look at it again that makes no sense, the value of `*zero_width` makes no difference when `abc.abcA + abc.abcB + abc.abcC` are already zero.
I need to do some comparisons of the listings while looking at all characters instead of only the ones defined in the DejaVu font. That involves font substitution though, ugh... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47699
On Thu Oct 5 13:26:56 2023 +0000, Nikolay Sivov wrote:
Right, it's allocated, so probably fine. Ultimately it does not have to be allocated, if we map the font already. With the freetype backend we don't map the font into the address space. Freetype might, but that's an implementation detail if anything.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3876#note_47700
participants (4)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Torge Matthies
-
Torge Matthies (＠tmatthies)