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

Maintain Xamarin iOS support by keeping the library "AOT-proof" #946

Closed
antonfirsov opened this issue Jul 14, 2019 · 51 comments · Fixed by #1575
Closed

Maintain Xamarin iOS support by keeping the library "AOT-proof" #946

antonfirsov opened this issue Jul 14, 2019 · 51 comments · Fixed by #1575
Labels
API area:aot help needed upstream-issue Issue depends on upstream dotnet fix.
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Jul 14, 2019

Description

Xamarin iOS users need to be able to "seed" open-generic library API-s with the help of AotCompilerTools, otherwise it will lead to runtime exceptions on that platform.

Related: #767, #785, #776, #800

Problems

Currently we are struggling to maintain this feature because of the following reasons:

  • There is no one in the core team having experience with Xamarin iOS or any other AOT platform.
  • We don't have any regression tests for this, we never know when a change breaks the "AOT-proofness" of an existing API
  • We have introduced a massive change with Introduce a non-generic Image base class #904, which will most likely break AOT & Xamarin iOS for many existing API-s in RC1. I have defined a point in epic Epic: introduce pixel-agnostic API across the whole library #907 to investigate this, however in practice it turned out that I hardly have the resources to do so during this summer. (Neither I think @JimBobSquarePants can make it.) Therefore it will most likely not happen for RC1.

Conclusion

With our current prioritites and resources, the only way to maintain this feature is community support. If you can provide any help (add missing AOT seeds, or fix regressions), feel free to open a Pull Request!

Long term solution

If someone could define a mono AOT test configuration for appveyor, that would be ace.

/CC @vicfergar @davilovick @gameleon-dev @Lapinou42 @dmanning23 @swoolcock

EDIT
PS: Could someone do a favor and chack the latest MyGet dev builds from on Xamarin iOS? (JPEG, PNG, Resize, Quantization etc.)

@antonfirsov antonfirsov changed the title Keep the library "AOT-proof" and iOS-compatible Maintain Xamarin iOS support by keeping the library "AOT-proof" Jul 14, 2019
@dmanning23
Copy link
Contributor

Cool, super appreciate the work yall have been putting into ImageSharp. I have some downtime this week and can take a look at the iOS build.

@JimBobSquarePants
Copy link
Member

@dmanning23 That would be absolutely amazing if you can, thank you very much!

dmanning23 added a commit to dmanning23/ImageSharp that referenced this issue Jul 19, 2019
@dmanning23
Copy link
Contributor

Opened a pull-request with a few changes to fix ImageSharp on iOS. I've verified with these changes that it works for jpg, gif, and image resizing.
I also rolled up some changes I've had in mind for the #800 ticket. I've verified that the AoTCompilerTools.Seed methods never actually have to be called, that code just needs to be present for the AoT compiler to pick it up.

Since these AoT/JIT errors only pop up at runtime, and only on physical iOS devices (doesn't occur on simulator!), it'll be extremely difficult to come up with a CI solution to detect them before they pop up. I've started asking around if any other Xamarin developers know how to detect & fix these issues preemptively.

@leonluc-dev
Copy link

leonluc-dev commented Jul 19, 2019

@dmanning23
Thanks for looking into this!

I've verified with these changes that it works for jpg, gif, and image resizing.

Would these changes also fix issues with PNG images? PNG was the format giving the most iOS based JIT/AOT issues in beta-0006.

@antonfirsov
Copy link
Member Author

antonfirsov commented Jul 19, 2019

Would these changes also fix issues with PNG images? PNG was the format giving the most iOS based JIT/AOT issues in beta-0006.

I don't think so.

Suggested follow-up step:

  • Run Image.Load(...) for all the test images within our test suite, inside a Xamarin iOS app (running on a device ...).
  • Collect the failing ones into a checklist of missing seeds, so we can address the issue in a systematic way.

@dmanning23
Copy link
Contributor

dmanning23 commented Jul 19, 2019

Ok, I can take a look at .png images also.
Are you having problems loading or saving to .png?

@leonluc-dev
Copy link

leonluc-dev commented Jul 20, 2019

Mostly loading from PNG bytes. I haven't been able to properly test encoding to PNG yet in an iOS environment.

In beta0006 I can consistently reproduce the following error by loading png image bytes using Image.Load(pngbytes):

Unhandled managed exception: Attempting to JIT compile method '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.
 (System.ExecutionEngineException)
  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) [0x00135] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Formats.Png.PngDecoderCore.DecodePixelData[TPixel] (System.IO.Stream compressedStream, SixLabors.ImageSharp.ImageFrame`1[TPixel] image, SixLabor
s.ImageSharp.Formats.Png.PngMetaData pngMetaData) [0x000e5] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Formats.Png.PngDecoderCore.ReadScanlines[TPixel] (System.IO.Stream dataStream, SixLabors.ImageSharp.ImageFrame`1[TPixel] image, SixLabors.ImageSharp.Formats.Png.PngMetaData pngMetaData) [0x00018] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Formats.Png.PngDecoderCore.Decode[TPixel] (System.IO.Stream stream) [0x00171] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Formats.Png.PngDecoder.Decode[TPixel] (SixLabors.ImageSharp.Configuration configuration, System.IO.Stream stream) [0x00008] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image.Decode[TPixel] (System.IO.Stream stream, SixLabors.ImageSharp.Configuration config) [0x00015] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image+<>c__DisplayClass55_0`1[TPixel].<Load>b__0 (System.IO.Stream s) [0x00000] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLa
bors.ImageSharp.Image.WithSeekableStream[T] (SixLabors.ImageSharp.Configuration config, System.IO.Stream stream, System.Func`2[T,TResult] action) [0x0002b] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image.Load[TPixel] (SixLabors.ImageSharp.Configuration config, System.IO.Stream stream, SixLabors.ImageSharp.Formats.IImageFormat& format) [0x00022] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image.Load[TPixel] (SixLabors.ImageSharp.Configuration config, System.IO.Stream stream) [0x00000] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image.Load[TPixel] (SixLabors.ImageSharp.Configuration config, System.Byte[] data) [0x00007] in <7430a37e6bca4100b1763856ff8f7867>:0 
  at SixLabors.ImageSharp.Image.Load (System.Byte[] data) [0x00005] in <7430a37e6bca4100b1763856ff8f7867>:0 

