Cancellable codec API, and overloads for loading/saving #1296
Conversation
…n-local # Conflicts: # src/ImageSharp/Formats/Bmp/BmpDecoder.cs # src/ImageSharp/Formats/Gif/GifDecoder.cs # src/ImageSharp/Formats/Jpeg/JpegDecoder.cs # src/ImageSharp/Formats/Png/PngDecoder.cs # src/ImageSharp/Formats/Png/PngDecoderCore.cs # src/ImageSharp/Formats/Tga/TgaDecoder.cs # src/ImageSharp/Image.FromStream.cs
antonfirsov
changed the title
antonfirsov
changed the title
tocsoft
left a comment
tocsoft
left a comment
There was a problem hiding this comment.
just a couple of things
As a rule for any async methods I feel we should have this guidance:
- Don't have multiple overloads which only differ on having a
CancellationTokenor not. - Public apis should always have the cancellation token, and it should be configured with a default value of
default - Internal/private apis should never have a default value for the
CancellationToken(to make it harder to miss passing a public token along)
|
|
||
| using var bufferedStream = new BufferedReadStream(configuration, stream); | ||
| return decoder.IdentifyAsync(bufferedStream); | ||
| return await decoder.IdentifyAsync(configuration, bufferedStream, cancellationToken); |
There was a problem hiding this comment.
as we are awaiting here this one should have a .ConfigureAwait(false)
There was a problem hiding this comment.
I don't think this actually needs awaiting does it?
There was a problem hiding this comment.
we do because of the use of the buffered stream
There was a problem hiding this comment.
We should double check elsewhere then.
There was a problem hiding this comment.
Will check if I can solve by adding a helper method.
There was a problem hiding this comment.
There's no need to create a BufferedReadStream in this method, so it can be simplified and the async context removed.
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
A couple of minor things... Very impressed though, it's cool!
|
|
||
| using var bufferedStream = new BufferedReadStream(configuration, stream); | ||
| return decoder.IdentifyAsync(bufferedStream); | ||
| return await decoder.IdentifyAsync(configuration, bufferedStream, cancellationToken); |
There was a problem hiding this comment.
I don't think this actually needs awaiting does it?
|
@tocsoft @JimBobSquarePants ready for final review. |
Codecov Report
@@ Coverage Diff @@
## master #1296 +/- ##
==========================================
+ Coverage 82.71% 82.79% +0.07%
==========================================
Files 692 689 -3
Lines 30718 30731 +13
Branches 3474 3472 -2
==========================================
+ Hits 25409 25444 +35
+ Misses 4603 4581 -22
Partials 706 706
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
I found and fixed a couple of issues where we needed to await IDisposable's but otherwise LGTM 👍
Prerequisites
Breaking changes
Image.LoadAsync(...),Image.IdentifyAsync(...),image.SaveAsync(...),image.SaveAsXyzAsync(...)overloads replaced by cancellable variants, with default argumentcancellationToken = defaultIImageEncoder,IImageDecoderandIImageVisitorreplaced by cancellable variantsDescription
ImageDecoderUtilitiesandImageEncoderUtilitiesIImageEncoderandIImageDecodercancellable.CancellationTokenis forwarded to the synchronous internal implementation classes (IImageDecoderInternalsandIImageEncoderInternalsimplementors)Image.LoadAsyncandImage.IdentifyAsynctogether with testsFormats/*/ImageExtensions.csfiles containing the.SaveAs*methods became unmaintainable with the exponential growth of parameters. Replacing them with generated code coming fromImageExtesions.Save.tt.This is a relatively large change right before releasing 1.0 (😅), so hoping for an thorough review to minimize risks.
Resolves #1280.