[PATCH v2 0/4] MR4406: win32u: Fix reported text extent and actual text size mismatch.
Freetype's FT_Load_Glyph may return different glyph metrics (in particular, horiAdvance) depending on load target flags (FT_LOAD_TARGET_MONO, FT_LOAD_TARGET_NORMAL ...). Among the consequences of that are: - the size of, e. g, GetTextExtentPoint() doesn't match the size of actually rendered text; - DrawTextW() with DT_CALCRECT flag returns wrong bounding rectangle. In the core of that is GetGlyphOutline() returning different values for GGO_METRICS format (used in various glyph metrics query functions) and the actual format used during rendering. It probably make sense to use effective fonts rendering option for GGO_METRICS so that matches. I did some ad-hoc testing on Windows with currently problematic Tahoma font and quite expectedly GetGlyphOutline(GGO_METRICS) always returns the same metrics as other output format options. While all the options also have the same metrics between each other (which is still not the case with Wine). I guess it is not easily possible to make all the face load options match each other with freetype (nor I am sure that is always the case on Windows for all the possible fonts), but making GGO_METRICS return the metrics matching actual gdi device context setup looks more important. Fixes Idle Spiral being unable to render typed text in save / load dialogs (which is using Winforms from Unity's Mono). -- v2: win32u: Use font AA flags when querying glyph outline with GGO_METRICS. d3dx9/tests: Make test_ID3DXFont() less dependent on GetGlyphOutlineW(GGO_METRICS) behaviour. https://gitlab.winehq.org/wine/wine/-/merge_requests/4406
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/font.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index edeba90b981..ccd36517038 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -4671,6 +4671,7 @@ static HFONT font_SelectFont( PHYSDEV dev, HFONT hfont, UINT *aa_flags ) *aa_flags = font_smoothing; } *aa_flags = font_funcs->get_aa_flags( font, *aa_flags, antialias_fakes ); + font->aa_flags = *aa_flags; } TRACE( "%p %s %d aa %x\n", hfont, debugstr_w(lf.lfFaceName), (int)lf.lfHeight, *aa_flags ); pthread_mutex_unlock( &font_lock ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4406
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/freetype.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/dlls/win32u/freetype.c b/dlls/win32u/freetype.c index 6043c66bc53..90edeae670d 100644 --- a/dlls/win32u/freetype.c +++ b/dlls/win32u/freetype.c @@ -3401,10 +3401,13 @@ static unsigned int get_bezier_glyph_outline(FT_Outline *outline, unsigned int b return needed; } -static FT_Int get_load_flags( UINT format ) +static FT_Int get_load_flags( UINT format, BOOL vertical_metrics, BOOL force_no_bitmap ) { FT_Int load_flags = FT_LOAD_IGNORE_GLOBAL_ADVANCE_WIDTH; + if (vertical_metrics) load_flags |= FT_LOAD_VERTICAL_LAYOUT; + if (force_no_bitmap || format != GGO_BITMAP) load_flags |= FT_LOAD_NO_BITMAP; + if (format & GGO_UNHINTED) return load_flags | FT_LOAD_NO_HINTING; @@ -3444,7 +3447,7 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT FT_Glyph_Metrics metrics; FT_Error err; FT_BBox bbox; - FT_Int load_flags = get_load_flags(format); + FT_Int load_flags; FT_Matrix transform_matrices[3], *matrices = NULL; BOOL vertical_metrics; @@ -3454,8 +3457,6 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT font->matrix.eM11, font->matrix.eM12, font->matrix.eM21, font->matrix.eM22); - format &= ~GGO_UNHINTED; - matrices = get_transform_matrices( font, tategaki, lpmat, transform_matrices ); vertical_metrics = (tategaki && FT_HAS_VERTICAL(ft_face)); @@ -3463,9 +3464,7 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT properly scaled and correct in 2.4.0 or greater */ if (vertical_metrics && FT_SimpleVersion < FT_VERSION_VALUE(2, 4, 0)) vertical_metrics = FALSE; - - if (matrices || format != GGO_BITMAP) load_flags |= FT_LOAD_NO_BITMAP; - if (vertical_metrics) load_flags |= FT_LOAD_VERTICAL_LAYOUT; + load_flags = get_load_flags(format, vertical_metrics, !!matrices); err = pFT_Load_Glyph(ft_face, glyph, load_flags); if (err && !(load_flags & FT_LOAD_NO_HINTING)) @@ -3480,6 +3479,8 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT return GDI_ERROR; } + format &= ~GGO_UNHINTED; + metrics = ft_face->glyph->metrics; if(font->fake_bold) { if (!get_bold_glyph_outline(ft_face->glyph, font->ppem, &metrics) && metrics.width) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4406
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/d3dx9_36/tests/core.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/dlls/d3dx9_36/tests/core.c b/dlls/d3dx9_36/tests/core.c index c3b80028f43..5b07fcd5ef7 100644 --- a/dlls/d3dx9_36/tests/core.c +++ b/dlls/d3dx9_36/tests/core.c @@ -356,7 +356,7 @@ static void test_ID3DXFont(IDirect3DDevice9 *device) IDirect3DTexture9 *texture; D3DSURFACE_DESC surf_desc; IDirect3DDevice9 *bufdev; - GLYPHMETRICS glyph_metrics; + GLYPHMETRICS glyph_metrics, gm_grayscale; D3DXFONT_DESCA desc; ID3DXSprite *sprite; RECT rect, blackbox; @@ -605,15 +605,27 @@ static void test_ID3DXFont(IDirect3DDevice9 *device) count = GetGlyphOutlineW(hdc, glyph, GGO_GLYPH_INDEX | GGO_METRICS, &glyph_metrics, 0, NULL, &mat); ok(count != GDI_ERROR, "Unexpected count %#lx.\n", count); + count = GetGlyphOutlineW(hdc, glyph, GGO_GLYPH_INDEX | GGO_GRAY8_BITMAP, &gm_grayscale, 0, NULL, &mat); + ok(count != GDI_ERROR, "Unexpected count %#lx.\n", count); + ret = ID3DXFont_GetTextMetricsW(font, &tm); ok(ret, "Unexpected ret %#x.\n", ret); - todo_wine ok(blackbox.right - blackbox.left == glyph_metrics.gmBlackBoxX + 2, "Got %ld, expected %d.\n", + todo_wine_if(blackbox.right - blackbox.left < glyph_metrics.gmBlackBoxX + 2) + ok(blackbox.right - blackbox.left == glyph_metrics.gmBlackBoxX + 2, "Got %ld, expected %d.\n", blackbox.right - blackbox.left, glyph_metrics.gmBlackBoxX + 2); - todo_wine ok(blackbox.bottom - blackbox.top == glyph_metrics.gmBlackBoxY + 2, "Got %ld, expected %d.\n", + + todo_wine_if(blackbox.bottom - blackbox.top < glyph_metrics.gmBlackBoxY + 2) + ok(blackbox.bottom - blackbox.top == glyph_metrics.gmBlackBoxY + 2, "Got %ld, expected %d.\n", blackbox.bottom - blackbox.top, glyph_metrics.gmBlackBoxY + 2); + + todo_wine_if(glyph_metrics.gmptGlyphOrigin.x != gm_grayscale.gmptGlyphOrigin.x + && cellinc.x == gm_grayscale.gmptGlyphOrigin.x - 1) ok(cellinc.x == glyph_metrics.gmptGlyphOrigin.x - 1, "Got %ld, expected %ld.\n", cellinc.x, glyph_metrics.gmptGlyphOrigin.x - 1); + + todo_wine_if(glyph_metrics.gmptGlyphOrigin.y != gm_grayscale.gmptGlyphOrigin.y + && cellinc.y == tm.tmAscent - gm_grayscale.gmptGlyphOrigin.y - 1) ok(cellinc.y == tm.tmAscent - glyph_metrics.gmptGlyphOrigin.y - 1, "Got %ld, expected %ld.\n", cellinc.y, tm.tmAscent - glyph_metrics.gmptGlyphOrigin.y - 1); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4406
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/freetype.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/freetype.c b/dlls/win32u/freetype.c index 90edeae670d..dcc90e728be 100644 --- a/dlls/win32u/freetype.c +++ b/dlls/win32u/freetype.c @@ -3450,6 +3450,7 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT FT_Int load_flags; FT_Matrix transform_matrices[3], *matrices = NULL; BOOL vertical_metrics; + UINT effective_format = format; TRACE("%p, %04x, %08x, %p, %08x, %p, %p\n", font, glyph, format, lpgm, buflen, buf, lpmat); @@ -3459,14 +3460,22 @@ static UINT freetype_get_glyph_outline( struct gdi_font *font, UINT glyph, UINT matrices = get_transform_matrices( font, tategaki, lpmat, transform_matrices ); + if ((format & ~GGO_GLYPH_INDEX) == GGO_METRICS) + effective_format = font->aa_flags | (format & GGO_GLYPH_INDEX); vertical_metrics = (tategaki && FT_HAS_VERTICAL(ft_face)); /* there is a freetype bug where vertical metrics are only properly scaled and correct in 2.4.0 or greater */ if (vertical_metrics && FT_SimpleVersion < FT_VERSION_VALUE(2, 4, 0)) vertical_metrics = FALSE; - load_flags = get_load_flags(format, vertical_metrics, !!matrices); + load_flags = get_load_flags(effective_format, vertical_metrics, !!matrices); err = pFT_Load_Glyph(ft_face, glyph, load_flags); + if (err && format != effective_format) + { + WARN("Failed to load glyph %#x, retrying with GGO_METRICS. Error %#x.\n", glyph, err); + load_flags = get_load_flags(effective_format, vertical_metrics, !!matrices); + err = pFT_Load_Glyph(ft_face, glyph, load_flags); + } if (err && !(load_flags & FT_LOAD_NO_HINTING)) { WARN("Failed to load glyph %#x, retrying without hinting. Error %#x.\n", glyph, err); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4406
v2: - added patch "d3dx9/tests: Make test_ID3DXFont() less dependent on GetGlyphOutlineW(GGO_METRICS) behaviour." to fix the test failures introduced with v1. The patchset actually changes nothing in ID3DXFont behaviour, the changes in test due to changes in GetGlyphOutlineW(GGO_METRICS) behaviour (which is used in test but not implementation). I did some investigation around this and I think the test breakage doesn't indicate that the MR is wrong but rather reflects some differences d3dx9 font implementation compared to Windows. First of all, all the same tests can be easily broken on Windows by using different font (e. g., Arial instead of Tahoma) and / or antialising quality in font, so the conditions which concerned tests assert are not generally satisified on Windows. The major difference in Windows implementation (which I think affects things as most here) is that Windows minds all the font antialiasing parameters (the same way, e. g., as if the text would be drawn with user32.DrawText). I displayed a test string rendered with ID3DXFont on screen through d3d9 presentation and I see Windows following quality parameter (e. g., drawing aliased with NONANTIALIASED_QUALITY). Behaviour in DRAFT_QUALITY is controlled by system parameter ("Performance Options / Smooth Edges of Screen Fonts" in SystemPropertiesPerformance.exe tool on Win10), changing this parameter affects whether antialiasing is present in ID3DXFont rendering. Wine d3dx9 currently always using antialiasing and requests glyphs with GGO_GRAY8_BITMAP, which is the major source of mismatch in these tests. If we want to make this behaviour closer match Windows me might probably use something like ExtTextOut() for rendering font instead of directly using glyph bitmap. But in that case this MR is prerequisite, the rendered image won't match the queried metrics without it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4406#note_52629
On Tue Nov 21 08:13:16 2023 +0000, Paul Gofman wrote:
v2: - added patch "d3dx9/tests: Make test_ID3DXFont() less dependent on GetGlyphOutlineW(GGO_METRICS) behaviour." to fix the test failures introduced with v1. The patchset actually changes nothing in ID3DXFont behaviour, the changes in test due to changes in GetGlyphOutlineW(GGO_METRICS) behaviour (which is used in test but not implementation). I did some investigation around this and I think the test breakage doesn't indicate that the MR is wrong but rather reflects some differences d3dx9 font implementation compared to Windows. First of all, all the same tests can be easily broken on Windows by using different font (e. g., Arial instead of Tahoma) and / or antialising quality in font, so the conditions which concerned tests assert are not generally satisified on Windows. The major difference in Windows implementation (which I think affects things as most here) is that Windows minds all the font antialiasing parameters (the same way, e. g., as if the text would be drawn with user32.DrawText). I displayed a test string rendered with ID3DXFont on screen through d3d9 presentation and I see Windows following quality parameter (e. g., drawing aliased with NONANTIALIASED_QUALITY). Behaviour in DRAFT_QUALITY is controlled by system parameter ("Performance Options / Smooth Edges of Screen Fonts" in SystemPropertiesPerformance.exe tool on Win10), changing this parameter affects whether antialiasing is present in ID3DXFont rendering. Wine d3dx9 currently always using antialiasing and requests glyphs with GGO_GRAY8_BITMAP, which is the major source of mismatch in these tests. If we want to make this behaviour closer match Windows me might probably use something like ExtTextOut() for rendering font instead of directly using glyph bitmap. But in that case this MR is prerequisite, the rendered image won't match the queried metrics without it. Font rendering isn't exactly my specialty so I want to be sure I understand the issue here. BTW, you don't need to go and find an answer to all my questions, I'm just trying to dump your brain WRT what you already found out :sweat:
In particular, you mention font quality affecting d3dx9 output with native but not with our implementation. How does that work exactly? Notice that we pass the quality setting over to CreateFontW(). Is that not enough? Is GGO_GRAY8_BITMAP just not right for non-antialiased fonts? Or is it a case where things don't work properly unless you draw the whole string at the same time? The DRAFT_QUALITY thing seems mostly orthogonal to d3dx9, as I understand it. I expect these tests to generally have room for improvement. From what I understand from your results, right now they're effectively passing by chance. So, while adding a few todo_wine to keep the tests passing might be okay for the time being, I'd prefer to fix the tests (which might even mean getting rid of a few of them) or at least figure out what needs to be fixed. It would be unfortunate to mark some test as todo_wine, implying that the implementation is broken, when in fact it's the test that's "too strict", for some definition of it. Looking at the whole MR, would it make sense to pass GGO_GRAY8_BITMAP to the GetGlyphOutlineW() calls in the d3dx9 tests? Or is that how you found out that the implementation is not correct in that regard? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4406#note_53022
On Tue Nov 21 08:13:16 2023 +0000, Matteo Bruni wrote:
Font rendering isn't exactly my specialty so I want to be sure I understand the issue here. BTW, you don't need to go and find an answer to all my questions, I'm just trying to dump your brain WRT what you already found out :sweat: In particular, you mention font quality affecting d3dx9 output with native but not with our implementation. How does that work exactly? Notice that we pass the quality setting over to CreateFontW(). Is that not enough? Is GGO_GRAY8_BITMAP just not right for non-antialiased fonts? Or is it a case where things don't work properly unless you draw the whole string at the same time? The DRAFT_QUALITY thing seems mostly orthogonal to d3dx9, as I understand it. I expect these tests to generally have room for improvement. From what I understand from your results, right now they're effectively passing by chance. So, while adding a few todo_wine to keep the tests passing might be okay for the time being, I'd prefer to fix the tests (which might even mean getting rid of a few of them) or at least figure out what needs to be fixed. It would be unfortunate to mark some test as todo_wine, implying that the implementation is broken, when in fact it's the test that's "too strict", for some definition of it. Looking at the whole MR, would it make sense to pass GGO_GRAY8_BITMAP to the GetGlyphOutlineW() calls in the d3dx9 tests? Or is that how you found out that the implementation is not correct in that regard? WRT font quality (essentially, antialiasing). How does that work in gdi (at least Wine, but based on what I tested on Windows on various occasion it is functionally similar, apart from some bugs). CreateFontW() does nothing, it merely stores LOGFONT structure under handle. Something happens when you SelectFont() into hdc (gdi device context). Here the antialias settings are actually guessed. A few thing affects that: font quality settings in LOGFONT ( CreateFont parameter), font own settings, preferences coming from device under device context. If that is screen compatible device the default option come from system settings (specifically, for DRAFT_QUALITY; both on Windows and in Wine too, at least on x11, it is affected by desktop settings). All that (rather convoluted) process ends up in antialias settings to be decided in hdc, but nothing else happens yet. What this hdc settings affect are user32 / gdi32 font rendering functions, like DrawText, ExtTextOut will effectively use the antialiasing in hdc. d3dx9 currently uses explicit GetGlyphOutlineW(GGO_GRAY8_BITMAP). This is different. I don't know a way to get glyph bitmap with "hdc selected font default" from it, whenever you request glyph data format stipulates the antialiasing (and metrics). So GetGlyphOutlineW(GGO_GRAY8_BITMAP) is going to return antialiased glyph (at least if font supports that at all) regardless of any settings (including hints to CreateFont). GetGlyphOutlineW(GGO_MEASURE) is special here, it result may depends on "font in hdc" settings but it doesn't return the glyph. I also didn't find a documented way to extract font antialiasing info from hdc. So to handle d3dx9 font antialiasing the same way as on Windows the only straightforward way I see is to stop using glyph data directly at all and use user32 or gdi32 function to draw glyph instead. I tested that d3dx9 glyph drawing on Windows follows gdi / hdc rules. I can attach an ad-hoc test which displays the letters on screen from d3dx9 rendering if that helps.
Looking at the whole MR, would it make sense to pass GGO_GRAY8_BITMAP to the GetGlyphOutlineW() calls in the d3dx9 tests? Or is that how you found out that the implementation is not correct in that regard?
That doesn't succeed on Windows. WRT the tests, yes, I think they are passing by chance on Windows (on Wine not quite as cellinc calculation is identical to the test). E. g., changing font from "Tahoma" to "Arial" will make concerned tests fail for some characters. More so when changing font antialiasing settings. I spent some time trying to get more out of this test, but this is not straightforward, from those attempts I concluded that relation of cellinc to gmptGlyphOrigin is a bit off in general (across various fonts and antialiasing settings the error is more than 1 pixel on Windows). In the present test with "Tahoma" gmptGlyphOrigin.x is in fact always 0. If we want to match that we probably need to try to re-explore how that d3dx9 font metrics are estimated across the different fonts, and how that can be linked to glyph metrics. I am not entirely sure if that worth it as exact matching Windows accuracy is also not achieved in gdi now, and it might be very hard / impractical to do, if even possible. I realize that such changes to the test (when the test itself doesn't reflect what it is supposed to) is a bit weird. Maybe we would rather remove these specific tests? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4406#note_53109
On Tue Nov 21 15:46:30 2023 +0000, Paul Gofman wrote:
WRT font quality (essentially, antialiasing). How does that work in gdi (at least Wine, but based on what I tested on Windows on various occasion it is functionally similar, apart from some bugs). CreateFontW() does nothing, it merely stores LOGFONT structure under handle. Something happens when you SelectFont() into hdc (gdi device context). Here the antialias settings are actually guessed. A few thing affects that: font quality settings in LOGFONT ( CreateFont parameter), font own settings, preferences coming from device under device context. If that is screen compatible device the default option come from system settings (specifically, for DRAFT_QUALITY; both on Windows and in Wine too, at least on x11, it is affected by desktop settings). All that (rather convoluted) process ends up in antialias settings to be decided in hdc, but nothing else happens yet. What this hdc settings affect are user32 / gdi32 font rendering functions, like DrawText, ExtTextOut will effectively use the antialiasing in hdc. d3dx9 currently uses explicit GetGlyphOutlineW(GGO_GRAY8_BITMAP). This is different. I don't know a way to get glyph bitmap with "hdc selected font default" from it, whenever you request glyph data format stipulates the antialiasing (and metrics). So GetGlyphOutlineW(GGO_GRAY8_BITMAP) is going to return antialiased glyph (at least if font supports that at all) regardless of any settings (including hints to CreateFont). GetGlyphOutlineW(GGO_MEASURE) is special here, it result may depends on "font in hdc" settings but it doesn't return the glyph. I also didn't find a documented way to extract font antialiasing info from hdc. So to handle d3dx9 font antialiasing the same way as on Windows the only straightforward way I see is to stop using glyph data directly at all and use user32 or gdi32 function to draw glyph instead. I tested that d3dx9 glyph drawing on Windows follows gdi / hdc rules. I can attach an ad-hoc test which displays the letters on screen from d3dx9 rendering if that helps.
Looking at the whole MR, would it make sense to pass GGO_GRAY8_BITMAP to the GetGlyphOutlineW() calls in the d3dx9 tests? Or is that how you found out that the implementation is not correct in that regard? That doesn't succeed on Windows. WRT the tests, yes, I think they are passing by chance on Windows (on Wine not quite as cellinc calculation is identical to the test). E. g., changing font from "Tahoma" to "Arial" will make concerned tests fail for some characters. More so when changing font antialiasing settings. I spent some time trying to get more out of this test, but this is not straightforward, from those attempts I concluded that relation of cellinc to gmptGlyphOrigin is a bit off in general (across various fonts and antialiasing settings the error is more than 1 pixel on Windows). In the present test with "Tahoma" gmptGlyphOrigin.x is in fact always 0. If we want to match that we probably need to try to re-explore how that d3dx9 font metrics are estimated across the different fonts, and how that can be linked to glyph metrics. I am not entirely sure if that worth it as exact matching Windows accuracy is also not achieved in gdi now, and it might be very hard / impractical to do, if even possible. I realize that such changes to the test (when the test itself doesn't reflect what it is supposed to) is a bit weird. Maybe we would rather remove these specific tests? Thanks a lot Paul, that was very clear.
Pragmatically, I think that removing these questionable tests is preferable to adding todo_wines, but I'd approve this MR in its current form (waiting until the rest is reviewed before doing that either way, though). It would be great if you could, as a follow up, come up with some d3dx9 test showing the fundamental issue with our current implementation you just explained. Maybe it could check the actual glyph texture data for the presence of any half-tone pixels when forcing non-antialiased text, or maybe there's something easier. No particular hurry of course, but it would make sure we (or at least I :grin:) don't forget about the shortcomings of our implementation in a week... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4406#note_53120
participants (3)
-
Matteo Bruni (＠Mystral) -
Paul Gofman
-
Paul Gofman (＠gofman)