A similar error occurs when loading a png image from other sources such as a stream.

@antonfirsov
Copy link
Member Author

Someone has to call all the PNG scanline decoder methods from AotCompilerTools.

I expect similar issues with gif, and probably even certain jpegs.

The following steps do not require any knowledge related of our codebase, and it would be really helpful if someone could do them:

  1. Upload our whole test image suite to an iOS device.
  2. Run this code from inside a Xamarin.iOS app. (If running into something like OutOfMemoryException: just delete the giant jpegs.)
  3. Send back Report.txt

This will help getting a nice overview about how bad the situation is. I want to avoid doing the discussion overhead image-by-image.

dmanning23 added a commit to dmanning23/ImageSharp that referenced this issue Jul 27, 2019
@dmanning23
Copy link
Contributor

Awesome, I will have some extra time next week and will create a quick iOS app with those images & code.

antonfirsov added a commit that referenced this issue Aug 6, 2019
#946: AoT compiler fixes and cleaned up AoTCompilerTools
@dmanning23
Copy link
Contributor

dmanning23 commented Aug 7, 2019

Ok wrote a quick&dirty iOS app that loads all the images in the test suite, and didn't run into any AoT/JIT errors. Whatever error it was hitting must have been fixed in #949

@gameleon-dev If you want to send me the specific .png I can double check that it will work going forward.

Here is the sample app with output:
https://github.com/dmanning23/ImageSharpTest.iOS

@JimBobSquarePants
Copy link
Member

@dmanning23 Legend! 👑

@leonluc-dev
Copy link

@dmanning23 Thank you so much!

If it's any help testing, the images I used that generated the error were iOS screenshots. (Taken with the default iOS screenshot feature)

I've attached one of them to this comment.

Attachment: IMG_7829

@dmanning23
Copy link
Contributor

You should be good to go, I was able to load the attached image:

2019-08-07 17:24:44.794 ImageSharpTest.iOS[2745:131288] Testing Content/62653769-9175ed80-b95e-11e9-9a8d-348b071cb714.png
2019-08-07 17:24:45.439 ImageSharpTest.iOS[2745:131288]    ... SUCCESS!

