Skip to content

Conversation

@pekspro
Copy link
Contributor

@pekspro pekspro commented Jul 4, 2020

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

Add LoadAsync methods that accepts string path.

This solves #1255

@codecov
Copy link

codecov bot commented Jul 4, 2020

Codecov Report

Merging #1257 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   82.59%   82.60%   +0.01%     
==========================================
  Files         697      697              
  Lines       30511    30521      +10     
  Branches     3444     3444              
==========================================
+ Hits        25201    25213      +12     
+ Misses       4605     4603       -2     
  Partials      705      705              
Flag Coverage Δ
#unittests 82.60% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/ImageSharp/Image.FromFile.cs 98.11% <100.00%> (+0.43%) ⬆️
src/ImageSharp/Image.FromStream.cs 79.68% <0.00%> (+1.56%) ⬆️

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 f80a960...f0d9784. Read the comment docs.

@pekspro
Copy link
Contributor Author

pekspro commented Jul 4, 2020

I have added another unit test after this PR was created. Could the validation be run again? Or do I need to create a new PR?

@JimBobSquarePants
Copy link
Member

Ah lovely, thanks for this! Will review ASAP. Don’t worry about additional commits, the CI system will automatically rebuild and test each time including everything as part of the same PR.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after adding the missing test.

/// <exception cref="UnknownImageFormatException">Image format not recognised.</exception>
/// <exception cref="InvalidImageContentException">Image contains invalid content.</exception>
/// <returns>A <see cref="Task{Image}"/> representing the asynchronous operation.</returns>
public static async Task<Image> LoadAsync(Configuration configuration, string path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is untested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov, this is now added.

@antonfirsov antonfirsov merged commit 4a48355 into SixLabors:master Jul 6, 2020
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0 milestone Jul 7, 2020
@TedStryker
Copy link

TedStryker commented Jul 14, 2020

@pekspro
I changed my code to use your new LoadAsync overload tosurprisingly find, that your's is noticeably slower.
When loading & resizing 2 random pictures (SaveAsync vs. SaveAsJpeg makes no difference), here is my "manual" benchmark (average after 30 executions):
Mine/Yours (ms):
300/550
180/300

I'm not so much into async I/O, so I have no explanation, but perhaps you have one and are able to further optimize this beautiful library.
Edit 2 (sorry): I ignored initial file loading from disk, so what the benchmarks show is probably loading from Windows filesystem cache.

My setup is Core 3.1 on Windows 8.1, my app is made with ASP.NET Core MVC.
The calls:
// My implementation: return File(await image.LoadFromDisk(requestDimensions), "image/jpeg", $"{image.Name}-{requestDimensions}.jpg");

// Yours: return File(await image.LoadFromDiskAsync(requestDimensions), "image/jpeg", $"{image.Name}-{requestDimensions}.jpg");

The code:
-Integration of your overload:

var outputStream = new MemoryStream();
using (var image = await Image.LoadAsync(Configuration.Default, Url, Config.JpegDecoder)) {               
		image.Mutate(i => i.Resize(new ResizeOptions {
			Size = new Size(requestedDimensions[0], requestedDimensions[1]),
			Mode = ResizeMode.BoxPad,
			Sampler = Config.ImageResampler,
		}));               

await image.SaveAsync(outputStream, Config.ImageJpegEncoder);
outputStream.Seek(0, SeekOrigin.Begin);

-Mine:

await using var fs = File.OpenRead(Url);
var result = new byte[fs.Length];
await fs.ReadAsync(result, 0, (int)fs.Length);

using var image = Image.Load<Rgba32>(result, Config.JpegDecoder);
image.Mutate(i => i.Resize(new ResizeOptions { /* same as above*/ }));

var outputStream = new MemoryStream();
image.SaveAsJpeg(outputStream, Config.ImageJpegEncoder);
outputStream.Seek(0, SeekOrigin.Begin);

Edit: code formatting

@antonfirsov
Copy link
Member

@TedStryker thanks for reporting this finding! It would be really helpful if you could open an issue for this performance problem. Conversations under closed issues/PR-s get lost and forgotten too often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants