microsoft / CsWinRT

C# language projection for the Windows Runtime
MIT License
542 stars 103 forks source link

SkXamlCanvas crashes with new prerelease packages #1645

Closed charlesroddie closed 2 months ago

charlesroddie commented 3 months ago

Putting an SkXamlCanvas in a WinUI app crashes with the new prerelease packages.

We are trying to test NativeAOT in WinUI for our app, which is built in SkiaSharp. We are using SkXamlCanvas because hardware accelerated support for SkiaSharp views in WinUI aren't released yet.

We get the exception:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
---> System.NotSupportedException: Managed vtable types (ie. containing any reference types) are not supported.
   at WinRT.ObjectReference`1.GetVtable(IntPtr thisPtr)
   at WinRT.ObjectReference`1..ctor(IntPtr thisPtr)
   at WinRT.ObjectReference`1.Attach(IntPtr& thisPtr, Guid iid)
   at WinRT.ObjectReference`1.TryAs(IObjectReference sourceRef, Guid iid, ObjectReference`1& objRef)
   at WinRT.IObjectReference.TryAs[T](Guid iid, ObjectReference`1& objRef)
   at WinRT.IObjectReference.As[T](Guid iid)
   at WinRT.IObjectReference.As[T]()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Reproduction:

https://github.com/charlesroddie/WinUITemplate/tree/SkXamlCanvas

Is this a CsWinRT issue or a Skiasharp issue?

Version info:

SkiaSharp 2.88.8 and also on 3.0.0-preview.3.1
CsWinRT 2.1.0-prerelease.240602.1
Microsoft.Windows.SDK.BuildTools 10.0.22621.3233
Microsoft.WindowsAppSDK 1.6.240531000-experimental1

Two trim warnings are produced in release mode, but the crash happens in debug (non-aot) and release (nativeaot):

1>ILC : Trim analysis warning IL2075: ABI.Microsoft.UI.Xaml.Data.ManagedCustomPropertyProviderVftbl.Do_Abi_GetCustomProperty_0(IntPtr,IntPtr,IntPtr*): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String,BindingFlags)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
1>ILC : Trim analysis warning IL2075: ABI.Microsoft.UI.Xaml.Data.ManagedCustomPropertyProviderVftbl.Do_Abi_GetIndexedProperty_1(IntPtr,IntPtr,Type,IntPtr*): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String,BindingFlags,Binder,Type,Type[],ParameterModifier[])'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
manodasanW commented 3 months ago

I assume the exception call stack is after a SkiaSharp API call. I haven't tried to repro this yet, but I took a quick look at SkiaSharp. It looks like they have their own implementation of IBufferByteAccess. Based on the exception message, I do believe the exception is coming from there. That code needs to be updated to reflect our new AOT compatible generated code. The main difference is the vftbl being used as the generic for ObjectReference. It should be updated to IUnknownVftbl which is what the exception message is referring to. There are a couple other changes, but you can see CsWinRT's update to its IBufferByteAccess interface implementation for storage APIs here which had the same changes done to make it AOT compatible.

charlesroddie commented 3 months ago

@mattleibow FYI work needs to be done in SkiaSharp to support WinUI changes.

mattleibow commented 3 months ago

@charlesroddie I fixed the issue in mono/SkiaSharp#2920. You can try out the artifacts here by downloading the nuget artifact:

https://dev.azure.com/xamarin/public/_build/results?buildId=118835&view=artifacts&pathAsName=false&type=publishedArtifacts

Ignore the failure, the tests are a bit flakey...

@manodasanW my fix is to just skip all the manual interop code and do it in C++. I am not sure if you know if this code is going to cause any issues: https://github.com/mono/SkiaSharp/pull/2920/files

intptr_t BufferExtensions::GetByteBuffer(IBuffer const& buffer)
{
    byte* current_data = nullptr;
    auto bufferByteAccess = buffer.as<winrt::impl::IBufferByteAccess>();
    bufferByteAccess->Buffer(&current_data);
    return (intptr_t)current_data;
}

But, one thing that I noticed is that it uses a lot of new code which only exists in the very latest WinRT.Runtime.dll. And this as far as I can tell is a viral dependency meaning that if I use this new version, all things that depend on SkiaSharp will also need this. I tried to copy some of the new code from all over, but several of the types and members are internal and it got crazy fast.

Another thing that I try and do is make sure I use the WinRT.Runtime that the SDK uses so I don't have to force all users of SkiaSharp to update things. @manodasanW when do you think this new version of WinRT.Runtime will be the version used by the .NET SDK? When I say "used" I mean the version included by default here: https://github.com/dotnet/sdk/blob/main/eng/ManualVersions.props

As of right now it is still .31

Since that is still preview and SkiaSharp is stable, I cannot really depend on it for SkiaSharp 2.x. However, the 2.x branch does not have the convenient native library that I can use. I technically can add it to 2.x but that is a lot of work, and that old branch is very iffy to build.

So, at this point @charlesroddie, are there any technical reasons why you are unable to use the 3.0 versions besides the fact it is not stable yet?

charlesroddie commented 3 months ago

So, at this point @charlesroddie, are there any technical reasons why you are unable to use the 3.0 versions besides the fact it is not stable yet?

We can test on 3.0. We'd just need to stub out Svg.Skia which is waiting for release to be 3.0-compatible.

I think going all-in on 3.0 makes sense.

manodasanW commented 2 months ago

We expect to update the .NET SDK with the new AOT compatible version pretty soon aligned with WinAppSDK stable. Given this has been addressed by the other project, closing this issue.