JimBobSquarePants pushed a commit that referenced this issue Aug 25, 2019
* Nits - Benchmarks (#884)

* Update metadata names

* Use WithIterationCount

* Format Benchmark documents

* Update copyright assignment to Six Labors & Contributors

* Update deps

* React to Benchmark library update

* ResizeWindowOld

* ResizeWindow refactor 1

* ResizeWindow refactor 2

* ResizeWindow refactor 3

* ResizeWindow refactor 4

* basic sliding window implementation (semi-stable state)

* fix ResizeWithCropHeightMode

* reference output for Resize_BasicSmall

* minor optimization

* Handle incorrect colorspace metadata. Fix #882 (#885)

* refactor
- ResizeWindow -> ResizeWorker
- Most logic moved to ResizeWorker

* refactor stuff + implement CalculateResizeWorkerWindowCount()

* utilize CalculateResizeWorkerHeightInWindowBands()
which has been renamed from CalculateResizeWorkerWindowCount()

* improve benchmark: ArrayCopy -> CopyBuffers

* moar RowInterval stuff

* buffer.CopyColumns(...)

* simplify ResizeWorker logic

* WorkingBufferSizeHintInBytes_IsAppliedCorrectly

* more robust tests

* ResizeTests.LargeImage

* optimized sliding works!

* reapply unsafe optimizations

* moar unsafe optimization

* benchmark WorkingBufferSizeHint effects

* memory profiling with Sandbox46

* add ResizeWorker.pptx

* refine ResizeWorker.pptx

* xmldoc for ResizeWorker

* update ResizeWorker.pptx [skip CI]

* fix tests

* fix license text in CopyBuffers benchmark

* update travis.yml

* I'm terrible copy-paster

* extend the CopyBuffers benchmark

* use HashCode.Combine()

* Pass correct output size in ResizeMode.Min #892 (#893)

* Cleanup General Convolution (#887)

* Remove multiple premultiplication.

* Use in DenseMatrix everywhere.

* Make private

* Dont convert vector row on first pass

* Remove incorrectly assigned alpha.

* Remove boxing.

* Use correct min row.

* Reorder parameters

* Correctly handle alpha component.

* Update tests

* Use dedicated methods over branching.

* Faster Jpeg Huffman Decoding. (#894)

* Read from underlying stream less often

* Update benchmark dependencies

* Experimental mango port

Currently broken

* Populate table, 64byte buffer

Still broken.

* Baseline, non RST works

* 15/19 baseline tests pass now.

* Optimize position change.

* 18/19 pass

* 19/19 baseline decoded

* Can now decode all images.

* Now faster and much cleaner.

* Cleanup

* Fix reader, update benchmarks

* Update dependencies

* Remove unused method

* No need to clean initial buffer

* Remove bounds check on ReadByte()

* Refactor from feedback

* Feature: adaptive histogram equalization (#673)

* first version of sliding window adaptive histogram equalization

* going now from top to bottom of the image, added more comments

* using memory allocator to create the histogram and the cdf

* mirroring rows which exceeds the borders

* mirroring also left and right borders

* gridsize and cliplimit are now parameters of the constructor

* using Parallel.For

* only applying clipping once, effect applying it multiple times is neglectable

* added abstract base class for histogram equalization, added option to enable / disable clipping

* small improvements

* clipLimit now in percent of the total number of pixels in the grid

* optimization: only calculating the cdf until the maximum histogram index

* fix: using configuration from the parameter instead of the default

* removed unnecessary loops in CalculateCdf, fixed typo in method name AddPixelsToHistogram

* added different approach for ahe: image is split up in tiles, cdf is computed for each tile. Grey value will be determined by interpolating between 4 tiles

* simplified interpolation between the tiles

* number of tiles is now fixed and depended on the width and height of the image

* moved calculating LUT's into separate method

* number of tiles is now part of the options and will be used with the sliding window approach also, so both methods are comparable

* removed no longer valid xml comment

* attempt fixing the borders

* refactoring to improve readability

* linear interpolation in the border tiles

* refactored processing the borders into separate methods

* fixing corner tiles

* fixed build errors

* fixing mistake during merge from upstream: setting test images to "update Resize reference output because of improved ResizeKernelMap calculations"

* using Parallel.ForEach for all inner tile calculations

* using Parallel.ForEach to calculate the lookup tables

* re-using pre allocated pixel row in GetPixelRow

* fixed issue with the border tiles, when tile width != tile height

* changed default value for ClipHistogram to false again

* alpha channel from the original image is now preserved

* added unit tests for adaptive histogram equalization

* Update External

* 2x faster adaptive tiled processor

* Remove double indexing and bounds checks

* Begin optimizing the global histogram

* Parallelize GlobalHistogramEqualizationProcessor

* Moving sliding window from left to right instead of from top to bottom

* The tile width and height is again depended on the image width: image.Width / Tiles

* Removed keeping track of the maximum histogram position

* Updated reference image for sliding window AHE for moving the sliding window from left to right

* Removed unnecessary call to Span.Clear(), all values are overwritten anyway

* Revert "Moving sliding window from left to right instead of from top to bottom"

This reverts commit 8f19e5e.

# Conflicts:
#	src/ImageSharp/Processing/Processors/Normalization/AdaptiveHistEqualizationSWProcessor.cs

* Split GetPixelRow in two version: one which mirrors the edges (only needed in the borders of the images) and one which does not

* Refactoring and cleanup sliding window processor

* Added an upper limit of 100 tiles

* Performance tweaks

* Update External

* ImageBrush shouldn't Dispose of the image it is using. (#883)

Fixes #881

* Now throws a better excpetion DrawImage source does not overlap target (#877)

* No longer throws when DrawImage source does not overlap target

Previously, when DrawImage was used to overlay an image, in cases where the source image did not overlap the target image, a very confusing error was reported: "System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
Parameter name: MaxDegreeOfParallelism"

Now, when this case happens, the DrawImage method will simply not affect the target image, which is the same way FillRegionProcessor handles such cases.

ParallelHelper.IterRows also now does more validation of the input rectangle so that any further cases of this kind of problem throw a more relvant exception. Note I switched from DebugGuard to Guard because IterRows is a public API and thus should always validate its inputs.

Fixes #875

* Refines DrawImage non-overlap error logic

Adresses PR feedback in #877.

Changes DrawImage shortcut to be less than or equal to. Also changes maxX calculation to use `Right` over `Width`. This is a semantic change that reflects intention better. No actual change because Top,Left for that rectanngle should always be 0,0.

And adds more informative argument names to ParallelHelper.IterRows error message.

* Non-overlapping DrawImage now throws

Adressing PR feedback from #877

DrawImage now throws when the source image does not overlap, with a useful error message.

Also improved the error messages for IterRows (and added validation to the other IterRows method)

* DrawImage overlap test changed to support RELEASE

The tests on the CI server are run in RELEASE which wrap the expected exception with an ImageProcessingException.

* Adress feedback for DrawImage exception

DrawImage throws an ImageProcessor exception which makes it easier to catch.

And reverted IterRows to use Guard helpers

* Bitmap encoder writes V3 header as default (#889)

* Bitmap encoder will now write a V3 header as default.

Introduces a new encoder option `SupportTransparency`: With this option a V4 header will be written with BITFIELDS compression.

* Add 4 and 16 bit images to the Identify test

* Add some useful links for bitmap documentation

* Add 32 bpp images to the Identify test

* Added further information on what will change for the encoder, if SupportTransparency is used.

* Add support for encoding 16 bit per pixel bitmaps (#899)

* Implemented encoding of 16 bits per pixel bitmaps

* Add unit tests for 16 bit encoding and Bgra5551 conversion

* Add additional Bgra5551 pixel conversion tests

* Add Bgra5551 tests for Short2/4 and HalfVector2/4

* Use scaled vector conversion

* define IImageVisitor

* pixel agnostic Mutate/Clone defined

* pixel-agnostic ResizeProcessor

* pixel-agnostic decoder API

* refactor FilterProcessor stuff

* basic fixes after rebase + temporarily comment out target frameworks

* refactor the rest of the FilterProcessor code

* reached a fully compiling state

* fix processor invocation tests

* re-add and fix filter invocation tests

* fix warnings and improve xmldocs

* *ProcessorImplementation<T> ===> *Processor<T>,

add suppression of SA1413 to AssemblyInfo.cs

* validating tests for Convolution processors

* validating tests for Convolution processors

* mark FileTestBase obsolete

* improve DitherTests CQ

* extended diffusion + dither tests

* Add additional pixel conversion tests

* validating tests for Effects

* validating tests for AutoOrient

* validating tests for Skew and Entropy crop

* validating tests for Overlay processors

* Add more pixel conversion tests

* skip DitherTests on old 32 bit runtimes

* refactor BoxBlur and GaussianBlur

* making filters public

* Further refactor on Gaussian stuff

* drop IEdgeDetectorProcessor

* Refactor edge detection

* refactor Effects processors

* Finished refactoring transforms

* sealed everything

* refactor HistogramEqualization

* cache Image dimensions into a field + re-enable all target frameworks

* fix Image.FromStream() + add tests

* full coverage for Image.Load (I hope)

* fix changes applied by mistake

* Fix docs for processing extensions

* publish non-generic QuantizeProcessor

* temporarily disable multitargeting

* add skeleton for Color type

* add more Rgba64 constructor overloads

* basic Color methods

* Pixel-agnostic Binarization processors

* Implement WernerPalette and WebSafePalette for Color

* refactor dithering and error diffusion to use Color

* Add support for encoding 8-bit bitmaps

* Changed WuQuantizer to OctreeQuantizer

* Update Readme (#905)

A few minor grammatical corrections.

* Trying MagickReferenceDecoder for the 8bit greyscale image encoder tests

* Setting dither to false in the OctreeQuantizer

* verbose naming for Histogram Equalization stuff + make it public

* formatting

* refactor of Overlays

* made AutoOrientExtensions non-generic

* cleanup and document Color

* cleanup

* Switched back to the default reference decoder

* Made PaletteQuantizer non-generic all the way

* QuantizedFrame<T> using ReadOnlyMemory<T>

* Correct readonly-semantics for QuantizedFrame

* introduce IQuantizedFrame<T>

* move all extension methods under a subfolder

(non-namespace providing)

* re-enable all target frameworks

* Using tolerant comparer for the Bit8Gs image

* More docs for Color, correct naming in ColorBuilder<TPixel>

* temporarily disable target frameworks

* Enabled dither again

* validating tests for: DrawPath, FillComplexPolygon

* validation in DrawImageTest

* RecolorImageTests

* FillPolygonTests

* DrawPolygonTests, DrawLinesTests

* DrawBeziersTests, DrawComplexPolygonTests

* move implicit Color conversion to pixel types,

add micro-optimizations

* fix merge issues

* DrawImageTests: add tolerance to make all test configurations happy

* Review changes

* started the refactor

* Pen, Brush & Processors refactored

* ImageSharp.Drawing compiles

* everything builds

* tests are passing

* fix new 8bit bmp code

* move drawing extensions to a (non-namespace-provider) subfolder

* rename files

* ImageBrush can apply a source image of a different pixel type than the target

* non-generic DrawImageProcessor

* DrawImageOfDifferentPixelType test cases

* clean-up drawing processors

* fix remaining stylecop issues

* Rgba32.Definitions: use Color instead of NamedColors<T>

* Remove NamedColors<T> usages

* Not using quantization for Grey8 images when encoding 8Bit Bitmaps

* Refactor Write8Bit into two methods: one for gray and one for color

* drop unnecessary generic IImageProcessorContext<TPixel> usages

* drop almost all usages of FileTestBase

* fix tests

* re-enable target frameworks

* Add tests for quantization of 32 bit bitmap to 8 bit color image

* Renamed DrawImageTest to DrawImageTests, fixed namespace

* Using tolerant comparer for the Encode_8BitColor tests

* Removed the Encode_8BitColor, because of the quantization differences with net472 and netcore

* Re-enabled Encode_8BitColor tests, if its 64 bit process

* Using MemoryMarshal.AsBytes in Write8BitGray to get the bytes of a row

* Fix merge mistake: Using DrawImageTests from current master branch

* API cleanup (related to #907) (#911)

* temporarily disable target frameworks

* drop DelegateProcessor

* drop IImageProcessingContext<TPixel>

* drop NamedColors<T>

* drop ColorBuilder<T>

* drop the *Base postfix for clean class hierarchies

* re-enable target frameworks

* use MathF in gradient brushes

* Move PngFilterMethod to the correct namespace.

* Fix for Issue #871 and #914 (#915)

* Fix #466 (#916)

* Added funding file with a link to open collective.

* Removes usage of linq in several critical paths (#918)

* Remove linq usage from jpeg + formatting

* png

* ICC + formattiing

* Resize

* Fix base class comparison.

* Updating the repo to use Directory.Build.props/targets files (#920)

* Updating the repo to use Directory.Build.props/targets files

* Adding an InternalsVisibleTo for DynamicProxyGenAssembly2, PublicKeyToken=null

* Removing duplicate includes from the ImageSharp.csproj

* Updating the .gitattributes file to explicitly list the line endings

* Removing the ImageSharp.ruleset file, as the one from standards should be used instead

* Updating the package version management to use `PackageReference Update`

* Fix build/test (#923)

* Prevent zigzag overflow. Fix #922

* Add test image and use constants

* Improve robustness of huffman decoder.

* Fix missing "using PixelFormats" line in Readme example (#921)

* Fix missing PixelFormats line in first API example

"<Rgba32>" does not appear to be defined without the "using SixLabors.ImageSharp.PixelFormats;" line and causes a "The type or namespace name 'Rgba32' could not be found (are you missing a using directive or an assembly reference?) (CS0246)" error to occur.

* Remove stray newline

* Feature: Bitmap RLE undefined pixel handling (#927)

* Add bitmap decoder option, how to treat skipped pixels for RLE

* Refactored bitmap tests into smaller tests, instead of just one test which goes through all bitmap files

* Add another adobe v3 header bitmap testcase

* Using the constant from BmpConstants to Identify bitmaps

* Bitmap decoder now can handle oversized palette's

* Add test for invalid palette size

* Renamed RleUndefinedPixelHandling to RleSkippedPixelHandling

* Explicitly using SystemDrawingReferenceDecoder in some BitmapDecoder tests

* Add test cases for unsupported bitmaps

* Comparing RLE test images to reference decoder only on windows

* Add test case for decoding winv4 fast path

* Add another 8 Bit RLE test with magick reference decoder

* Optimize RLE skipped pixel handling

* Refactor RLE decoding to eliminate code duplication

* Using MagickReferenceDecoder for the 8-Bit RLE test

* Fix 925 (#929)

* Prevent overflow

* Cleanup huffman table

* Search for RST markers.

* Fix Benchmarks project

* Bitmap decoder now can decode bitmap arrays (#930)

* Bitmap Decoder can now decode BitmapArray

* Add tests for bitmap metadata decoing. Fix an issue that a bitmap with a v5 header would be set in the metadata as an v4 header.

* Fixed issue with decoding bitmap arrays: color map size was not determined correctly. Added more test images.

* Refactor colormap size duplicate declaration.

* Fixed an issue, that when an unsupported bitmap is loaded the typ marker was not correctly shown in the error message

* Throw UnkownFormatException on Image.Load  (#932)

* Throw ImageFormatException on load

* Unseal class and make constructor internal
- This is so that no one can new it up / inherit it outside of the assembly

* Add new exception for distinguish between different exception
- This will be used on image.load operations with invalid image streams

* ImageFormatException -> UnkownImageFormatException

* Add Image.Load throws exception tests

* Fix #937 (#938)

* Add support for decoding RLE24 Bitmaps (#939)

* Add support for decoding RLE24

* Simplified determining colorMapSize, OS/2 always has 3 bytes for each palette entry

* Enum value for RLE24 is remapped to a different value, to be clearly separate from valid windows values.

* Introduce non-generic ImageFrameCollection (#941)

* temporarily disable target frameworks

* drop DelegateProcessor

* drop IImageProcessingContext<TPixel>

* drop NamedColors<T>

* drop ColorBuilder<T>

* drop the *Base postfix for clean class hierarchies

* adding basic skeletons

* non-generic ImageFrameCollection API definition

* non-generic ImageFrameCollection tests

* cleanup + docs + more tests

* implement ImageFrameCollection methods

* tests for generic PixelOperations.To<TDest>()

* experimental implementation

* fix .ttinclude

* generate generic From<TSourcePixel>(...)

* fix RgbaVector <--> BT709 Gray pixel conversion

* Gray8 and Gray16 using ConvertFromRgbaScaledVector4() by default

* fixed all conversion tests

* ConstructGif_FromDifferentPixelTypes

* fix xmldoc and other StyelCop findings

* re-enable all target frameworks

* fix NonGenericAddFrame() and NonGenericInsertFrame()

* fix remaining bugs

* #946: AoT compiler fixes and hid Seed methods

* #946: fixed StyleCop errors

* Master cleanup (#952)

* Fix gitignore and line endings

* Update README.md

* Bokeh blur implementation (#842)

* Added base BokehBlurProcessor class, and kernel parameters

* Added method to calculate the kernel parameters

* Switched to float, added method to create the 1D kernels

* Added complex kernels normalization

* Added BokehBlurExtensions class

* Added the Complex64 struct type

* Switched to Complex64 in the BokehBlurProcessor

* Added caching system for the bokeh processor parameters

* Added WeightedSum method to the Complex64 type

* Added IEquatable<T> interface to the Complex64 type

* New complex types added

* Added method to reshape a DenseMatrix<T> with no copies

* Added bokeh convolution first pass (WIP)

* Added second bokeh convolution pass (WIP)

* Added image sum pass to the bokeh processor (WIP)

* Minor bug fixes (WIP)

* Switched to Vector4 processing in the bokeh computation

* Minor tweaks

* Added Unit test for the bokeh kernel components

* Minor performance improvements

* Minor code refactoring, added gamma parameter (WIP)

* Removed unused temp buffers in the bokeh processing

* Gamma highlight processing implemented

* Speed optimizations, fixed partials computations in target rectangle

* Increased epsilon value in the unit tests

* Fixed for alpha transparency blur

* Fixed a bug when only blurring a target rectangle

* Added bokeh blur image tests (WIP)

* Added IXunitSerializable interface to the test info class

* culture independent parsing in BokehBlurTest.cs

* Performance optimizations in the bokeh processor

* Reduced number of memory allocations, fixed bug with multiple components

* Initialization and other speed improvements

* More initialization speed improvements

* Replaced LINQ with manual loop

* Added BokehBlur overload to just specify the target bounds

* Speed optimizations to the bokeh 1D convolutions

* More speed optimizations to the bokeh processor

* Fixed code style and Complex64.ToString method

* Fixed processing buffer initialization

* Minor performance improvements

* FIxed issue when applying bokeh blur to specific bounds

* Minor speed optimizaations

* Minor code refactoring

* Fixed convolution upper bound in second 1D pass

* improve BokehBlurTest coverage

* use Gray8 instead of Alpha8

* Adjusted guard position in bokeh processor constructor

* Added BokehBlurParameters struct

* Added BokehBlurKernelData struct

* Minor code refactoring

* Fixed API change build errors

* Bug fixes with the pixel premultiplication steps

* Removed unwanted unpremultiplication pass

* Removed unused using directives

* Fixed missing using directives in conditional branches

* Update from latest upstream master

* Update Block8x8F.Generated.cs

* Update GenericBlock8x8.Generated.cs

* Manual checking for files with LF (see gitter)

* Removed unused using directive

* Added IEquatable<ComplexVector4> interface

* Added IEquatable<BokehBlurParameters> interface

* Moved bokeh blur parameters types

* Added reference to original source code

* Complex convolution methods moved to another class

* Switched to MathF in the bokeh blur processor

* Switched to Vector4.Clamp

* Added Buffer2D<T>.Slice API

* Added BokehBlurExecutionMode enum

* Added new bokeh blur processor constructors

* Added new bokeh blur extension overloads with execution mode

* Code refactoring in preparation for the execution mode switch

* Implemented execution mode switch in the bokeh processor

* Moved BokehBlurExecutionMode struct

* Removed unused using directives

* Minor code refactoring

* More minor code refactoring

* Update External

* Fix undisposed buffers

* Bokeh blur processor cache switched to concurrent dictionary

* Minor code refactoring

* remove duplicate props from csproj

* Add support for read and write tEXt, iTXt and zTXt chunks (#951)

* Add support for writing tEXt chunks

* Add support for reading zTXt chunks

* Add check, if keyword is valid

* Add support for reading iTXt chunks

* Add support for writing iTXt chunks

* Remove Test Decode_TextEncodingSetToUnicode_TextIsReadWithCorrectEncoding: Assertion is wrong, the correct keyword name is "Software"

* Add support for writing zTXt chunk

* Add an encoder Option to enable compression when the string is larger than a given threshold

* Moved uncompressing text into separate method

* Remove textEncoding option from png decoder options: the encoding is determined by the specification: https://www.w3.org/TR/PNG/#11zTXt

* Removed invalid compressed zTXt chunk from test image

* Revert accidentally committed changes to Sandbox Program.cs

* Review adjustments

* Using 1024 bytes as a limit when to compress text as recommended by the spec

* Fix inconsistent line endings

* Trim leading and trailing whitespace on png keywords

* Move some metadata related tests into GifMetaDataTests.cs

* Add test case for gif with large text

* Gif text metadata is now a list of strings

* Encoder writes each comment as a separate block

* Adjustment of the Tests to the recent changes

* Move comments to GifMetadata

* Move Png TextData to format PngMetaData

* #244 Add support for interlaced PNG encoding (#955)

* #244 Implement interlaced PNG encoding

* #244 Update documentations

* #244 Remove comment

* Cleanup

* Update PngEncoderCore.cs

* fix some spelling (#957)

* fix some spelling

* more typos

* more typos

* more typos

* more typos

* more typos

* linearSegment

* fix "as" usage (#959)

* redundant usings (#960)

* use params where possible (#961)

* use params where possible

* use params

* fix some spelling (#962)

*  Add test illustrating issue

* Add test from issue #928

* Add possible fix

* remove unused variables and methods (#963)

* remove unused variables and methods

* remove some redundant variables

* remove some redundant variables

* redundant variables

* Update DrawTextOnImageTests.cs

* Minor optimizations

* cleanup

* Cleanup (#965)

* redundant ()

* redundant stirng interpolation

* use method groups

* redundant unsafe

* redundant qualifiers

* redundant ()

* redundant init

* redundant init

* redundant casts

* redundant casts

* Throw ObjectDisposedException when trying to operate on a disposed image (#968)

* disable multitargeting + TreatWarningsAsErrors to for fast development

* Check if image is disposed

in significant Image and Image<T> methods

* Mutate / Clone: ensure image is not disposed

* Revert "disable multitargeting + TreatWarningsAsErrors to for fast development"

This reverts commit 9ad74f7.

* remove some redundant variables and type params (#971)

* remove redundant variable init

* redundant variables

* remove redundant tileY variable

* remove redundant sum variable

* redundant mcu variable

* redundant type params

* Revert "remove redundant sum variable"

This reverts commit 21de86c.

* redundant comment

* remove redundant ParallelOptions

* aviod multiple array lookup

*  use var where apparent  (#972)

* use var where apparent

* use var where apparent

* should use Rgb24

* cache max in ConvertToRgba

* cache max value

* remove some redundant usings (#976)

* remove SteppedRange (#980)

* Implement decoder tests for debugging tests and add deflate bug output file

* Add TiffConfigurationModule to core Configuration. Implement simple unit-tests for Tiff decoder.

* Add benchmarks for big and medium tiff files

* Move Tiff classes to Formats.Tiff namespace

* Mark Tiff format tests with "Tiff" category, cleanup test classes

* Improve performance of Tiff decoders

* remove some redundant constructor overloads from exceptions (#979)

* remove some redundant constructor overloads from exceptions

* re add ImageProcessingException ctor

only used in release

* Fix of build error, cleanup.
Correct Metadata updated name (for Resolution properties), temporary disable tiff native metadata properties.

* Implement temporary tiff native metadata structures

* Mark missed tiff tests, exclude DecodeManual test

* Update test images submodule

* Explanations
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0 milestone Oct 24, 2019
@omatrot
Copy link

omatrot commented May 2, 2020

@antonfirsov Following #1183 I have the same problem with RC1.

@JimBobSquarePants
Copy link
Member

@unamed000 Can you please try building from source from this branch js/aot-experiments

I'm hoping the reference to Configuration.Default is enough to trigger the linker to preserve the relevant code since each codec is explicitly referenced in the construction of the default instance. If not' I'll explicitly reference the individual formats and codecs.

@unamed000
Copy link

Hi @JimBobSquarePants

It's not working, still the same stacktrace and error with the same line

@JimBobSquarePants
Copy link
Member

It's not working, still the same stacktrace and error with the same line

I'm at a loss then....

Am I right in saying you can tell the linker to ignore the SixLabors binaries?

@unamed000
Copy link

There is a an attribute that we can tell the linker to ignore a class as far as i know which is:
[Preserve (AllMembers = true)]

But i'm not really so sure about this though.

@JimBobSquarePants
Copy link
Member

You're ahead of me then. I've only briefly looked at the documentation. I don't do any application development.

@unamed000
Copy link

unamed000 commented Sep 3, 2020

After trying to disable line by line. I figured out which line is causing this:
It was actually this in ReadFrameColors method. The Rgba32.FromRgb24 method
image

But i've no idea how to solve this.
I can workaround like this on my project:
private static void SetupAot() { var gifDecoderCore = new GifDecoderCore(null, null); gifDecoderCore.Decode<Rgba32>(null, CancellationToken.None); }

But then, the app will crash immediately without throwing any stacktrace if i send a high dimension gif file. And i guess this is memory issue problem? Is there anyway that i can workaround this?

Edit:
I've another bug the same with Rgba32 for PNG file (PngDecoderCore). Wonder why it's a special error for Rgba32 ....

@antonfirsov
Copy link
Member Author

antonfirsov commented Sep 3, 2020

@unamed000 can you try what happens if you alter the implementation of Rgba32.FromRgb24 ?

Variant 1

[MethodImpl(InliningOptions.ShortMethod)]
public void FromRgb24(Rgb24 source)
{
    this.R = source.R;
    this.G = source.G;
    this.B = source.B;
    this.A = byte.MaxValue;
}

Variant 2 (No inlining)

public void FromRgb24(Rgb24 source)
{
    this.R = source.R;
    this.G = source.G;
    this.B = source.B;
    this.A = byte.MaxValue;
}

EDIT: With and without the SetupAot workaround.

@JimBobSquarePants
Copy link
Member

I might have to dig out the Raspberry Pi again if we change that.

Perhaps adding implementation calls for all of the IPixel methods to AotCompilerTools would be better. They're missing currently.

@unamed000
Copy link

unamed000 commented Sep 3, 2020

@antonfirsov
I tried that already, for removing the InliningOptions and not.
The result is InliningOptions does not change anything. It's just my work around that will affect the result.

Also, any idea why a high dimension gif file is breaking on iOS device? Could it be because it's using too much memory?

@antonfirsov
Copy link
Member Author

@JimBobSquarePants the difference between the "small" and "large" gifs might be actually a difference in transFlag here:

if (!transFlag)
{
// #403 The left + width value can be larger than the image width
for (int x = descriptorLeft; x < descriptorRight && x < imageWidth; x++)
{
int index = Unsafe.Add(ref indicesRowRef, x - descriptorLeft);
ref TPixel pixel = ref Unsafe.Add(ref rowRef, x);
Rgb24 rgb = colorTable[index];
pixel.FromRgb24(rgb);
}
}
else
{
for (int x = descriptorLeft; x < descriptorRight && x < imageWidth; x++)
{
int index = Unsafe.Add(ref indicesRowRef, x - descriptorLeft);
if (transIndex != index)
{
ref TPixel pixel = ref Unsafe.Add(ref rowRef, x);
Rgb24 rgb = colorTable[index];
pixel.FromRgb24(rgb);
}
}
}

Does this make sense?
If this is true, the AOT compiler might probably fail to properly compile one of the paths.

It's probably worth to try seeding FromRgb24 and similar calls in Seed<TPixel>:

TPixel pixel = default;
pixel.FromRgb24(default);

@unamed000 can you try if adding this changes anything? (Probably in combination with other hacks.)

@antonfirsov
Copy link
Member Author

Also, any idea why a high dimension gif file is breaking on iOS device? Could it be because it's using too much memory?

@unamed000 in your previous comment you stated:

I try to resize the second image into 50x50 but it's still the same issue, same like it was not the dimensions after all?

Can you try if it works with the resized version with any of the hacks?

@unamed000
Copy link

Hi @antonfirsov , i'll try your suggestion tomorrow

Can you try if it works with the resized version with any of the hacks?

I actually tried that before, here is the summary for what i did

Small Image Big Image
With my hack (SetupAot) Able to Load Crash without yielding any stacktrace
Without my hack JIT Compiler stacktrace (ReadFromColors method) JIT Compiler stack trace(ReadFromColors method)

So i think with my current hack, i'm able to get rid of the AOT issue, though the crash without stacktrace could be something else, it could be either:

  • Something loop forever inside (this is the case i usually see when Xamarin does not yield any stacktrace)
  • Too high memory usage.

@antonfirsov
Copy link
Member Author

Too high memory usage.

What if you upscale a different gif? (One that did noes not produce this issue originally)
If it also hangs, can you incrementally find a size where it starts to hang? (or maybe slow down very noticeably)

@unamed000
Copy link

unamed000 commented Sep 4, 2020

It's probably worth to try seeding FromRgb24 and similar calls in Seed:

Tried that as well, somehow not working, it's only work if i seed directly for Rgba32.FromRgb24

the difference between the "small" and "large" gifs might be actually a difference in transFlag here:

The transFlag are the same for both images since the width and height ratio is the same.

What if you upscale a different gif? (One that did noes not produce this issue originally)

I've resize the gif to like your suggestion and here is the result:
250x250 gif, it's working fine
contho250x250

400x400 gif, it's crashing without stacktrace
contho400x400

Though these gif are very light, it crashes when only 400x400 pixels, so i don't think that could be high memory usage?

EDIT:
Sorry for wrong debugging information, actually it was Image.Save method that throw nothing with high dimension gif, it was crashing inside the Encode method of GifEncoderCore. And it was crashing at QuantizeFrame method.

And after further debugging, it was this method which crash silently
image

ErrorDither.ApplyQuantizationDither

@antonfirsov
Copy link
Member Author

So far it looks like the crash is a completely separate problem. Does it happen on emulator as well?

Can you help us by filing a new GitHub issue for it (with templates filled) so we can track it?

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Sep 7, 2020

Tried that as well, somehow not working, it's only work if i seed directly for Rgba32.FromRgb24

Just to clarify @unamed000 you are saying the following did not work when added to AotCompilerTools?

TPixel pixel = default;

Rgb24 rgb24 = default;
pixel.FromRgb24(rgb24);

@antonfirsov if that is the case then we should migrate to using T4 for generating the tools. Messier to extend but if it's the only thing that works we have little choice.

I would like to try this though before we go that route.

private static void AotPixelInterface<TPixel>()
    where TPixel : unmanaged, IPixel<TPixel>
{
    TPixel pixel = default;
    Rgba32 rgba32 = default;
    pixel.ToRgba32(ref rgba32);
    pixel.FromRgba32(rgba32);
    pixel.FromScaledVector4(pixel.ToScaledVector4());
    pixel.FromVector4(pixel.ToVector4());

    pixel.FromArgb32(default);
    pixel.FromBgr24(default);
    pixel.FromBgra32(default);
    pixel.FromBgra5551(default);
    pixel.FromL16(default);
    pixel.FromL8(default);
    pixel.FromLa16(default);
    pixel.FromLa32(default);
    pixel.FromRgb24(default);
    pixel.FromRgb48(default);
    pixel.FromRgba64(default);
    pixel.FromRgb24(default);
}

I've added them to https://github.com/SixLabors/ImageSharp/tree/cc84f6f66a73febd3c72ec93a932ac6868662acb for testing.

@unamed000
Copy link

@JimBobSquarePants
yes, i tried that before and i didn't workout.
But could you give me a little of explanation why we need to seed these? Even though i copied the entire code into my project seem like i need to seed them as well (I don't have much knowledge about these "seeding" things)

@JimBobSquarePants
Copy link
Member

@unamed000 I just updated my branch as I realized I made a mistake in the loader code and was calling the Rgba32 version only. I would pull the latest from that branch and build/reference from that source code.

The reason we seed the code is to attempt to ensure the AOT compiler doesn't try to delete code branches it incorrectly thinks are not being used. By explicitly calling troublesome methods we try to get the compiler to recognize their use.

You shouldn't have to copy the code to your system I wouldn't think.

@unamed000
Copy link

@JimBobSquarePants would you indicate which branch you did the update?

@JimBobSquarePants
Copy link
Member

@JimBobSquarePants
Copy link
Member

@unamed000 How did you get on building from the source in the branch?

@JimBobSquarePants
Copy link
Member

We need someone to raise an issue upstream here for us to track. We can't fix issues with the AOT compiler.

@JimBobSquarePants JimBobSquarePants added the upstream-issue Issue depends on upstream dotnet fix. label Oct 13, 2020
@UltraNamahage
Copy link
Contributor

I had a similar problem with Unity Android IL2CPP, but I found a solution.
Xamarin iOS and Unity Android IL2CPP both work with Mono, so I think you can solve it the same way.

According to the Unity manual, you need to write a specific code to make a particular generic method work.

This should probably cache all interface methods that have generic methods.
For example, this part.

I wanted to shrink a large PNG image, so I created the following code.

/// <summary>
/// Because it calls an internal class, it needs to be placed inside the ImageSharp project.
/// </summary>
static class UnityAoT<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
    /// <summary>
    /// call from <see cref="SixLabors.ImageSharp.Advanced.AotCompilerTools.Seed{TPixel}"/>
    /// </summary>
    internal static void UnitySeed()
    {
        AotCompileImageProcessor();
        AotCompileResampler();
        AotCompileEncoder();
    }
    private static void AotCompileEncoder()
    {
        default(PngEncoderCore).Encode<TPixel>(default, default, default);
    }
    private static void AotCompileImageProcessor()
    {
        AotCompilerPixelSpecifics<ResizeProcessor>();
    }
    private static void AotCompilerPixelSpecifics<TProc>()
        where TProc : class, ICloningImageProcessor
    {
        default(TProc).CreatePixelSpecificCloningProcessor<TPixel>(default, default, default);
        default(TProc).CreatePixelSpecificProcessor<TPixel>(default, default, default);
    }
    private static void AotCompileResampler()
    {
        AotCompileResamplers<BicubicResampler>();
    }
    private static void AotCompileResamplers<TResampler>()
        where TResampler : struct, IResampler
    {
        // TODO : Methodize it.
        default(ResizeProcessor<TPixel>).ApplyTransform<TResampler>(default);
        default(TResampler).ApplyTransform(default(ResizeProcessor<TPixel>));
    }
}

This is working fine, but doesn't allow me to do anything other than shrink.
I think we need to cache more methods to do all the processing.

@JimBobSquarePants
Copy link
Member

@UltraNamahage Thanks for the additional info.

We have a class AotCompilerTools which we have used to for AOT issues previously. Perhaps you can update that?

internal static class AotCompilerTools

@UltraNamahage
Copy link
Contributor

@JimBobSquarePants Please wait a week.

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

Successfully merging a pull request may close this issue.

9 participants