Async APIs / Fake Async Codecs #1196

tocsoft · 2020-05-06T11:39:18Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This adds the API surface to support async saving and loading from streams.

This is really a bit of a cheat and doesn't actually process the stream asynchronously just helps prevent people, when loading from aspnetcore setting where syncIO is disabled, having to manually do the memory copy we keep tellign them to do.. we just do it for you if we believe it will be needed and allows us (in the future) to start actually implement the codecs asynchronously as and when we want.

This API addition does raise the whether we need/want to introduce sync XXXWithFormat() apis to match the async ones and depricate/hide the overloads with out the params to keep the sync and async APIs in sync.

Also should the FormattedImage structs be made IDisposable and pass the dispose along to the Image so you can do

using(var i = async Image.ImageWithFormat(stream)){
     i.Image.DoStuff();
}

vs

var i = async Image.ImageWithFormat(stream) 
using(var img = i.Image){
     img.DoStuff();
}

or do we use some other way of representing those sorts of APIs?

+ stylecop

codecov · 2020-05-06T11:53:10Z

Codecov Report

Merging #1196 into master will decrease coverage by 0.34%.
The diff coverage is 49.69%.

@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
- Coverage   82.74%   82.40%   -0.35%     
==========================================
  Files         695      696       +1     
  Lines       30252    30553     +301     
  Branches     3429     3482      +53     
==========================================
+ Hits        25033    25176     +143     
- Misses       4523     4658     +135     
- Partials      696      719      +23

Flag	Coverage Δ
#unittests	`82.40% <49.69%> (-0.35%)`	⬇️

