[PATCH 0/1] MR7054: gdiplus: Use font linking only for missing glyphs.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57076 I don't know if this is how native does it, but it makes intuitive sense to me that if we have a glyph in the requested font, we should use it, and this fixes the bug in question. This should avoid the possibility of regression in cases where font linking isn't needed. There is a possibility of regression in corner cases handled better by the earlier version of font linking. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054
From: Esme Povirk <esme(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57076 --- dlls/gdiplus/graphics.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 4699f8c21d3..f1c5aa5631a 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -5359,29 +5359,47 @@ static void generate_font_link_info(struct gdip_format_string_info *info, DWORD HFONT map_hfont, hfont, old_font; LONG processed, progress = 0; struct gdip_font_link_section *section; - DWORD font_codepages, string_codepages; + DWORD string_codepages; + WORD *glyph_indices; list_init(&info->font_link_info.sections); info->font_link_info.base_font = base_font; + glyph_indices = calloc(length, sizeof(*glyph_indices)); + GetGlyphIndicesW(info->hdc, info->string, length, glyph_indices, GGI_MARK_NONEXISTING_GLYPHS); + + /* Newlines won't have a glyph but don't need a fallback */ + for (progress = 0; progress < length; progress++) + if (info->string[progress] == '\r' || info->string[progress] == '\n') + glyph_indices[progress] = 0; + GetGlobalFontLinkObject(&iMLFL); get_font_hfont(info->graphics, base_font, NULL, &hfont, NULL, NULL); - IMLangFontLink_GetFontCodePages(iMLFL, info->hdc, hfont, &font_codepages); + progress = 0; while (progress < length) { section = calloc(1, sizeof(*section)); section->start = progress; - IMLangFontLink_GetStrCodePages(iMLFL, &info->string[progress], length - progress, - font_codepages, &string_codepages, &processed); - if (font_codepages & string_codepages) + if (glyph_indices[progress] != 0xffff) { section->font = (GpFont *)base_font; + + processed = 0; + while (progress + processed < length && glyph_indices[progress + processed] != 0xffff) + processed++; } else { + int to_process = 0; + + while (progress + to_process < length && glyph_indices[progress + to_process] == 0xffff) + to_process++; + + IMLangFontLink_GetStrCodePages(iMLFL, &info->string[progress], to_process, + 0, &string_codepages, &processed); IMLangFontLink_MapFont(iMLFL, info->hdc, string_codepages, hfont, &map_hfont); old_font = SelectObject(info->hdc, map_hfont); GdipCreateFontFromDC(info->hdc, &gpfont); @@ -5397,6 +5415,7 @@ static void generate_font_link_info(struct gdip_format_string_info *info, DWORD DeleteObject(hfont); IMLangFontLink_Release(iMLFL); + free(glyph_indices); } static void font_link_get_text_extent_point(struct gdip_format_string_info *info, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7054
Nikolay Sivov (@nsivov) commented about dlls/gdiplus/graphics.c:
struct gdip_font_link_section *section; - DWORD font_codepages, string_codepages; + DWORD string_codepages; + WORD *glyph_indices;
list_init(&info->font_link_info.sections); info->font_link_info.base_font = base_font;
+ glyph_indices = calloc(length, sizeof(*glyph_indices)); + GetGlyphIndicesW(info->hdc, info->string, length, glyph_indices, GGI_MARK_NONEXISTING_GLYPHS); + + /* Newlines won't have a glyph but don't need a fallback */ + for (progress = 0; progress < length; progress++) + if (info->string[progress] == '\r' || info->string[progress] == '\n') + glyph_indices[progress] = 0; +
Handling this as a special case is most likely wrong, but if it works for now so be it. Regarding approach in general, yes, it seems preferable to use same font when it works, so sequences like "<latin1><japanese><latin2>" wouldn't use Japanese font for <latin2> range. I can only say that in directwrite it's handled differently - fallback is not going to be applied for all three ranges at once from this example, it will be applied per-range. Chromium layout does this differently, by nesting fallbacks to get more consistent output. I don't know what gdi+ is doing, but if you've tested this, it's good enough I think. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054#note_91033
On Sat Dec 21 22:36:08 2024 +0000, Nikolay Sivov wrote:
Handling this as a special case is most likely wrong, but if it works for now so be it. Regarding approach in general, yes, it seems preferable to use same font when it works, so sequences like "<latin1><japanese><latin2>" wouldn't use Japanese font for <latin2> range. I can only say that in directwrite it's handled differently - fallback is not going to be applied for all three ranges at once from this example, it will be applied per-range. Chromium layout does this differently, by nesting fallbacks to get more consistent output. I don't know what gdi+ is doing, but if you've tested this, it's good enough I think. The special case makes sense to me only because those characters are also handled in the actual drawing routine as a special case.
I have no idea how native does it (I can't think of any good way to test what fonts it's actually using), and no particular reason to believe it resembles this (other than knowing that the times it broke, it's been using fallbacks when it doesn't need to). I don't think native GDI+ does anything nearly as complex as dwrite, but that doesn't mean this way makes sense. Can you explain what you mean by per-range? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054#note_91035
On Sat Dec 21 23:11:27 2024 +0000, Esme Povirk wrote:
The special case makes sense to me only because those characters are also handled in the actual drawing routine as a special case. I have no idea how native does it (I can't think of any good way to test what fonts it's actually using), and no particular reason to believe it resembles this (other than knowing that the times it broke, it's been using fallbacks when it doesn't need to). I don't think native GDI+ does anything nearly as complex as dwrite, but that doesn't mean this way makes sense. Can you explain what you mean by per-range? By range I mean a segment of original text that has uniform properties, one of them being writing script property. In directwrite this is done by splitting up the text by the script (each unicode point belongs to some script, or is attached to preceding/following range), and then further by properties that user had applied to the text, such as preferred locale for example. This way we never do font mapping for text that contains characters from different scripts. I don't remember now if I had tested this properly, but it's certainly possible to do. We probably don't do such segmentation in gdi+.
For gdi+ this might be impossible to test reliably, because it does not let you control fallback behavior. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054#note_91036
On Sat Dec 21 23:25:51 2024 +0000, Nikolay Sivov wrote:
By range I mean a segment of original text that has uniform properties, one of them being writing script property. In directwrite this is done by splitting up the text by the script (each unicode point belongs to some script, or is attached to preceding/following range), and then further by properties that user had applied to the text, such as preferred locale for example. This way we never do font mapping for text that contains characters from different scripts. I don't remember now if I had tested this properly, but it's certainly possible to do. We probably don't do such segmentation in gdi+. For gdi+ this might be impossible to test reliably, because it does not let you control fallback behavior. One thing I hadn't considered before: space characters will most likely be available in the desired font. On long strings, this is likely to lead to a lot of font switching, which is probably not good for performance.
I'm not really sure when this will come up in practice and what our priorities will be, but in every known regressive case, we ended up using font linking when every character was available in the requested font. In those cases, everything was done right before and we should make sure they're handled without a fallback. In other cases, they never worked correctly before. With the priority being to avoid regressions, I think I should try to preserve established behavior of both cases as much as possible. I think that means going ahead and allowing font linking to be used, as before, for characters in the original font that follow characters that needed font linking. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054#note_91585
On Tue Jan 7 20:15:43 2025 +0000, Esme Povirk wrote:
One thing I hadn't considered before: space characters will most likely be available in the desired font. On long strings, this is likely to lead to a lot of font switching, which is probably not good for performance. I'm not really sure when this will come up in practice and what our priorities will be, but in every known regressive case, we ended up using font linking when every character was available in the requested font. In those cases, everything was done right before and we should make sure they're handled without a fallback. In other cases, they never worked correctly before. With the priority being to avoid regressions, I think I should try to preserve established behavior of both cases as much as possible. I think that means going ahead and allowing font linking to be used, as before, for characters in the original font that follow characters that needed font linking. For a regression fix this shouldn't wait for my opinion. I don't know how this should actually work.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7054#note_91586
participants (3)
-
Esme Povirk -
Esme Povirk (＠madewokherd)
-
Nikolay Sivov (＠nsivov)