-
-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DrawImageProcessor AOT Issue #1703
Comments
I now see that this is aligned with the 1.10 milestone. If ready, is there a way to test a pre-release in Unity3D to confirm resolution? |
You can try our alpha builds from the MyGet feed |
Thanks you! It looks like that resolves some of the AOT issues but this one remains (Drawing.DrawImageProcessor+ProcessorFactoryWorker~1:
|
We already have seeded DrawImageProcessor here, but i guess we need to seed |
I'm not familar with the conventions the ImageSharp team uses. The following addition (hack) to AotCompilerTools.cs resolves the issue for me:
|
@stonstad would you mind creating a PR? |
The existing logic in AotCompilerTools appears to define types without instantiation which is elegant compared to my instantiation hack job. I'm similarly ignorant of how AOT compilation resolves types. |
@stonstad can you provide sample source code please? Pretty sure you're using 2 different pixel types (it's not forbidden, it'll just narrow the suspect range :P). |
|
Thanks! I'm not very familiar with AOT but just from my understanding of generics: But ImageSharp's pixel type system consists of values types only and ImageSharp/src/ImageSharp/Processing/Processors/Drawing/DrawImageProcessor{TPixelBg,TPixelFg}.cs Lines 15 to 19 in 61b137d
Drawing processor uses 2 generic value types so we actually need to populate each pair of pixels. For example, code above uses // Create Visitor with source type TPixel1
var visitor = new ProcessorFactoryVisitor<TPixelBg>(configuration, this, source, sourceRectangle);
// Visit image of unknown TPixel2 type which cannot be compiled AOT with current implementation
this.Image.AcceptVisitor(visitor); That explains why this solves the issue: [Preserve]
private static void AotCompileDrawImageProcessor<TPixelBg, TPixelFg>()
where TPixelBg : unmanaged, IPixel<TPixelBg>
where TPixelFg : unmanaged, IPixel<TPixelFg>
{
try
{
_ = new DrawImageProcessor<TPixelBg, TPixelFg>(...);
}
catch
{
}
} I guess you called it somewhere with |
Yes, I'm calling via Seed w/ AotCompileDrawImageProcessor<TPixel, TPixel>() and this works. |
I can't really think of any viable solution other than separate Roslyn-based compiler for AOT users. |
We could always raise the issue upstream to Unity and get them to fix their compiler. We raised an issue with MS and they're investigating. |
But there's nothing wrong with Unity compiler, we'are simply not seeding Given stacktrace is not from compiler but from runtime (test), called everything correctly but failed to find implementation for Everything we tell AOT is that there's |
I'd argue that's not true. When we raised dotnet/runtime#52559 with MS for issues with their compiler that we required pre-seeding for we received the following comment.
I would say that Unity should be aiming for that same bar. |
The only legitimate case when an AOT compiled C# application can hit an issue like this, is if one tries to do Exceptions like this one - where the stack trace says we're in the middle of running regular non-reflection C# when all of sudden it crashes - are simply AOT compiler bugs. The workaround with a "seeding" method are not great because e.g. they prevent trimming unused code - suddenly a lot of code is needed just because it's referenced from the seeding method. It should be eventually fixed upstream and I recommend developers to exercise pressure on the respective runtime team by filing issues instead of working around. The result will be a better runtime. |
Unity's requirement of seeding IL2CPP compilation is by design. See generic virtual methods (https://docs.unity3d.com/Manual/ScriptingRestrictions.html). One can debate what a proper design approach is for the Unity IL2CPP compiler team but this won't change the behavior that ImageSharp fails on their platform. Unity developers are already accustomed to seeding the IL2CPP compiler -- Is it theoretically possible to add a public method to ImageSharp to facilitate seeding for the API consumer? |
The Unity doc makes it sound like generic virtual method processing is in the same bucket as Reflection.Emit and just a part of life, but it isn't. It is possible to precompute the set of targets ahead of time and compile them. .NET Native does it, and CoreRT/NativeAOT does it. Here it was getting implemented in the CppCodegen backend of the CoreRT compiler (that generates textual C++, much like IL2CPP) and that backend was just a toy that a couple people hacked on - still this implementation would never throw from a dispatch at runtime. I'm only suggesting that instead of just raising an issue in repos like ImageSharp, one should also raise an issue for Unity so that they can increment the refCount of people hurt by this, because it's fixable (unlike Reflection.Emit and co. that would be fixable, but with perf penalties that nobody really wants because they involve an interpreter). |
The corresponding Unity issue (opened 7/16/21) is "1351111 (Open) IL2CPP AOT Compilation Regression". They have been given a reproduction project which uses ImageSharp 1.0.3. Pragmatically speaking, Unity isn't likely to resolve the behavior. |
If the ImageSharp team asserts the behavior is a Unity bug but the Unity team refers to it as documented behavior, it may be wise to specify that ImageSharp support for Unity is unsupported. *edited, readability. |
@stonstad do you have access to this issue? Can you post a reference to #1703 (comment) there? Maybe they haven't considered those arguments yet.
Personally, I would be fine with a community PR, that extends AotCompilerTools with automatic seeding for the |
Already done! After reading @MichalStrehovsky's helpful comment I updated the Unity ticket with a link to this thread and cited his helpful analysis. |
Do we have a link to the actual Unity ticket? |
A direct link to the Unity Ticket exposes access to all my tickets, of which quite a few contain proprietary data.
…________________________________
From: James Jackson-South ***@***.***>
Sent: Tuesday, July 20, 2021 6:18 PM
To: SixLabors/ImageSharp
Cc: Shaun Tonstad; Mention
Subject: Re: [SixLabors/ImageSharp] DrawImageProcessor AOT Issue (#1703)
Do we have a link to the actual Unity ticket?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1703 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAXZBR5OPTZLAMLA7CT6ASLTYX745ANCNFSM5AP6RNDQ>.
|
Ah ok, no problem |
Unity: We successfully reproduced this issue. It will be possible to follow the progress and a chosen resolution in our public Issue Tracker. https://issuetracker.unity3d.com/issues/system-dot-executionengineexception-error-in-l2cpp-build |
As a trade off IL2CPP by default does not do an exhaustive search for generic virtual implementations - although it is possible to do so. This was done to reduce executable size and conversion time, since the search will find all implementations that could be called, but likely aren’t. This also matched what Mono AOT did at the time and supports most Unity content. Unity 2021.1 and above IL2CPP supports a
Unity 2021.2 adds a new IL2CPP Code Generation build setting - "Faster (smaller) builds" that will not have this limitation. This mode generates generic code that can work with any type, so it has no limitations on generic instances, although this comes with a runtime performance cost. |
We are getting a similar error in Xamarin Forms:
It only throws in Release Mode, it does not occur in Debug mode. It occurs in an IValueConverter, where we merely needed to extract the height of the image. The libraries that we have loaded are as follows:
As we are looking to release to store this week, and this ticket has been open for two months, we are stripping ImageSharp out of the app for the time being, and substituting an alternative. We do look forwards to a fix, however, as we like the library very much. |
Have you raised the issue with Xamarin? Without people raising issues we’ll never get them to fix their compiler. |
Hi Jim, No, we haven't raised it with Xamarin, I found the same issue here (closed without a fix or work-around): SixLabors.ImageSharp: issue 1708 We discovered this during a Release-To-Store-Candidate QA release and unfortunately we haven't been able to get an internal ticket authorised by our product owner to pursue this, his exact words were '... just rip it out and replace it and get on with the release [to store] ...'. I've added it to my own tech-debt list but looking at one of the comments in the link above, I'm not hopeful that Xamarin [Microsoft] will fix this because of the pending .Net Maui release, I suspect that all of the effort with just two months to go before the GA is being put into .Net Maui, it's associated Blazor integration, and Visual Studio 2022, all of which we are also working with, so I'm afraid this is a low priority for us. The good news for SixLabors is that Xamarin will only be supported for another year or so, at which point his bug will just disseaper into obscurity. I do accept it's a Xamarin Compiler issue, and not a SixLabors issue, however, so apologies for us not pursuing it further with them and leaving it in your lap. Best wishes! Anthony EDIT: Thanks for the thumbs up, I just thought I'd add the exact code that caused it to throw in the IValueConverter:
|
Yep so the people involved in that conversation were, shall we say.... Less than helpful. I know a lot of work has gone into the AOT Compiler for .NET 6 so it could be that this issue simply goes away, (I'm actually very confident following Michal's comments). It's incredibly frustrating to have these issues and we've put in a ton of effort to try and workaround them. It could be that the workaround above could be a fix enough for the short term but I'd need someone with the tooling to test it (I don't personally work with Xamarin). If we can get someone to do that I will ship it with the next release. |
The good news is that I have audited our code base, and although ImageSharp is used quite a lot, it only throws in the IValueConverter above, so I'll put a temporary fix in there using another library, and revisit this and put the ImageSharp code back in when it is finally fixed. It is, after all, a splendid library, and I want to keep using it in my .Net Maui Blazor work. Thanks again, and Ciao for now! |
According to the linked issue in the public tracker this is fixed. https://issuetracker.unity3d.com/issues/system-dot-executionengineexception-error-in-l2cpp-build |
Prerequisites
DEBUG
andRELEASE
modeDescription
I'm seeing AOT-related exceptions using ImageSharp in Unity3D. This is somewhat expected since Unity IL2CPP performs AOT compilation. However, I had expected these errors to be largely mitigated in ImageSharp 1.0.3.
This is fixed -- see different AOT error in thread below.
System.ExecutionEngineException: Attempting to call method 'SixLabors.ImageSharp.Formats.Png.PngDecoderCore::Decode<SixLabors.ImageSharp.PixelFormats.Rgba32>' for which no ahead of time (AOT) code was generated.
System Configuration
Unity IL2CPP AOT Compilation in Unity 2021.1.x.
ImageSharp 1.0.3
Thanks,
Shaun
The text was updated successfully, but these errors were encountered: