Skip to content
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

Closed
4 tasks done
stonstad opened this issue Jul 16, 2021 · 33 comments
Closed
4 tasks done

DrawImageProcessor AOT Issue #1703

stonstad opened this issue Jul 16, 2021 · 33 comments
Labels
area:aot upstream-issue Issue depends on upstream dotnet fix.

Comments

@stonstad
Copy link

stonstad commented Jul 16, 2021

Prerequisites

  • I have written a descriptive issue title
  • I have verified that I am running the latest version of ImageSharp
  • I have verified if the problem exist in both DEBUG and RELEASE mode
  • I have searched open and closed issues to ensure it has not already been reported

Description

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

@stonstad
Copy link
Author

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?

@brianpopow
Copy link
Collaborator

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

@stonstad
Copy link
Author

stonstad commented Jul 16, 2021

Thanks you! It looks like that resolves some of the AOT issues but this one remains (Drawing.DrawImageProcessor+ProcessorFactoryWorker~1:

System.ExecutionEngineException: Attempting to call method 'SixLabors.ImageSharp.Processing.Processors.Drawing.DrawImageProcessor+ProcessorFactoryVisitor`1<SixLabors.ImageSharp.PixelFormats.Rgba32>::Visit<SixLabors.ImageSharp.PixelFormats.Rgba32>' for which no ahead of time (AOT) code was generated.
  at SixLabors.ImageSharp.Image`1[TPixel].Accept (SixLabors.ImageSharp.Advanced.IImageVisitor visitor) [0x00000] in <00000000000000000000000000000000>:0 
  at SixLabors.ImageSharp.Processing.Processors.Drawing.DrawImageProcessor.CreatePixelSpecificProcessor[TPixelBg] (SixLabors.ImageSharp.Configuration configuration, SixLabors.ImageSharp.Image`1[TPixel] source, SixLabors.ImageSharp.Rectangle sourceRectangle) [0x00000] in <00000000000000000000000000000000>:0 
  at SixLabors.ImageSharp.Processing.DefaultImageProcessorContext`1[TPixel].ApplyProcessor (SixLabors.ImageSharp.Processing.Processors.IImageProcessor processor, SixLabors.ImageSharp.Rectangle rectangle) [0x00000] in <00000000000000000000000000000000>:0 
  at SixLabors.ImageSharp.Processing.DefaultImageProcessorContext`1[TPixel].ApplyProcessor (SixLabors.ImageSharp.Processing.Processors.IImageProcessor processor) [0x00000] in <00000000000000000000000000000000>:0 
  at Reproduction+<>c__DisplayClass2_1.<RunTest>b__1 (SixLabors.ImageSharp.Processing.IImageProcessingContext o) [0x00000] in <00000000000000000000000000000000>:0 
  at System.Action`1[T].Invoke (T obj) [0x00000] in <00000000000000000000000000000000>:0 
  at Reproduction.RunTest () [0x00000] in <00000000000000000000000000000000>:0 

@brianpopow
Copy link
Collaborator

We already have seeded DrawImageProcessor here, but i guess we need to seed DrawImageProcessor<TPixelBg, TPixelFg> also? Im not so familiar with AOT issues.

@stonstad
Copy link
Author

I'm not familar with the conventions the ImageSharp team uses. The following addition (hack) to AotCompilerTools.cs resolves the issue for me:

        [Preserve]
        private static void AotCompileDrawImageProcessor<TPixel>()
        where TPixel : unmanaged, IPixel<TPixel>
        {
            try
            {
                _ = new ProcessorFactoryVisitor<TPixel>(null, null, null, Rectangle.Empty);
            }
            catch
            {
            }
        }

        [Preserve]
        private static void AotCompileDrawImageProcessor<TPixelBg, TPixelFg>()
            where TPixelBg : unmanaged, IPixel<TPixelBg>
            where TPixelFg : unmanaged, IPixel<TPixelFg>
        {
            try
            {
                _ = new DrawImageProcessor<TPixelBg, TPixelFg>(null, null, null, Rectangle.Empty, Point.Empty, PixelColorBlendingMode.Normal, PixelAlphaCompositionMode.Clear, 0);
            }
            catch
            {
            }
        }

@stonstad stonstad changed the title PNGEncoder/JPEGEncoder AOT Issue DrawImageProcessor AOT Issue Jul 16, 2021
@brianpopow
Copy link
Collaborator

@stonstad would you mind creating a PR?

@stonstad
Copy link
Author

stonstad commented Jul 16, 2021

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.

@br3aker
Copy link
Contributor

br3aker commented Jul 16, 2021

@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).

@stonstad
Copy link
Author

stonstad commented Jul 16, 2021

using (Image<Rgba32> sourceImage = Image.Load<Rgba32>(screenshotBytes)) // load up source images
using (Image<Rgba32> outputImage = new Image<Rgba32>(targetWidth, targetHeight)) // create output image of the correct dimensions
{
   GraphicsOptions graphicsOptions = new GraphicsOptions()
   {
     BlendPercentage = 1.0f,
     Antialias = true,
     ColorBlendingMode = PixelColorBlendingMode.Multiply
   };

   sourceImage.Mutate(o => o.Resize(targetWidth, targetHeight));
   outputImage.Mutate(o => o.DrawImage(sourceImage, new Point(0, 0), graphicsOptions));

   using (MemoryStream memoryStream = new MemoryStream())
   {
      outputImage.SaveAsPng(memoryStream, new PngEncoder() { BitDepth = PngBitDepth.Bit8, ColorType = PngColorType.Palette, CompressionLevel = PngCompressionLevel.DefaultCompression, Quantizer = new WuQuantizer() });
      memoryStream.Seek(0, SeekOrigin.Begin);
      screenshotBytes = memoryStream.GetBuffer();
   }
}

@br3aker
Copy link
Contributor

br3aker commented Jul 16, 2021

Thanks!

I'm not very familiar with AOT but just from my understanding of generics:
MyClass<TGetClass1> and MyClass<TGenClass2> share same code thus AOT should work fine resolving types for actually any reference type.

But ImageSharp's pixel type system consists of values types only and MyClass<PixelType1>/MyClass<PixelType2> cannot share single codegen. And that's where the problem lies:

/// <typeparam name="TPixelBg">The pixel format of destination image.</typeparam>
/// <typeparam name="TPixelFg">The pixel format of source image.</typeparam>
internal class DrawImageProcessor<TPixelBg, TPixelFg> : ImageProcessor<TPixelBg>
where TPixelBg : unmanaged, IPixel<TPixelBg>
where TPixelFg : unmanaged, IPixel<TPixelFg>

Drawing processor uses 2 generic value types so we actually need to populate each pair of pixels. For example, code above uses DrawImageProcessor which draws Rgba32 over Rgba32. So this visitor code won't be populated:

// 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 AotCompileDrawImageProcessor<Rgba32, Rgba32>() type parameters? @stonstad
It should be enough to quick-fix this issue for this specific case of rgba32 -> rgba32, AotCompileDrawImageProcessor<TPixel> call is not needed - it's already populated with current aot implementation.

@stonstad
Copy link
Author

Yes, I'm calling via Seed w/ AotCompileDrawImageProcessor<TPixel, TPixel>() and this works.

@br3aker
Copy link
Contributor

br3aker commented Jul 17, 2021

<TPixel, TPixel> should be threated as a hack for quick fixing this issue. Global solution for this issue is to compile each pair of pixel types for each paired generic type which is not really a good solution for two reasons:

  1. ImageSharp currently has 28 pixel types (and 1 more as feature request), just the number of permutations is 28!/26! = 756 seeding calls for each type using 2 pixel types, just typing that is not really a fun thing to do
  2. I would expect aot binary size to explode thanks to the point above

I can't really think of any viable solution other than separate Roslyn-based compiler for AOT users.

@JimBobSquarePants
Copy link
Member

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.

@br3aker
Copy link
Contributor

br3aker commented Jul 17, 2021

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 DrawImageProcessor type enough. DrawImageProcessor.ProcessorFactoryVisitor<TPixelBg>.Visit<TPixelFg>(Image<TPixelFg>) to be precise.

Given stacktrace is not from compiler but from runtime (test), called everything correctly but failed to find implementation for Visit call for specific <Rgba32, Rgba32> case. On top of it, visitors are Image-based, not Image<TPixel> with IImageProcessor abstraction.

Everything we tell AOT is that there's DrawImageProcessor<TPixel> which deals with some type Image. There's no way AOT can resolve this I'm afraid.

@JimBobSquarePants
Copy link
Member

But there's nothing wrong with Unity compiler, we'are simply not seeding DrawImageProcessor type enough.

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.

It doesn't look right that we would need this for "regular" C# - I think this is an AOT compiler bug.

I would say that Unity should be aiming for that same bar.

@MichalStrehovsky
Copy link

The only legitimate case when an AOT compiled C# application can hit an issue like this, is if one tries to do MakeGenericMethod/MakeGenericType/Array.CreateInstance. Those APIs try to create dynamic instantiations using type arguments that the AOT compiler had no chance of seeing. The API call itself should fail with a useful exception message. It's the only full AOT deficiency that can be excused.

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.

@stonstad
Copy link
Author

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?

@MichalStrehovsky
Copy link

Unity developers are already accustomed to seeding the IL2CPP compiler

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).

@stonstad
Copy link
Author

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.

@stonstad
Copy link
Author

stonstad commented Jul 20, 2021

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.

@antonfirsov
Copy link
Member

antonfirsov commented Jul 20, 2021

The corresponding Unity issue (opened 7/16/21) is "1351111 (Open) IL2CPP AOT Compilation Regression".
Pragmatically speaking, Unity isn't likely to resolve the behavior.

@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.

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.

Personally, I would be fine with a community PR, that extends AotCompilerTools with automatic seeding for the TPixelBg == TPixelFg case. This is not a universal workaround, but at least it would not bloat the code for AOT platforms which can actually handle this. (But I would reeeaaally ❤️ to see some more nagging happening against the Unity team in parallel.)

@stonstad
Copy link
Author

stonstad commented Jul 20, 2021

@stonstad do you have access to this issue? Can you post a reference to [#1703 (comment)]

Already done! After reading @MichalStrehovsky's helpful comment I updated the Unity ticket with a link to this thread and cited his helpful analysis.

@JimBobSquarePants
Copy link
Member

Do we have a link to the actual Unity ticket?

@stonstad
Copy link
Author

stonstad commented Jul 20, 2021 via email

@JimBobSquarePants
Copy link
Member

Ah ok, no problem

@stonstad
Copy link
Author

stonstad commented Aug 6, 2021

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

@scott-ferguson-unity
Copy link

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 --generic-virtual-method-iterations that will perform a deeper search for generic virtual usages and their needed implementations. It appears that for the ImageSharp, at least of the example that @stonstad submitted, a value of 5 is sufficient. IL2CPP command line arguments can set by using the environment variable IL2CPP_ADDITIONAL_ARGS environment variable or using an editor script:

using UnityEditor;
using UnityEditor.Build;
using UnityEditor.Build.Reporting;

public class SetIl2cppSettings : IPreprocessBuildWithReport
{
   public int callbackOrder => 0;
       
   public void OnPreprocessBuild(BuildReport report)
   {
      PlayerSettings.SetAdditionalIl2CppArgs("--generic-virtual-method-iterations=5");
   }
}

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.

@HarrisonOfTheNorth
Copy link

HarrisonOfTheNorth commented Sep 13, 2021

We are getting a similar error in Xamarin Forms:

(0) : Unhandled Exception:
(0) : System.ExecutionEngineException: Attempting to JIT compile method 'void SixLabors.ImageSharp.Formats.Png.PngScanlineProcessor:ProcessRgbaScanline<SixLabors.ImageSharp.PixelFormats.Rgba32> (SixLabors.ImageSharp.Configuration,SixLabors.ImageSharp.Formats.Png.PngHeader&,System.ReadOnlySpan`1<byte>,System.Span`1<SixLabors.ImageSharp.PixelFormats.Rgba32>,int,int)' while running in aot-only mode. See https://docs.microsoft.com/xamarin/ios/internals/limitations for more information.
(0) : at SixLabors.ImageSharp.Formats.Png.PngDecoderCore.ProcessDefilteredScanline[TPixel] (System.ReadOnlySpan`1[T] defilteredScanline, SixLabors.ImageSharp.ImageFrame`1[TPixel] pixels, SixLabors.ImageSharp.Formats.Png.PngMetadata pngMetadata) <0x10290cb50 + 0x00614> in <fd7f20ccdc8d46db928374008bbc5166#49456f10d5ab53f0b6aafbbdfb647b59>:0
(0) : at SixLabors.ImageSharp.Formats.Png.PngDecoderCore.DecodePixelData[TPixel] (System.IO.Compression.DeflateStream compressedStream, SixLabors.ImageSharp.ImageFrame`1[TPixel<\M-b\M^@\M-&>

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:

SixLabors.ImageSharp 1.0.3
SixLabors.ImageSharp.Drawing 1.0.0-beta13
...
Xamarin.Forms 5.0.0.2012

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.

@JimBobSquarePants
Copy link
Member

Have you raised the issue with Xamarin? Without people raising issues we’ll never get them to fix their compiler.

@HarrisonOfTheNorth
Copy link

HarrisonOfTheNorth commented Sep 13, 2021

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:

                    using (Image<Rgba32> imageImageSharp = SixLabors.ImageSharp.Image.Load<Rgba32>(imageBytes))
                    {
                        var mainDisplayInfo = DeviceDisplay.MainDisplayInfo;
                        var displayWidth = mainDisplayInfo.Width;
                        var density = mainDisplayInfo.Density;

                        var width = imageImageSharp.Width;
                        var height = imageImageSharp.Height;

                        _height = ((displayWidth - (40 * density)) / width * height) / density;

                    }

@JimBobSquarePants
Copy link
Member

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.

@HarrisonOfTheNorth
Copy link

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!

@JimBobSquarePants
Copy link
Member

According to the linked issue in the public tracker this is fixed.

image

https://issuetracker.unity3d.com/issues/system-dot-executionengineexception-error-in-l2cpp-build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:aot upstream-issue Issue depends on upstream dotnet fix.
Projects
None yet
Development

No branches or pull requests

8 participants