1918864 - Update mingw to upstream revision c0313ec3381521db6c5e3aca746ff1e3e29208d7

Status: RESOLVED FIXED

(Firefox Build System :: General, task, P3)

Description

[Ryan VanderMeulen [:RyanVM]]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=781ff7cd39330cd93252d47c1eed17d37a0c9d5e

Looks like we're hitting bustage from upstream commit 4ef04b0a7f7a20735de2f58b5c0496fcb4c7d191, however.
https://treeherder.mozilla.org/logviewer?job_id=474335119&repo=try&lineNumber=106877

 INFO -  In file included from /builds/worker/checkouts/gecko/gfx/angle/checkout/src/libANGLE/renderer/d3d/d3d11/Renderer11.cpp:75:
 INFO -  In file included from /builds/worker/checkouts/gecko/gfx/angle/checkout/src/libANGLE/renderer/d3d/d3d11/converged/CompositorNativeWindow11.h:18:
 INFO -  In file included from /builds/worker/fetches/clang/bin/../x86_64-w64-mingw32/include/windows.ui.composition.h:1380:
ERROR -  /builds/worker/fetches/clang/bin/../x86_64-w64-mingw32/include/windows.ui.core.h:1605:34: error: field has incomplete type 'struct Point'
 INFO -   1605 |                     struct Point AdjustedPoint;
 INFO -        |                                  ^
 INFO -  /builds/worker/fetches/clang/bin/../x86_64-w64-mingw32/include/windows.ui.core.h:1605:28: note: forward declaration of 'ABI::Windows::UI::Core::Point'
 INFO -   1605 |                     struct Point AdjustedPoint;
 INFO -        |                            ^
ERROR -  /builds/worker/fetches/clang/bin/../x86_64-w64-mingw32/include/windows.ui.core.h:2762:30: error: ISO C++ forbids forward references to 'enum' types
 INFO -   2762 |                         enum VirtualKey key,
 INFO -        |                              ^
ERROR -  /builds/worker/fetches/clang/bin/../x86_64-w64-mingw32/include/windows.ui.core.h:4879:30: error: ISO C++ forbids forward references to 'enum' types
 INFO -   4879 |                         enum VirtualKeyModifiers *value) = 0;
 INFO -        |                              ^
 INFO -  3 errors generated.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1918864 - Update mingw to upstream revision c0313ec3381521db6c5e3aca746ff1e3e29208d7.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Jacek reached out on Matrix that the failures from #c0 were fixed upstream. On a fresh Try push, we're seeing these failures now:
https://treeherder.mozilla.org/logviewer?job_id=474564412&repo=try&lineNumber=138097

ERROR -  /builds/worker/checkouts/gecko/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp:156:25: error: member access into incomplete type 'IDWriteFontFace7'
 INFO -    156 |         fDWriteFontFace7->Release();
 INFO -        |                         ^
 INFO -  /builds/worker/checkouts/gecko/gfx/skia/skia/src/ports/SkTypeface_win_dw.h:40:11: note: forward declaration of 'IDWriteFontFace7'
 INFO -     40 | interface IDWriteFontFace7;
 INFO -        |           ^
 INFO -  1 error generated.

Comment 3

[Biswapriyo Nath [:biswa96]]

The errors in windows.ui.core.h were fixed in upstream wine project https://bugs.winehq.org/show_bug.cgi?id=55347. Those changes need to be imported in mingw-w64.

For the IDWriteFontFace7 related issues, would it be possible to disable that for mingw-w64? Another related interface declaration has inline overload function which is not supported in widl, AFAIK.

Comment 4

[Biswapriyo Nath [:biswa96]]

I have sent a patch to add IDWriteFontFace7 in wine project https://gitlab.winehq.org/wine/wine/-/merge_requests/6578. It needs some modification in skia code due to absence of default parameter value in IDL.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:ahochheiden, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Making progress with current tip, but still more bustage.
https://treeherder.mozilla.org/logviewer?job_id=476500207&repo=try&lineNumber=37050

Comment 7

[Biswapriyo Nath [:biswa96]]

