Function pointers and LibraryImport

[maxkatz6]

Description of Change

This PR is pretty big. If necessary, I can split HarfBuzz and Skia changes on two PRs. In theory, generated code can be be extracted into a separated PR as well, since all changes should be under conditional compilation.

What was changed step by step:

• Generator was updated to emit LibraryImport instead of DllImport, when USE_LIBRARY_IMPORT is set.
• Generator was updated to emit function pointers instead of managed delegates, when USE_LIBRARY_IMPORT is used.
• Since function pointers can't be used as generics, every DelegateProxies.Create<T> usage was replaced without generics.
• Since Skia expects bool as 1 byte, and function pointers by default marshal it as 4 bytes, I had to disable runtime marshalling. This change required to replace remaining DllImports with LibraryImport as well. As a bonus - final app size should be smaller without runtime marshalling (assuming other dependencies don't use it too).

Note:

• Due to older Mono bug, LibraryImport were defined with void* instead of full function pointer, but it doesn't seem to cause other issues
• Generated file diffs only have additions and no removals, tried to keep this way to keep less impact.
• For HarfBuss I used 2.8.2 native baseline version, as it seems latest when it was regenerated (to keep diff smaller).
• Tested on Windows and WASM (only .NET 9, but both Mono and LLVM). Will test on mobile and mac once there is any nightly build.

Bugs Fixed

• Fixes [Epic] Use newer/better interop primitives #2615
• Fixes [BUG] Delegate proxies crash with NRE when user delegate is null (only if rewrite to function pointers) #2916 (only reproducible with function pointers)

Not sure if this PR fixed them, or some another, but these two issues are not reproducible anymore:

• Fixes [QUESTION] Minimal WebAssembly example with NativeAOT-LLVM #1928
• Fixes [BUG] flushClearColor throws an error #1931

API Changes

None. Unless I missed any.

Behavioral Changes

None. Hopefully.

Required skia PR

None.

PR Checklist

• Has tests (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Merged related skia PRs
• Changes adhere to coding standard
• Updated documentation

[mattleibow]

YES!

[mattleibow]

So far this:

D:\a\1\s\binding\SkiaSharp\GRGlInterface.cs(216,83): error CA1420: Managed parameter or return types require runtime marshalling to be enabled (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1420) [D:\a\1\s\binding\SkiaSharp\SkiaSharp.csproj::TargetFramework=net7.0-tizen]
D:\a\1\s\binding\SkiaSharp\GRGlInterface.cs(234,11): error CA1421: 'System.Runtime.InteropServices.Marshal.PtrToStructure<SkiaSharp.GRGlInterface.EvasGlApi>(nint)' uses runtime marshalling even when 'DisableRuntimeMarshallingAttribute' is applied. Use features like 'sizeof' and pointers directly to ensure accurate results. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1421) [D:\a\1\s\binding\SkiaSharp\SkiaSharp.csproj::TargetFramework=net7.0-tizen]

[mattleibow]

The device failures are because all my certs just expired. The other test failures are the fact that the main branch is read and cannot pull assets. I have updated certs and re-triggered a full build.

The compile will still fail for windows I think, but the devices may run. Not 10% sure as I think there may be some bad conditionals somewhere causing the error.

[mattleibow]

Will test on mobile and mac once there is any nightly build.

The device tests should be running for iOS, Android and macOS for .NET 7, so we shall see. I do need to add .NET 8 but I will do that once this is merged.

[mattleibow]

New build here: https://dev.azure.com/xamarin/public/_build/results?buildId=118700&view=results

[maxkatz6]

So far this:

Thanks. Pushed another commit that should fix tizen build. EvasGlApi+reflection probably could be replaced with a nint[] array + named dictionary over it, but that's something for another day.

[maxkatz6]

Also updated remaining main interop projects for consistency: SkiaSharp.Resources, SkiaSharp.SceneGraph, SkiaSharp.Skottie. Didn't touch SkiaSharp.Views.** projects, as these would require even more changes.

[maxkatz6]

@mattleibow tests are failing, but I am a bit confused why.
From linux (and similar iOS) build tests, it fails on:

  Error Message:
   System.EntryPointNotFoundException : Unable to find an entry point named 'sk_rtree_factory_new' in shared library 'libSkiaSharp'.
  Stack Trace:
     at SkiaSharp.SkiaApi.sk_rtree_factory_new()
   at SkiaSharp.SKPictureRecorder.BeginRecording(SKRect cullRect, Boolean useRTree) in /home/vsts/work/1/s/binding/SkiaSharp/SKPictureRecorder.cs:line 43
   at SkiaSharp.Tests.SKPictureRecorderTest.CanCreateDrawableFromRecorder(Boolean useRTree) in /home/vsts/work/1/s/tests/Tests/SkiaSharp/SKPictureRecorderTest.cs:line 60

But I tripple checked, and sk_rtree_factory_new is correctly defined in LibraryImport/DllImport, as well as referenced skia submodule has this method included.
Am I missing something?

[mattleibow]

as part of the build I check to see if the native things have changed and then download the previous native binaries. However, the last few builds on main never finished. I triggered them so should work now.

Let me retry.

[maxkatz6]

@mattleibow side question, but what frameworks does skia sharp still supports? Especially from Mono world.
It seems like net framework does support function pointers, if you provide UnmanagedCallersOnly polyfill. But I doubt old Xamarin/Mono will work this way, without MonoPInvokeCallback. It's still technically can be used via netstandard target of the lib.

[filipnavara]

Xamarin/Mono

The old mono/mono (along with Xamarin.iOS/Xamarin.Android) went out of support earlier this year. I doubt there's any point in wasting more time on it. If someone uses the unsupported framework they can stick to older SkiaSharp release.

[maxkatz6]

Another problem is DisableRuntimeMarshalling, which also makes it easier to marshal bools in function pointers. Since DisableRuntimeMarshalling is not supported in .NET Framework, I don't think I can use function pointers without replacing all booleans with bytes or something.

[mattleibow]

Another problem is DisableRuntimeMarshalling, which also makes it easier to marshal bools in function pointers. Since DisableRuntimeMarshalling is not supported in .NET Framework, I don't think I can use function pointers without replacing all booleans with bytes or something.

We can do this in the generator... I usually add a [MarshalAs (UnmanagedType.I1)] to everything that is bool. Does that stop working with LibraryImport?

internal static extern sk_maskfilter_t sk_maskfilter_new_blur_with_flags (SKBlurStyle param0, Single sigma, [MarshalAs (UnmanagedType.I1)] bool respectCTM);

[maxkatz6]

Either way it should work with netframework, since it's just a source code generation, nothing different in runtime.

[maxkatz6]

@mattleibow just pushed a commit moving all the #if #else from DelegateProxy files to auto generated code. I reused the same generator tool for now just by extending it.

This way almost all the boilerplate is hidden behind generated code.

[mattleibow]

Yeah, much smarter... Writing a source generator to generate source for generated sources... At this point am I thinking like AI.

[maxkatz6]

@mattleibow hi!
Checked CI build and found two problems with tests:

1. I used protected internal in methods used by delegate proxies. It doesn't affect library consumers as per protected internal (C# Reference) (If the derived class is defined in a different assembly from the base class, overridden members have protected access. - which is exactly how it worked before this PR). But since Tests projects has InternalsVisibleTo configured, it also has to use protected internal.
2. There were two tests asserting generated managed delegates. Since these managed delegates are not generated anymore in .NET7+ builds, I disabled them. .NET Framework tests still check for them.

Hopefully CI should pass now.
Do you think anything else needs to be done for this PR?

[mattleibow]

After much thrashing on CI just to get a green main branch, I see we now have some failing tests that are related to this PR.

https://dev.azure.com/xamarin/public/_build/results?buildId=121990&view=ms.vss-test-web.build-test-results-tab

I think it is just these 2 really:

The first one is interesting be cause it can't find LoadLibrary. I see it is actually LoadLibraryA: https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibrarya Not sure if the new attributes require special treatment of this? I see the old way had CharSet = CharSet.Ansi. Is there an equivalent to this with the new things:

Use this field with a member of the CharSet enumeration to specify the marshaling behavior of string parameters and to specify which entry-point name to invoke (the exact name given or a name ending with "A" or "W").

The second failure is probably because you are skipping the tests in the .Console project but it also needs to be in the .Devices project.

[mattleibow]

@maxkatz6 I think this PR is good, just the 2 failures. One is a missing condition/define in the .Devices project and then the other is the auto-naming of p/invokes based on Charset that I am not sure has a direct parallel, but maybe setting the string marshalling or just using the EntryPoint property on the attribute?

[filipnavara]

The first one is interesting be cause it can't find LoadLibrary. I see it is actually LoadLibraryA: https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibrarya Not sure if the new attributes require special treatment of this? I see the old way had CharSet = CharSet.Ansi.

The distinction of A/W-suffixed APIs has not been relevant for a long while. All supported Windows versions have LoadLibraryW Unicode API and it should be used, if necessary by specifying Entrypoint = "LoadLibraryW", StringMarshalling = StringMarshalling.Utf16 in the LibraryImport attribute. (LoadLibraryA is legacy API dating back to Windows 9x era.)

Modern .NET has NativeLibrary.Load but I guess that may not be applicable here if the tests still run on .NET Framework.

[mattleibow]

I need to check that these new things are not slower: dotnet/runtime#106491

Might be unrelated, but might be related to usage of new things. Has anyone done any perf benchmarks on any platforms?

I will try set up something, but if anyone has a complex app that draws a bunch of things it would be great if you ran a check and got a speedscope: https://github.com/dotnet/maui/wiki/Profiling-.NET-MAUI-Apps

I suppose a test would be:

• Release
• Debug (maybe)
• Debug with Interpreter

[maxkatz6]

@mattleibow both tests should be fixed now.

I can run our control catalog on my iOS device later this week.

[mattleibow]

Woohoo! This is finally green and finally good to go. Merged and now time for that !

Thanks so much for this! This has been on the list since net7.0 started doing cool things.