[Managed DWrite] Migrate Factory to managed #6171

ThomasGoulet73 · 2022-02-23T05:15:47Z

Contributes to #5305.

Description

This is the first part of #5305. I migrated only Factory to managed for now. This should allow to migrate DirectWriteForwarder to C# by migrating everything related to Factory because almost everything related to DWrite starts from Factory.

For now, the bindings to DWrite are generated by dotnet/ClangSharp and modified by hand (I plan to use dotnet/ClangSharp without modifications when I'm able to tune it like I want).

This is only a small chunk of the migration of DirectWriteForwarder to C# but hopefully this will allow to fully migrate it.

Customer Impact

It might be faster and should allow better support of trimming and the support of NativeAOT once everything is migrated to C#.

Regression

No.

Testing

Local build + CI + I tested a few apps to make sure that the text rendering is working.

Risk

Low. For the most part, it is a copy of the C++ code manually rewritten to C# with as little changes as possible.

ThomasGoulet73 · 2022-02-23T05:30:51Z

src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/TextAnalyzer.cpp

@@ -20,7 +20,7 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
        __in_ecount(length) const WCHAR* text,
        UINT32                           length,
        CultureInfo^                     culture,
-        Factory^                         factory,
+        IDWriteFactory*                  pDWriteFactory,


I changed Factory to IDWriteFactory to be able to remove Factory from DirectWriteForwarder. Factory was only used to call DWriteFactoryAddRef and then use the returned IDWriteFactory. ~~I moved the responsibility of calling DWriteFactoryAddRef to the caller the method.~~ EDIT: This is no longer the case, I changed it back to it's initial behavior by just calling AddRef on IDWriteFactory.

ThomasGoulet73 · 2022-02-23T05:31:59Z

src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/Factory.h

@@ -25,159 +25,18 @@ namespace MS { namespace Internal { namespace Text { namespace TextInterface
    /// <summary>
    /// The root factory interface for all DWrite objects.
    /// </summary>
-    private ref class Factory sealed : public CriticalHandle
+    private ref class InternalFactory abstract sealed


I renamed Factory to InternalFactory to avoid conflicts in PresentationCore since I added a new Factory class with the same namespace.

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Kernel32/Interop.GetModuleHandle.cs

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Kernel32/Interop.GetProcAddress.cs

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Kernel32/Interop.LoadLibrary.cs

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Kernel32/Interop.LoadLibraryEx.cs

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Interop.HRESULT_FROM_WIN32.cs

src/Microsoft.DotNet.Wpf/src/Common/src/Interop/Windows/Kernel32/Interop.FreeLibrary.cs

hez2010 · 2022-03-12T14:50:20Z

So can someone review and merge this PR so we can push forward the removal of C++/CLI in favor of making WPF trim/AOT-compatible?
Converting C++/CLI code to C# can also greatly improve the interop performance.

Symbai · 2022-03-12T17:16:32Z

So can someone review and merge this PR

Lol, you must be new here 😂. Welcome to the WPF repo where it takes months before a PR get's reviewed. So sit down, relax, take a coke or a coffee while waiting and enjoy your experience.

kant2002

Left comments about RCW created.

kant2002 · 2022-03-16T03:51:53Z

src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/Interop/DWrite/IDWriteFactory.cs

+
+namespace MS.Internal.Interop.DWrite
+{
+    internal unsafe struct IDWriteFactory : IUnknown


I would not try to replicate COM inheritance here. If you look how RCW/CCW with ComWrapppers implemented in runtime, or even more lean in WinForms.

For example IUnknown interface not needed, since you can use regular casts instead of QueryInterface and Marshal.AddRef(IntPtr)/Marshal.Release(IntPtr)

I would prefer to leave it like this. I try to stay closer to the code generated by ClangSharp which maps the header files to C#. This offers some kind of type safety when using generics (Like here). I also try to avoid using Marshal.AddRef(IntPtr)/Marshal.Release(IntPtr) because we can just call the methods implemented in the struct or in IUnknown. The drawback is that the code is slightly larger but I think the type safety and cleaner code (In my opinion) is worth it.

kant2002 · 2022-03-16T03:53:38Z

src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/Interop/DWrite/IDWriteFactory.cs

+
+        public int QueryInterface(Guid* riid, void** ppvObject)
+        {
+            return ((delegate* unmanaged[Stdcall]<IDWriteFactory*, Guid*, void**, int>)(lpVtbl[0]))((IDWriteFactory*)Unsafe.AsPointer(ref this), riid, ppvObject);


[Stdcall] is default call convention, and no need to specify it here.

No need to use explicit types like IDWriteFactory* where you can just have IntPtr or void*

Fixed in 557eea0394c625106e6d7f87fb35426987b47277. I tweaked ClangSharp to not add Stdcall.

kant2002 · 2022-04-07T01:55:21Z

For the @dipeshmsft or other team members.
https://github.com/dotnet/windowsdesktop/blob/c7e42392fa9470cc0e19ff85811f3c0ed53e0972/pkg/windowsdesktop/sfx/Microsoft.WindowsDesktop.App.Ref.sfxproj#L69-L70

This is almost indication that DirectWriteForwarder can be sliced and diced as we like. If I understand correctly, no ref assemblies, no contract guarantees from MS.

miloush · 2022-04-07T09:52:51Z

I would also like to point out that there is DWriteCore which should have compatible API (except the factory is createdy by DWriteCoreCreateFactory). It would be good if there could be a flag or something to use that one instead.

miloush · 2022-04-07T10:06:17Z

@kant2002 @ThomasGoulet73 I don't have much experience with RCW development, so please bear with me - I would like to learn why this approach is better than just relying on the RCW generated by the runtime.

i.e. what are the benefits of

internal unsafe struct IDWriteFactory : IUnknown
    {
        public void** lpVtbl;

        public int QueryInterface(Guid* riid, void** ppvObject)
        {
            return ((delegate* unmanaged<IDWriteFactory*, Guid*, void**, int>)(lpVtbl[0]))((IDWriteFactory*)Unsafe.AsPointer(ref this), riid, ppvObject);
        }

        public uint AddRef()
        {
            return ((delegate* unmanaged<IDWriteFactory*, uint>)(lpVtbl[1]))((IDWriteFactory*)Unsafe.AsPointer(ref this));
        }

        public uint Release()
        {
            return ((delegate* unmanaged<IDWriteFactory*, uint>)(lpVtbl[2]))((IDWriteFactory*)Unsafe.AsPointer(ref this));
        }

        ...

        public int CreateTextAnalyzer(IDWriteTextAnalyzer** textAnalyzer)
        {
            return ((delegate* unmanaged<IDWriteFactory*, IDWriteTextAnalyzer**, int>)(lpVtbl[21]))((IDWriteFactory*)Unsafe.AsPointer(ref this), textAnalyzer);
        }

over

[ComImport, Guid(UuidOf.IDWriteFactory), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
internal interface IDWriteFactory
{
    ...
    IDWriteTextAnalyzer CreateTextAnalyzer();
}

?

kant2002 · 2022-04-07T10:17:14Z

@miloush

I would also like to point out that there is DWriteCore which should have compatible API

That's very nice and if that's closely aligned maybe that's best way to use it. Still we have to wait for the feedback from the team, which almost does not receive funding on this area.

i.e. what are the benefits of

Benefits of hand-written RCW/CCW is that this make code linker friendly, and trimming friendly. Without that ILTrim does not know what COM interfaces it should brings. Also NativeAOT does not support built-in COM, so if this PR lands, and subsequent works too, then maybe can have WPF in NativeAOT. Right now because of that built-in COM trimming during R2R publish disabled for WPF apps, making them unnecessary larger.

ThomasGoulet73 · 2022-04-12T22:54:50Z

@dotnet/wpf-developers: Could this PR be considered for the next test pass?

I think it's a valuable PR since it's a first step for a migration to managed DirectWriteForwarder to allow linker and NativeAOT friendliness. I think it would be good to merge it early in .Net 7 previews to allow testing. I understand that this PR is rather large but it is the smallest changes I could think of for a starting point for the migration (I'm open to suggestions if someone can think of way to split this PR in smaller changes).

Symbai · 2022-05-05T09:28:18Z

Looks like they don't. Several PR's got queued for test now but this not. @ThomasGoulet73 I hope you still got patience.

Contributes to dotnet#5305

…ite-factory

singhashish-wpf · 2023-09-18T09:30:34Z

UPDATE:
This was included in the recent test pass.
We are currently analyzing the new test failures due to this change. As of now we have 4 new DRT failures and 15 new Microsuite failure reports with this change.

Feature	Test Case
3D	/Name="DrtBasic3D()"
	/Name="DrtElement3D()"
Annotations	/Name="DrtAnnotations()"
Imaging	/Name="DrtImaging()"

Feature	Test Case
3D	/Name="AmbientLight" /SubArea="BVT"
Annotations	/Name="MenuSuite.menu1.fixed.inkstickynote" /SubArea="MenuSuite"
	/Name="MenuSuite.menu1.fixed.stickynote" /SubArea="MenuSuite"
	/Name="MultiNoteSuite.multinote3_5.fixed.inkstickynote" /SubArea="MultiNoteSuite"
	/Name="MultiNoteSuite.multinote3_5.fixed.stickynote" /SubArea="MultiNoteSuite"
	/Name="MultiSegmentSuite.multisegment12_4.fixed./fds=false.nonvisible" /SubArea="MultiSegmentSuite"
	/Name="MultiSegmentSuite.multisegment12_4.fixed./fds=false.visible" /SubArea="MultiSegmentSuite"
	/Name="MultiSegmentSuite.multisegment12_4.fixed.nonvisible" /SubArea="MultiSegmentSuite"
	/Name="MultiSegmentSuite.multisegment12_4.fixed.visible" /SubArea="MultiSegmentSuite"
	/Name="PaginatorSuite.adp_getpage2" /SubArea="PaginatorSuite"
	/Name="StylingSuite.style8.fixed.inkstickynote" /SubArea="StylingSuite"
	/Name="StylingSuite.style8.fixed.stickynote" /SubArea="StylingSuite"
DataServices	/Name="Element3DDatabind(BindingUnderA3DViewport)" /SubArea="Controls"
Editing	/Name="IMEInsertDeleteTest_Japanese" /SubArea="IME"
Text	/Name="Display_CT_FontEmbedding5-3" /SubArea="Display_CT_FontEmbeddingTests"

kant2002 · 2023-09-18T09:45:23Z

Thank you guys!!

SmRiley · 2023-09-20T03:27:23Z

Thank you guys!!

Thank you very much for your contribution to promoting this feature. I am also very interested in this feature, but I don’t quite understand the above reply. Is the progress of this PR already at the final stage?

kant2002 · 2023-09-20T07:02:27Z

I can always negatively talk, how WPF team is moving slow, we still do not have all DRT ported, etc, etc, etc. It maybe because we have bad technical team, or other way around because we have bad management, or somebody from MS do not give enough money on the project. I simply don't know.

What I know from reply of Ashish is that this issue is getting attention, and we need only fix couple tests to start talking what else is needed. I really appreciate that even after long time of idling this issue may start moving. So I decide to say thanks to WPF team to paying attention to this PR.

driver1998 · 2023-09-20T07:45:16Z

This is the first baby step of the C++/CLI -> C# migration throughout WPF, still, good to hear things finally start moving. Thanks to the team!

pchaurasia14 · 2023-09-20T09:06:24Z

Is the progress of this PR already at the final stage?

@SmRiley - Well, it's hard to say right now. Once we investigate, isolate and fix the failing tests (either in the PR or the tests themselves), we will be in a better shape to consider the next steps with this PR.

We want to ensure that we fix the known issues first and take this PR early on so it gives us enough room to troubleshoot any future compat problems.

@kant2002 - I think some of the tests are already available on the test repo (if you'd like to take a crack at them)

…ite-factory

ThomasGoulet73 · 2023-09-21T03:21:21Z

I'm glad this PR is being considered by the team.

@pchaurasia14 I tried running the tests locally but it didn't seem to work, I'll try again in a couple of days. Let me know if the team is able to isolate the bug. Thanks.

singhashish-wpf · 2023-09-21T04:32:57Z

cc:// @harshit7962

harshit7962 · 2023-09-26T06:20:41Z

In Annotations Test, we are calling the InitializeComponent method on FixedDocument1Class here.
This makes subsequent calls to factory.cs and it's CreteFontFile method, and here we pass fontFileLoader as a nullptr, which in turn throws an ArgumentNullException.
On comparing it with the same method call in Factory.cpp here, we are passing _wpfFontFileLoader as the fontFileLoader.

@ThomasGoulet73 - Will passing the same as the parameter here solve the issue? or did you omit it foreseeing some other failure?

ThomasGoulet73 · 2023-09-27T03:36:19Z

Thank you @harshit7962. That does seem like a mistake, it was probably an oversight when trying to get it to build and then I forgot to change it.

I pushed a commit that fixes the null parameter, let me know if it fixes the tests.

I was able to run DrtAnnotations but I can't get it to fail, the tests passes before and after my last commit. DrtAnnotations also seem to pass in the CI (If it's the same thing?).

harshit7962 · 2023-09-27T05:20:21Z

DRTAnnotations and related tests are passing after your fix. We will be checking the other failures as of now.

pchaurasia14 · 2023-10-12T10:01:30Z

src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/Text/TextInterface/Factory.cs

+    /// <summary>
+    /// The root factory interface for all DWrite objects.
+    /// </summary>
+    internal unsafe class Factory


@ThomasGoulet73 - I've noticed that the equivalent class in the C++ implementation, does inherit from CriticalHandle which is missing here.

Thoughts?

I think this is a mistake. I think I was trying to reuse the class NativeIUnknownWrapper but Factory.cpp does more things in ReleaseHandle than what NativeIUnknownWrapper.ReleaseHandle does. I'll try to fix it soon and make it closer to C++ and we can clean it up later if needed.

I pushed a new commit where I redid in C# what Factory.ReleaseHandle did in C++. I took a different approach where ReleaseHandle is in a private class that inherits NativeIUnknownWrapper where it also unregisters and releases FontFileLoader and FontCollectionLoader in ReleaseHandle.

I verified that everything the FontFileLoader and FontCollectionLoader where unregistered and released when Factory is garbage collected in a sample application that forces it to be garbage collected.

…ite-factory

singhashish-wpf · 2023-10-25T08:58:07Z

All internal tests are green now for this PR, also the due process is complete. We are good to merge this one. Tagging @dotnet/dotnet-wpf-triage @dotnet/wpf-developers for any final comments or suggestions.

singhashish-wpf · 2023-10-27T09:22:09Z

Thank you @ThomasGoulet73 for all your hard work and patience! Apologies on behalf of the team for the delays. Thank you to all the contributors who commented, reviewed, debugged, and fixed test failures.

wstaelens · 2023-10-27T09:39:36Z

Yay! Updates to WPF! 🥳

ThomasGoulet73 · 2023-10-27T14:32:31Z

Big thanks to the team (Even if there was a delay 😄) and to everyone who reviewed this PR!

psmulovics · 2023-10-27T14:55:52Z

BIG one down - well done all who contributed, reviewed, merged, etc this one!

ThomasGoulet73 requested a review from a team as a code owner February 23, 2022 05:15

ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Feb 23, 2022

ghost requested review from dipeshmsft, singhashish-wpf and SamBent February 23, 2022 05:15

ThomasGoulet73 commented Feb 23, 2022

View reviewed changes

hez2010 reviewed Feb 28, 2022

View reviewed changes

bgrainger mentioned this pull request Mar 10, 2022

Poor performance for <Resource> fonts #6236

Closed

ThomasGoulet73 mentioned this pull request Mar 16, 2022

[Managed DWrite] Migrate FontCollectionLoader to managed #6260

Open

ThomasGoulet73 added the Community Contribution A label for all community Contributions label Mar 16, 2022

kant2002 reviewed Mar 16, 2022

View reviewed changes

ghost assigned ThomasGoulet73 Apr 7, 2022

ThomasGoulet73 force-pushed the managed-dwrite-factory branch from 557eea0 to 4d756fb Compare April 9, 2022 02:37

dipeshmsft self-assigned this May 5, 2022

Symbai mentioned this pull request May 6, 2022

Improve UI of error dialog for missing framework/runtime dotnet/runtime#3816

Closed

ThomasGoulet73 added 7 commits May 10, 2022 20:35

Migrate DWrite Factory to managed.

3ba4527

Contributes to dotnet#5305

Add license + cleanup

b59b454

Fix Release build

3fb2018

Fix potential memory leak

693d46a

Add back comments + cleanup

6944a79

Use code closer to generated ClangSharp code

7dd48dc

Rename IUnknown.AddReference to AddRef

1a866dc

ThomasGoulet73 added 3 commits May 9, 2023 22:05

Merge branch 'main' of https://github.com/dotnet/wpf into managed-dwr…

132849f

…ite-factory

Sync managed DWriteLoader with C++/CLI following dotnet#6538

19bd765

Use NativeLibrary instead of direct P/Invoke

49550d1

Symbai mentioned this pull request Sep 17, 2023

Code protection is severe issue. #8230

Open

Merge branch 'main' of https://github.com/dotnet/wpf into managed-dwr…

57c3799

…ite-factory

Try to fix Annotations Test

8a874fa

AaronRobinsonMSFT approved these changes Oct 10, 2023

View reviewed changes

pchaurasia14 approved these changes Oct 12, 2023

View reviewed changes

ThomasGoulet73 added 2 commits October 15, 2023 16:21

Merge branch 'main' of https://github.com/dotnet/wpf into managed-dwr…

c9226f3

…ite-factory

Unregister and release FontFileLoader and FontCollectionLoader

34fe007

singhashish-wpf approved these changes Oct 25, 2023

View reviewed changes

singhashish-wpf merged commit 3c64f9c into dotnet:main Oct 27, 2023
8 checks passed

ThomasGoulet73 deleted the managed-dwrite-factory branch October 27, 2023 14:30

ghost locked as resolved and limited conversation to collaborators Nov 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Managed DWrite] Migrate Factory to managed #6171

[Managed DWrite] Migrate Factory to managed #6171

ThomasGoulet73 commented Feb 23, 2022 •

edited

Loading

ThomasGoulet73 Feb 23, 2022 •

edited

Loading

ThomasGoulet73 Feb 23, 2022

hez2010 commented Mar 12, 2022 •

edited

Loading

Symbai commented Mar 12, 2022

kant2002 left a comment

kant2002 Mar 16, 2022

ThomasGoulet73 Apr 7, 2022

kant2002 Mar 16, 2022

ThomasGoulet73 Apr 7, 2022

kant2002 commented Apr 7, 2022

miloush commented Apr 7, 2022

miloush commented Apr 7, 2022

kant2002 commented Apr 7, 2022

ThomasGoulet73 commented Apr 12, 2022

Symbai commented May 5, 2022

singhashish-wpf commented Sep 18, 2023

kant2002 commented Sep 18, 2023

SmRiley commented Sep 20, 2023 •

edited

Loading

kant2002 commented Sep 20, 2023

driver1998 commented Sep 20, 2023

pchaurasia14 commented Sep 20, 2023

ThomasGoulet73 commented Sep 21, 2023 •

edited

Loading

singhashish-wpf commented Sep 21, 2023

harshit7962 commented Sep 26, 2023 •

edited by pchaurasia14

Loading

ThomasGoulet73 commented Sep 27, 2023

harshit7962 commented Sep 27, 2023

pchaurasia14 Oct 12, 2023

ThomasGoulet73 Oct 13, 2023

ThomasGoulet73 Oct 16, 2023

singhashish-wpf commented Oct 25, 2023

singhashish-wpf commented Oct 27, 2023

wstaelens commented Oct 27, 2023

ThomasGoulet73 commented Oct 27, 2023

psmulovics commented Oct 27, 2023

[Managed DWrite] Migrate Factory to managed #6171

[Managed DWrite] Migrate Factory to managed #6171

Conversation

ThomasGoulet73 commented Feb 23, 2022 • edited Loading

Description

Customer Impact

Regression

Testing

Risk

ThomasGoulet73 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 commented Mar 12, 2022 • edited Loading

Symbai commented Mar 12, 2022

kant2002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kant2002 commented Apr 7, 2022

miloush commented Apr 7, 2022

miloush commented Apr 7, 2022

kant2002 commented Apr 7, 2022

ThomasGoulet73 commented Apr 12, 2022

Symbai commented May 5, 2022

singhashish-wpf commented Sep 18, 2023

kant2002 commented Sep 18, 2023

SmRiley commented Sep 20, 2023 • edited Loading

kant2002 commented Sep 20, 2023

driver1998 commented Sep 20, 2023

pchaurasia14 commented Sep 20, 2023

ThomasGoulet73 commented Sep 21, 2023 • edited Loading

singhashish-wpf commented Sep 21, 2023

harshit7962 commented Sep 26, 2023 • edited by pchaurasia14 Loading

ThomasGoulet73 commented Sep 27, 2023

harshit7962 commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhashish-wpf commented Oct 25, 2023

singhashish-wpf commented Oct 27, 2023

wstaelens commented Oct 27, 2023

ThomasGoulet73 commented Oct 27, 2023

psmulovics commented Oct 27, 2023

ThomasGoulet73 commented Feb 23, 2022 •

edited

Loading

ThomasGoulet73 Feb 23, 2022 •

edited

Loading

hez2010 commented Mar 12, 2022 •

edited

Loading

SmRiley commented Sep 20, 2023 •

edited

Loading

ThomasGoulet73 commented Sep 21, 2023 •

edited

Loading

harshit7962 commented Sep 26, 2023 •

edited by pchaurasia14

Loading