Please try to remove the mingw condition in accessible/windows/uia/uiaRawElmProvider.cpp file. Those are added in upstream.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

That fixes the uiaRawElmProvider.cpp build failure, yeah. The failures in the cairo and skia code remain.
https://treeherder.mozilla.org/logviewer?job_id=476539238&repo=try&lineNumber=134905

Comment 9

[Biswapriyo Nath [:biswa96]]

The DEFINE_ENUM_FLAG_OPERATORS(DWRITE_GLYPH_IMAGE_FORMATS); line should be removed from gfx/cairo/cairo/src/win32/dw-extra.h. The skia failure need some changes either in mingw-w64 header or in skia. I can not decide which one and need someone to review it, jackec maybe.

Comment 10

[Biswapriyo Nath [:biswa96]]

Attachment: mingw-dwrite_3.patch

Comment 11

[Biswapriyo Nath [:biswa96]]

Please try with this new patch file in taskcluster/scripts/misc/mingw-dwrite_3.patch

Comment 12

[Biswapriyo Nath [:biswa96]]

Comment on attachment 9428562 [details] [diff] [review]
mingw-dwrite_3.patch

New mingw-dwrite_3.patch with required changes for skia.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Attachment: WIP: Bug 1918864 - Remove obsolete line from dw-extra.h.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

On the bright side, we have working builds now with the mingw-dwrite_3 patch and the cairo change from comment 9. Unfortunately, there's COLRv1 font reftest failures:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/fe3N_MFjQ_SodHxFs9oCuw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 15

[Tom Ritter [:tjr]]

Just as an update, I've been digging into the reftest failure, and have come up with the following, which I'll also be sharing in #mingw-w64

We have some tests that render something, and then compare it to a known-good image, to confirm nothing has broken in our rendering. While doing a regular MinGW version bump to tip, it triggered on of the reftests and started failing it. Note that the failure is only on x64

The reftest is here: https://ritter.vg/misc/ff/mingw-reftest-fail.html#E (note that a single letter after the # is required)

If you visit this in Firefox you'll see a Green to Blue gradient.

We bumped MinGW to version 335ffe5b71d019c9ac5af29961fd872064ecb25c which is a big widl header update. As part of that update, the following code was changed from

/* Choose NTDDI Version */
#ifndef NTDDI_VERSION
#ifdef _WIN32_WINNT
#define NTDDI_VERSION NTDDI_VERSION_FROM_WIN32_WINNT(_WIN32_WINNT)
#else
#define NTDDI_VERSION NTDDI_WS03
#endif
#endif

to

/* Choose NTDDI Version */
#ifndef NTDDI_VERSION
# ifdef _WIN32_WINNT
#  if _WIN32_WINNT < _WIN32_WINNT_WIN10
/* For versions before Windows 10, set the corresponding NTDDI_VERSION. */
#   define NTDDI_VERSION NTDDI_VERSION_FROM_WIN32_WINNT(_WIN32_WINNT)
#  else
/* As _WIN32_WINNT doesn't distinguish between versions of Windows 10/11,
 * set NTDDI_VERSION to the highest version. */
#   define NTDDI_VERSION WDK_NTDDI_VERSION
#  endif
# else
#  define NTDDI_VERSION NTDDI_WS03
# endif
#endif

functionally upgrading the NTDDI_VERSION macro. Doing this resulted in a bunch of code in Firefox to suddently become active, code that was gated by NTDDI_VERSION being a lower version.
This code is in

• https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkScalerContext_win_dw.cpp#444-1486
• https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp - 7 places of shorter #if blocks

If we compile Firefox with MinGW on version 335ffe5b71d019c9ac5af29961fd872064ecb25c using the new value of NTDDI_VERSION - the reftest starts failing. It renders as just a red block.
I can't promise how long this link will be active, but here is an x64 debug build of Firefox exhibiting this behavior:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/KJIcZW3iQEyN4m2-7be0FA/runs/0/artifacts/public/build/target.zip
From https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=KJIcZW3iQEyN4m2-7be0FA.0&revision=76911821aad97748c17ab8ccd517fd9c88703024