Impacted Files	Coverage Δ
src/ImageSharp/Formats/Gif/GifDecoder.cs	`50.00% <0.00%> (-42.31%)`	⬇️
src/ImageSharp/Formats/Gif/GifDecoderCore.cs	`79.20% <0.00%> (-4.70%)`	⬇️
src/ImageSharp/Formats/Jpeg/JpegDecoder.cs	`48.14% <0.00%> (-44.71%)`	⬇️
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs	`89.68% <0.00%> (-3.20%)`	⬇️
src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs	`94.61% <ø> (ø)`
src/ImageSharp/Formats/Png/PngDecoder.cs	`47.61% <0.00%> (-43.30%)`	⬇️
src/ImageSharp/Formats/Png/PngDecoderCore.cs	`88.09% <0.00%> (-2.52%)`	⬇️
src/ImageSharp/Formats/Tga/TgaDecoder.cs	`45.45% <0.00%> (-45.46%)`	⬇️
src/ImageSharp/Formats/Tga/TgaDecoderCore.cs	`87.57% <0.00%> (-3.70%)`	⬇️
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs	`93.53% <50.00%> (-0.91%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c417f...73fed79. Read the comment docs.

JimBobSquarePants · 2020-05-06T16:35:28Z

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

+            }
+            else
+            {
+                // cheat for now do async copy of the stream into memory stream and use the sync version


I use a pool backed memory stream that allocates small chunks in .Web. Not sure how performant reading would be though.

https://github.com/SixLabors/ImageSharp.Web/blob/master/src/ImageSharp.Web/ChunkedMemoryStream.cs

Is maybe RecyclableMemoryStream an option? Its meant to be a drop in replacement for MemoryStream

I want to have a look at a read-only version of this less overhead and should support all out target frameworks.

RecyclableMemoryStream seems more sophisticated at first glance.

Whichever we choose, I think it would make sense to integrate our adapted solution with MemoryAllocator.

src/ImageSharp/Formats/Jpeg/JpegDecoder.cs

src/ImageSharp/Formats/Png/PngEncoder.cs

src/ImageSharp/Image.Decode.cs

src/ImageSharp/Image.cs

src/ImageSharp/Image.FromStream.cs

JimBobSquarePants · 2020-05-08T19:14:29Z

@tocsoft I'll review this over the weekend.

antonfirsov · 2020-05-11T15:04:13Z

I will also try my best to have a look, but I need a couple of days, hope to make it before Saturday.

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

JimBobSquarePants · 2020-05-17T14:27:43Z

Making FormattedImage etc implement IDisposable would certainly make the code less awkward to consume, I'm not sure about the naming of those types though as they suggest to me that they affect the layout of the image in memory. I'm, thinking something like FormatImagePair

src/ImageSharp/Advanced/AdvancedImageExtensions.cs

src/ImageSharp/Image.FromStream.cs

antonfirsov

Sorry for joining the party late. Mostly comments / discussion starters, the only real concern is the one about the FormattedImage type.

antonfirsov · 2020-05-21T23:12:25Z

src/ImageSharp/Image.FromStream.cs

+        /// <exception cref="UnknownImageFormatException">Image format not recognised.</exception>
+        /// <exception cref="InvalidImageContentException">Image contains invalid content.</exception>
+        /// <returns>A <see cref="Task{FormattedImage}"/> representing the asynchronous operation.</returns>
+        public static Task<FormattedImage> LoadWithFormatAsync(Stream stream)


Wondering whether it would be better to have this family of methods under FormattedImage and FormattedImageInfo. Cleaner naming, but would be worse for discoverability.

Structurally cleaner yeah, but I think we'll end up confusing people if we split out the async API. I'd rather avoid the questions.

antonfirsov · 2020-05-21T23:26:32Z

src/ImageSharp/FormattedImage.cs

+    /// <summary>
+    /// Struct to curry <see cref="Image"/> and <see cref="IImageFormat"/> for return from async overloads.
+    /// </summary>
+    public readonly struct FormattedImage : IEquatable<FormattedImage>


What is the benefit for defining this type as a struct instead of a class? (Performance doesn't matter here.)

Is it future proof in it's current form? We may want to extend it to contain more more stuff.

I'm not happy with the name just now... Needs some thought

I've been putting some thought to this; I really don't want it to be an extensible object.

My reasoning is that we are merely replicating the synchronous API here.

I'm still not happy with the name Formatted suggests the image data is encoded. Do we even need our own types? We could use a named Tuple return type?

public static Task<(Image image, IImageFormat Format)> LoadWithFormatAsync (...

I think this also means we can keep the existing synchronous out IImageFormat APIs. It becomes more obvious with a tuple we are working around the syntatic limitations of the language.

Named Tuples require a minimum language versions though (I'm assuming @tocsoft that's why you went the explicit struct route). Do we want to enforce that?

Dammit this is tricky.

To be honest I totally didn't think about named tuples having a min version.

The reason I made an explicit type was because I was working on thought that you shouldn't return Tuples from public APIs, but on this sort of return site (working around language constructs) in hind sight I'm now less bothered in this instance. Even for older language versions just the types of the 2 properties are unique enough to distinguish what the items are.

Thanks! I think I’m gonna push onwards with them then. The language constraints are low enough and they just feel right as an alternative to the sync API.

If it's not an extensibility point, anonymous tuple looks better.

antonfirsov · 2020-05-21T23:34:33Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

+            }
+            else
+            {
+                using (var ms = new MemoryStream())


In the future we may consider using our own variant of RecyclableMemoryStream to reduce allocation pressure.

Update: I joined the existing discussion.

We don't even need one tbh. We know the length of the input stream, tightly control the lifecycle of the MemoryStream. We could simply rend an array from the shared pool and pass to the MemoryStream constructor.

antonfirsov · 2020-05-21T23:44:20Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

+        {
+            if (stream.CanSeek)
+            {
+                return this.Decode<TPixel>(stream);


I'm not sure whether this execution path is Stephen Toub proof, however I can live with it 😄

I have mixed feelings: the strict ASP.NET rules prohibiting sync code are unnecessary for CPU heavy imaging code path, but using async over sync is also far from being ideal. The picture is somewhat better if I view it as an implementation detail of decoders to be improved in a future by implementing actual async decoder path.

antonfirsov · 2020-05-21T23:48:32Z

src/ImageSharp/Formats/Bmp/BmpDecoder.cs

+            Guard.NotNull(stream, nameof(stream));
+
+            var decoder = new BmpDecoderCore(configuration, this);
+
+            try
+            {
+                return await decoder.DecodeAsync<TPixel>(stream).ConfigureAwait(false);
+            }
+            catch (InvalidMemoryOperationException ex)
+            {
+                Size dims = decoder.Dimensions;
+
+                throw new InvalidImageContentException($"Can not decode image. Failed to allocate buffers for possibly degenerate dimensions: {dims.Width}x{dims.Height}. This error can happen for very large RLE bitmaps, which are not supported.", ex);
+            }


We can DRY away these code duplications with the help of an IImageDecoderCore interface if we want. (Same for encoders.)

Yeah, I like this. Can come later though since everything is internal (unless someone is desperately keen to do it).

JimBobSquarePants · 2020-05-27T17:35:55Z

I need to pick this up as it's likely beneficial to #1205

tocsoft added 4 commits May 3, 2020 13:14

Add Async APIs to IImageEncoder and IImageDecoder

9bbd505

Save async tests

4d95e2e

implement Load Async apis

dff8ab6

IdentifyAsync

c69683e

+ stylecop

JimBobSquarePants reviewed May 6, 2020

View reviewed changes

brianpopow reviewed May 6, 2020

View reviewed changes

JimBobSquarePants and others added 14 commits May 17, 2020 11:19

Update src/ImageSharp/Formats/Jpeg/JpegDecoder.cs

2599275

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Formats/Png/PngEncoder.cs

ef3b71f

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.Decode.cs

e643553

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.Decode.cs

a27eb49

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.cs

f7beaa1

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

2fc3058

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

740d133

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

6c18639

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

b505771

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

1969165

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Update src/ImageSharp/Image.FromStream.cs

43b0d5e

Co-authored-by: Brian Popow <38701097+brianpopow@users.noreply.github.com>

Merge branch 'master' into sw/fake-async-codecs

84c7958

async await where required

9bbf05d

implement IEquatable<T>

1f74e31

JimBobSquarePants added the API label May 17, 2020

Rename extension

6361a22

tocsoft commented May 17, 2020

View reviewed changes

src/ImageSharp/Advanced/AdvancedImageExtensions.cs Outdated Show resolved Hide resolved

Revert unrequired async/await additions

5a8f78a

tocsoft commented May 17, 2020

View reviewed changes

src/ImageSharp/Image.FromStream.cs Outdated Show resolved Hide resolved

Update Image.FromStream.cs

b8232e4

Merge branch 'master' into sw/fake-async-codecs

c8c561b

antonfirsov reviewed May 21, 2020

View reviewed changes

Merge branch 'master' into sw/fake-async-codecs

72257ba

JimBobSquarePants added 2 commits June 9, 2020 17:15

Merge branch 'master' into sw/fake-async-codecs

5d4a18f

Use named tuple

73cc796

antonfirsov approved these changes Jun 9, 2020

View reviewed changes

Use pooled stream for async decode/identify

73fed79

JimBobSquarePants added this to the 1.0.0 milestone Jun 9, 2020

JimBobSquarePants merged commit b29bb74 into master Jun 9, 2020

JimBobSquarePants deleted the sw/fake-async-codecs branch June 9, 2020 23:05

dependabot bot mentioned this pull request Sep 23, 2025

Bump the nuget group with 5 updates norschel/enterJSWebSecurity2025-Demo1#8

Open

dependabot bot mentioned this pull request Oct 19, 2025

Bump the nuget group with 1 update ewdlop/bepuphysics2Fork#1

Open

Uh oh!

Async APIs / Fake Async Codecs #1196

Async APIs / Fake Async Codecs #1196

Uh oh!

Conversation

tocsoft commented May 6, 2020

Prerequisites

Description

Uh oh!

codecov bot commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JimBobSquarePants commented May 8, 2020

Uh oh!

antonfirsov commented May 11, 2020

Uh oh!

JimBobSquarePants commented May 17, 2020

Uh oh!

Uh oh!

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

antonfirsov May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 6, 2020 •

edited

Loading

JimBobSquarePants May 6, 2020 •

edited

Loading

antonfirsov May 21, 2020 •

edited

Loading

antonfirsov May 21, 2020 •

edited

Loading

antonfirsov May 21, 2020 •

edited

Loading

antonfirsov May 21, 2020 •

edited

Loading