If I compile Firefox with MinGW on version 335ffe5b71d019c9ac5af29961fd872064ecb25c but I patch back the old "Choose NTDDI_VERSION Version" block, the reftest passes.
Build: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/IxLYBWPfS5iNYY8USGJvHQ/runs/0/artifacts/public/build/target.zip
From https://treeherder.mozilla.org/jobs?repo=try&revision=ffa25d4e8779f59cc490b09fb04dcd069c72858d

My assumption if that something in the newly activated blocks of Firefox code is using a MinGW-defined function that is not used elsewhere (probably) and that MinGW definition has a subtle error relative to the Microsoft headers.

Skimming the code I would start with

• DWRITE_COLOR_F - an alias for D3DCOLORVALUE which is defined multiple times and most of the times looks fine, but this one is weird? https://searchfox.org/mingw/source/mingw-w64-headers/include/dwrite_2.h#138-156
• DWRITE_COLOR_COMPOSITE_MODE - checked
• DWRITE_PAINT_ELEMENT - seems correct, but it is a complex union of structs. Maybe a padding issue?
• DWRITE_PAINT_TYPE_* - checked
• DWRITE_GLYPH_IMAGE_FORMATS_COLR_PAINT_TREE - checked
• DWRITE_PAINT_FEATURE_LEVEL_COLR_V1 - checked
• GetGlyphRunOutline - checked
• IDWriteFontFace7
• IDWritePaintReader
• CreatePaintReader
• DWRITE_PAINT_ATTRIBUTES
• DWRITE_FONT_AXIS_ATTRIBUTES_VARIABLE

And then extend to

• D2D1_GRADIENT_STOP
• D2D1_COLOR_F
• D2D1_EXTEND_MODE
• IDWriteGeometrySink
• IDWriteFontFace5
• IDWriteFontResource
• DWRITE_FONT_AXIS_VALUE

Comment 16

[Biswapriyo Nath [:biswa96]]

Could you provide the path to the source file or directory for the failed tests?

Comment 17

[Tom Ritter [:tjr]]

(In reply to Biswapriyo Nath [:biswa96] from comment #16)

Could you provide the path to the source file or directory for the failed tests?

It's failing this test: https://searchfox.org/mozilla-central/source/layout/reftests/font-face/colrv1-01.html - which is reproduced at https://ritter.vg/misc/ff/mingw-reftest-fail.html#E (the #E or some other letter is required.)

Comment 18

[Jonathan Kew [:jfkthame]]

We handle COLR font rendering within Gecko in a platform-independent way. I suspect that what may be happening here is that the Skia/DirectWrite code to render COLRv1 actually conflicts with how we render the fonts, and we need to explicitly turn off that support so that it just does standard monochrome rasterization of the glyphs we ask for.

I note that in my local (non-mingw) Windows build, the #if-guarded block at https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkScalerContext_win_dw.cpp#444-1486 is not enabled. Though if I manually enable it and rebuild, the test still passes (I get the expected gradient, not just a red square), so something further is going on.

Tom, do you have (or could you re-create) a new debug build? I was going to poke at it a bit, but the one

I can't promise how long this link will be active, but here is an x64 debug build of Firefox exhibiting this behavior:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/KJIcZW3iQEyN4m2-7be0FA/runs/0/artifacts/public/build/target.zip
from comment 15 has (unsurprisingly) expired.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Here's a more recent Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=28505f64e36aa18a28b356592c1659759dd23d5f&group_state=expanded

Comment 20

[Jonathan Kew [:jfkthame]]

That build renders the color glyphs fine for me, though I see from treeherder that it's failing there. Probably because I'm on Win10 (not 11), and some runtime version/API-availability check is choosing a different code path.

My suspicion is that the problem arises when Skia decides to use "native" Windows COLR font rendering, and that doesn't integrate well with Gecko. As long as it's compiled with older headers or running on an older Windows version, it doesn't take that path, and our in-house rendering works fine.

Can someone on Win11 confirm whether the code here successfully gets the fDWriteFontFace7 interface? If you comment out that code (so that fDWriteFontFace7 simply remains null, as if QueryInterface had failed), does that affect the rendering?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I can confirm that the standalone test case from comment 17 shows the red square on Win11 and the gradient on Win10 with the MinGW Try build from comment 19, as expected/speculated.

I also ran a Try push based on comment 20 (https://treeherder.mozilla.org/jobs?repo=try&revision=a5fd7409de11f77df57d4b54837ff4046d281ba4) and it shows the gradient on both Win10 and Win11. LMK if I misunderstood the ask, though!

Comment 22

[Tom Ritter [:tjr]]

Oh, interesting. So it sounds like this isn't a MinGW bug at all, it's either a bug in Skia or a bug in something we do when we use that bit of the code (which we only do for MinGW on Windows 11.) What is the best path forward here? Ensure MinGW doesn't wind up doing that? Or try to fix the bug so it's not a future tech debt we have to fix at an inopportune time?

Comment 23

[Jonathan Kew [:jfkthame]]

OK, so it seems pretty clear that the problem arises when Skia decides to enable DirectWrite COLR rendering.

Further confirmation of that could be found by doing a Visual Studio-based (non-MinGW) build with whatever SDK versions are required to activate this same codepath. (In my local build on Windows, the #ifs here and similar places are false, so all that code is simply omitted; and even if I manually enable that code -- which compiles without complaint -- the QueryInterface(&fDWriteFontFace7) call fails at runtime.)

I'm assuming that a build where this code is present, run on Win11, will exhibit the same problem even without MinGW being involved. If so, that suggests that our standard builds will start failing on Win11 at some point when we update toolchains/SDKs/etc.

There are basically two possible ways forward:

(a) Create a (compile-time? run-time?) option to disable Skia's use of "native" DirectWrite COLR rendering. That would be pretty simple (and is arguably a configuration option that would make sense for Skia to offer anyway -- though I don't know whether upstream would accept it).

(b) Update thebes and webrender to use DirectWrite's color-glyph rendering. This seems like probably a significantly bigger chunk of work, and would mean we'd be maintaining two COLR-rendering architectures (for the time being, until we're ready to assume native support is available ot all users).

(Somewhat similar issues apply on Linux, fwiw: current FreeType now includes COLR-font support, but we don't use this; we handle all the COLR work in thebes, and just use FreeType to rasterize monochrome glyph components.)

My vote would be to do (a), at least for the time being: patch Skia to forcibly disable this code. Eventually we might want to move to (b), but for now maintaining a single, cross-platform implementation of COLR font rendering that depends only on standard monochrome glyph rasterizers seems the more attractive option.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Well, today is our lucky day! I just so happen to do my local builds with the Win11 26100 SDK and sure enough, I get a red rectangle when I run the testcase on top of that build on my Win11 laptop.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Interestingly, our CI builds use the Win11 22621 SDK without issue. So it's only the very latest Windows SDK that appears to trigger the bug. I wonder if I can hack together a Windows build using the vs2022-car toolchain instead and see if the same reftest failures strike.

EDIT: Yes, yes I can.
https://treeherder.mozilla.org/jobs?repo=try&revision=570824da82326b5f7f93685f643217a8b6491833

Not sure the deal is with the reftest timeouts, but I can at least confirm that the builds from that push show the same behavioral difference between Win10 and Win11 with the isolated testcase.

Comment 26

[Tom Ritter [:tjr]]

(In reply to Jonathan Kew [:jfkthame] from comment #23)

My vote would be to do (a), at least for the time being: patch Skia to forcibly disable this code. Eventually we might want to move to (b), but for now maintaining a single, cross-platform implementation of COLR font rendering that depends only on standard monochrome glyph rasterizers seems the more attractive option.

That sounds fine to me! Is this something that you think you can do in the short term (before the holidays)? If not, I can try and guess at the best way to do the plumbing to give it best chance of being upstreamed.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

We should probably also move that work to a new bug to avoid rabbit-holing this one even more than it already has been :-)

Comment 28

[Jonathan Kew [:jfkthame]]

I've posted a tentative patch in bug 1933050; testing appreciated!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61ab7f0a1011
Update mingw to upstream revision c0313ec3381521db6c5e3aca746ff1e3e29208d7. r=tjr

Comment 30

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/61ab7f0a1011