SixLabors / ImageSharp

:camera: A modern, cross-platform, 2D Graphics library for .NET
https://sixlabors.com/products/imagesharp/
Other
7.41k stars 852 forks source link

.Net 5.0 RC-1 breaks ImageSharp #1356

Closed lofcz closed 4 years ago

lofcz commented 4 years ago

Prerequisites

Description

I've encountered a strange bug when mutating images after migrating mvc app from net fw48 to net5 rc1. I've swapped to async image handling. Steps I do (async action on a controller):

1) await HttpContext.Request.ReadFormAsync()
2) Image<Rgba32> img = Image.Load<Rgba32>([first image from form].OpenReadStream(), out IImageFormat imgFormat);
3) await img.SaveAsync("some path"); // saves the image correctly
4) img.Mutate(x => x.Resize(w, h, s)); // thumbnail; used Bicubic, tried Lancoz3 to check if that could be the problem
5) await img.SaveAsync("thumbnail path");

// also tried alternative
5) await using FileStream stream = System.IO.File.Create("some path");
6) await img.SaveAsync(stream, imgFormat);

Extremely frustrating is that this is happening only on production, not on my local machine. I've tried to use another sampler and use latest dev build of ImageSharp, neither helped. In each and every case the first half of an image is mutated correctly, while the other is either blank or filled with artifacts. I'm clueless about how should I move forward, the only option so far would be to go back to sync processing of images which I'd very much like to avoid. Attaching some test images:

Saved thumbnail 1:

a8f3fce0-c6ec-4e4b-a304-ae381301f9ef_thumbnail (1)

Saved non-mutated image 1:

a8f3fce0-c6ec-4e4b-a304-ae381301f9ef

Saved thumbnail 2:

803238f6-8248-4df7-ac73-a0f36840295a_thumbnail

Saved non-mutated image 2:

803238f6-8248-4df7-ac73-a0f36840295a

Saved thumbnail 3:

bc54f22d-1ae8-4135-ad38-3f3e761767f4_thumbnail

Saved non-mutated image 3:

bc54f22d-1ae8-4135-ad38-3f3e761767f4

System Configuration

ImageSharp version:

JimBobSquarePants commented 4 years ago

It's hard to be sure by reading from your code sample but it looks to me like you are not disposing of your input stream nor the image.

Image<Rgba32> img = Image.Load<Rgba32>([first image from form].OpenReadStream(), out IImageFormat imgFormat);

Should be.

using Stream inputStream = [first image from form].OpenReadStream();
using Image<Rgba32> img = Image.Load<Rgba32>(inputStream, out IImageFormat imgFormat);

In the case of Image, the GC would collect everything eventually but it's wasteful so you should always dispose after using. The stream though must be disposed of to ensure the content is all written from any internal buffers.

lofcz commented 4 years ago

@JimBobSquarePants thanks, I've noticed that when I clone the image and then mutate the clonned copy it works properly both on dev and prod. Couldn't that be caused by ConfigureAwait(false) ImageSharp is using when processing async save?

JimBobSquarePants commented 4 years ago

I couldn't tell you. Could you please post a complete sample so that we can correctly replicate and diagnose any issues.

klogeaage commented 4 years ago

I can confirm this issue, which started occurring after I upgraded from Net Core 3.1 to 5.0 RC1. I am NOT using the async variants of the ImageSharp methods, except the controller if of course async, and I believe I am using/disposing of Imageand Streamscorrectly. Going back to .Net Core 3.1 fixed the issue, but of course I would also like to move to 5.0.

JimBobSquarePants commented 4 years ago

I’ll need a means to replicate the issue. Without one nothing happens.

klogeaage commented 4 years ago

Sure, I'll see if I can replicate in some unit tests.

JimBobSquarePants commented 4 years ago

Thanks. That would be incredibly helpful!

klogeaage commented 4 years ago

Argh, it's working fine in an Xunit test with just the imaging and stream operations :-(

So for what it's worth, here is my ResizeTo method:

public MemoryStream ResizeTo(Stream input, int width)
{
    var output = new MemoryStream();
    input.Seek(0, SeekOrigin.Begin);
    using Image image = Image.Load(input);
    image.Mutate(x => x.Resize(width: width, 0));
    image.SaveAsJpeg(output);
    output.Seek(0, SeekOrigin.Begin);
    input.Seek(0, SeekOrigin.Begin);
    return output;
}

And here is the simple test that is working with .Net Core 3.1 and .Net 5 RC1:

using System.IO;
using System.Threading.Tasks;
using Xunit;

namespace JourneyDoc.API.Services.Tests
{
    public class ImageProcessorTests
    {
        readonly ImageProcessor _imageProcessor = new ImageProcessor();

        [Theory]
        [InlineData(@".\TestData\20200826_132801730")]
        public async Task ResizeTo_ReturnsExpectedOutputAsync(string inputFileName)
        {
            string outputFileName = inputFileName + "_out.jpg";
            using Stream inputStream = new FileStream(inputFileName + ".jpg", FileMode.Open, FileAccess.Read);
            using (MemoryStream lowResStream = _imageProcessor.ResizeTo(inputStream, ImageProcessor.LowResWidth))
            {
                using (Stream outputStream = new FileStream(outputFileName, FileMode.Create, FileAccess.Write))
                {
                    await lowResStream.CopyToAsync(outputStream);
                }
            }
            // Assert.Equals missing here, but image looks fine when just inspected manually
        }
    }
}
klogeaage commented 4 years ago

And the code (fragment) that is not working in .Net 5.0 RC1:

    var imageResizer = new ImageProcessor();
    // Build low-resolution version as well
    using (MemoryStream lowResStream = imageResizer.ResizeTo(destinationStream, ImageProcessor.LowResWidth))
    {
        await _blobStorageClient.UploadAsync(file.FileName, file.ContentType, lowResStream, blobId + 
            ImageProcessor.LowResExtension);
        _logger.LogInformation($"Uploaded low-res image of {blobId}.");
    }
...
    string uri = await _blobStorageClient.UploadAsync(file.FileName, file.ContentType, destinationStream, uploadId);
    destinationStream.Dispose();
    originalStream.Dispose();

All quite simple, I think. Very strange...

The only good thing is that it is going wrong very consistently in the Asp.Net production environment in Azure. So it is not an intermittent problem..

JimBobSquarePants commented 4 years ago

I’m assuming you confirmed that the issue isn’t with the Azure upload by saving the output to the file system before uploading?

klogeaage commented 4 years ago

No, I haven't, but it is uploading the original image as well as video and sound files with no issues, so I don't think there is an issue with that. Also, the Azure blob storage libraries are used extensively in many applications so any problem with those is likely to be reported and fixed very quickly. And this has been working for months until earlier this week where I did the 5.0 update.

klogeaage commented 4 years ago

@lofcz Could you please elaborate a little on the workaround you mentioned with Cloning the image?

JimBobSquarePants commented 4 years ago

It’s best to check just to rule out any possibilities.

What is ImageProcessor? What happens when you use LoadAsync?

I wouldn’t recommend Clone since that creates an unnecessary copy.

klogeaage commented 4 years ago

ImageProcessor is simply my encapsulation of your library, i.e. it is a service class for the controller. Here it is:

using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Metadata.Profiles.Exif;
using SixLabors.ImageSharp.Processing;
using System.IO;

namespace JourneyDoc.API.Services
{
    public class ImageProcessor
    {
        public const string LowResExtension = ".low";
        public const int LowResWidth = 700;
        public const int DefaultQuality = 90;

        public MemoryStream ResizeTo(Stream input, int width)
        {
            var output = new MemoryStream();
            input.Seek(0, SeekOrigin.Begin);
            using Image image = Image.Load(input);
            image.Mutate(x => x.Resize(width: width, 0));
            image.SaveAsJpeg(output);
            output.Seek(0, SeekOrigin.Begin);
            input.Seek(0, SeekOrigin.Begin);
            return output;
        }

        public Stream ReCompress(Stream input, int quality = DefaultQuality)
        {
            var output = new MemoryStream();
            input.Seek(0, SeekOrigin.Begin);
            using Image image = Image.Load(input);
            image.Mutate(x => x.AutoOrient());
            image.SaveAsJpeg(output, new SixLabors.ImageSharp.Formats.Jpeg.JpegEncoder { Quality = quality });
            output.Seek(0, SeekOrigin.Begin);
            input.Seek(0, SeekOrigin.Begin);
            return output;
        }

        /// <summary>
        /// Rotate image so it always is displayed correctly. Workaround for client not doing this.
        /// </summary>
        /// <param name="input"></param>
        /// <returns></returns>
        public Stream AutoOrient(Stream input)
        {
            input.Seek(0, SeekOrigin.Begin);
            using Image image = Image.Load(input);
            IExifValue exifOrientation = image.Metadata?.ExifProfile?.GetValue(ExifTag.Orientation);
            if (exifOrientation?.GetValue() == null || exifOrientation.GetValue().ToString() == "1")
            {
                input.Seek(0, SeekOrigin.Begin);
                return input;
            }
            var output = new MemoryStream();
            image.Mutate(x => x.AutoOrient());
            image.SaveAsJpeg(output, new SixLabors.ImageSharp.Formats.Jpeg.JpegEncoder { Quality = DefaultQuality });
            output.Seek(0, SeekOrigin.Begin);
            input.Seek(0, SeekOrigin.Begin);
            return output;
        }
    }
}

I don't use LoadAsync or any of the other async methods in ImageSharp, I probably should - do you think it could have a positive effect on this problem? It seems that @lofcz was using them, so now we know it doesn't seem to really influence this issue.

P.S. It's getting late here in Denmark, so don't expect any immediate response tonight.

klogeaage commented 4 years ago

I have new information about this problem: it only manifests itself on servers with relatively little memory. I was wondering why I didn't observe the problem in my test environment, which is running on one rather big Azure App Service plan with 7 GB of memory, while the pre-production environment is running on a smaller setup with only 1.75 GB of memory (but more of them to scale out). Turns out that if I scale up the pre-prod environment to a bigger P2V2 App Service with 7 GB of memory, then the problem goes away!

At no point has it reported any out of memory exceptions and .Net Core 3.1 is running fine on the small machines. It may also explain why my unit tests, running on a development machine with 24 GB memory didn't replicate the issue.

The small servers were reporting around 70% max memory usage before the upgrade, and now we are down to 20%, which seems a ridiculously low margin.

What do you make of that?

klogeaage commented 4 years ago

Found this rather recent issue that could be relevant: https://developercommunity.visualstudio.com/content/problem/1129949/programs-compiled-with-net-50-use-significantly-mo.html

lofcz commented 4 years ago

The workaround I've mentioned earlier proved to be unreliable. I've resorted to libvips backend for image processing temporary until this is resolved. To stress - i'm not even sure if this is caused by ImageSharp as it worked on my local machine correctly. @klogeaage

klogeaage commented 4 years ago

@JimBobSquarePants I think this warrants an issue with .Net 5, but I don't have enough insights into ImageSharp workings to register it.

brianpopow commented 4 years ago

@klogeaage could you give me the input image you have used or tell me maybe the dimensions? Im trying to reproduce the issue.

klogeaage commented 4 years ago

@brianpopow I've seen it with many images taken with iOS devices, e.g. 3024 x 4032 in 72 dpi. Example from our garden:

20200923_063524549

klogeaage commented 4 years ago

And this is how it looks when attempting a resize on a server with too little memory (for .Net 5.0, works fine with Net.Core 3.1): 72047050-5bf0-4e25-b7bd-0a4133fd1d7d low

While this is the result from a server with exact same configuration, except plenty of memory: c3548dde-87e4-4a6a-adc4-046d12776000 low

Pretty convincing, if I may say so... @brianpopow

brianpopow commented 4 years ago

@klogeaage i was able to reproduce it with docker with this image: mcr.microsoft.com/dotnet/sdk:5.0.100-rc.1

Here is the branch if someone wants to test it: https://github.com/SixLabors/ImageSharp/tree/bp/Issue1356docker

I have restricted docker to 1 GB and have given it 24 GB, this issue was still there in both cases.

The weird thing is that my resized image looks different:

garden

edit: when i run it on my desktop computer (windows), its working fine with netcoreapp5.0

klogeaage commented 4 years ago

@brianpopow , Yes, that is weird. Sometimes I feel this wonderful world of it is so complex and unpredictable that it is a miracle anything works...

But really glad you could reproduce the issue, now you just need to find the root cause and fix it, or make a qualified report to Microsoft, of course.

JimBobSquarePants commented 4 years ago

@brianpopow @klogeaage This is good progress. Now we need to establish whether the issue is with the jpeg decoder/encoder or with the resizer.

brianpopow commented 4 years ago

@JimBobSquarePants unfortunately its neither. I have tried bmp encoder and png encoder and its the same issue. Also edge detection instead of resizing and still the same issue.

JimBobSquarePants commented 4 years ago

Different decoders too? If not we have a memory allocation problem..

brianpopow commented 4 years ago

Different decoders too? If not we have a memory allocation problem..

same with bmp, png and jpg decoder

edit: but something is funky with the jpeg decoder, its has this weird green pattern output. Here is The output of the OP image: small

lofcz commented 4 years ago

@brianpopow regarding jpeg decoder: there was an extra defect with jpeg images, the image was converted to a super low quality / jittered in addition to one half being blank. No green strips tho.

brianpopow commented 4 years ago

@lofcz yes something is different with the jpeg decoder. Its also defect even without Mutate. Looks like this:

jpg_defect

tredition commented 4 years ago

Good to see I'm not the only one. I have a similar problem with cropping. The following stuff is implemented in a web api project which works fine on my local machine. But when I deploy this to an Azure App Service, cropping leads to broken images. This happens since .NET 5 RC-1.

Code:

var image = Image.Load<Rgba32>(inputFilename);
image.Mutate(m => m.Crop(new Rectangle(left, top, width, height)));
image.Save(outputFilename);

Some PNG images still work fine, but JPG images of any size are broken. You could take any dimensions for cropping, it doesn't matter.

Original image: 2017-06-24-u-bahn-rave-27

Want to crop this area: image

Leads to this result: image

Larger view: image

JimBobSquarePants commented 4 years ago

We're going to need to get issues raised upstream in the dotnet repo. They've changed something which is affecting us.

klogeaage commented 4 years ago

Might I suggest this issue is renamed to something like ".Net 5.0 RC-1 breaks ImageSharp" ?

JimBobSquarePants commented 4 years ago

@brianpopow I just updated your branch to include all the proper .NET5 build targets and updated some test dependencies. I have all tests passing on .NET5 locally. Can you please confirm in your docker instance (I haven't a clue how to do that)?

tannergooding commented 4 years ago

Just commenting so I can get notifications on this. We'd like to have a tracking issue in dotnet/runtime if this does turn out to be a runtime related issue.

I'm also happy to help investigate further if required.

tannergooding commented 4 years ago

@JimBobSquarePants, as an FYI we've logged a tracking issue, https://github.com/dotnet/runtime/issues/42912, as this is something we would classify as blocking-release if it is indeed an issue in dotnet/runtime.

Let us know if we can help with root causing or investigating it further. As we get closer to shipping, the window to get fixes in narrows and so we'd like to try and assist however possible to ensure this gets resolved quickly 😄

JimBobSquarePants commented 4 years ago

Thanks @tannergooding really appreciate your help here!

tocsoft commented 4 years ago

@brianpopow / @klogeaage I have attempted to create a standalone reproduction of the bug at https://github.com/tocsoft/Net5CorruptingImageSharp but I don't seem to be able to cause the issues you both have been seeing could I ask if you could attempt to run the code in repo and see if the outputs. I can't tell if its just my machine doesn't suffer from the issue or my tests are flawed.

tocsoft commented 4 years ago

My reproduction isn't aiming at making it easy for us to debug at this stage its so that, if we can reproduce it consistently with only memory/.net version constants changing, we can raise an issue in against the runtime.

klogeaage commented 4 years ago

@tocsoft, I have now installed Docker (which is new to me), cloned your repo and successfully run your PowerShell script. Unfortunately, the output looks perfectly good, so no reproduction of the issue ☹ Just to re-iterate, I only see the issue when deploying a release configuration to an Azure App Service from Visual Studio 2019 with a self-contained .Net 5 RC-1 runtime being installed. And only when the App Service is rather limited in memory. Is there anyway we can use that exact setup to recreate the issue? A new version of VS2019 has just been released, just for the heck of it, I'll see if that makes any difference. Update: It didn't, same issue.

tocsoft commented 4 years ago

@klogeaage Thanks for trying that.. sounds like I've not set up the test correctly I'll keep working see if I can get local repeatable reproduction. Hoping @brianpopow might end up having an idea on what I've done wrong seeing as he seemed to have managed to reproduce the issue locally/in docker before.

tannergooding commented 4 years ago

Is this only reproing on Windows or has anyone tried on Linux yet?

tannergooding commented 4 years ago

So far I haven't been able to repro.

The test project I made is a simple console app using <PackageReference Include="SixLabors.ImageSharp" Version="1.0.2-alpha.0.9" /> (latest nightly) on net5.0 using the public RC1 from https://get.dot.net and the repro code from: https://github.com/SixLabors/ImageSharp/tree/bp/Issue1356docker

It is being run on a Windows 10 x64 Pro Hyper-V machine with 6 cores and 512MB of RAM (I don't think I can get Windows much lower than that >.>).

Will try to build the latest ImageSharp locally next and will try docker after that.

klogeaage commented 4 years ago

Is this only reproing on Windows or has anyone tried on Linux yet?

I just tried deploying to a Linux based App Service in Azure and there the issue reproduces in almost identical manner, including it goes away with plenty of memory.

And it turns out I can run my test environment at less than half price on Linux, so I'm contemplating switching permanently. But I digress :-)

brianpopow commented 4 years ago

Is this only reproing on Windows or has anyone tried on Linux yet?

@tannergooding so far i have only seen this issue on linux, but the one who started this thread said it fails on windows server 2016 datacenter 10.0.14393, net5 rc1, too.

@lofcz could you please confirm that it is indeed failing on windows?

brianpopow commented 4 years ago

@brianpopow I just updated your branch to include all the proper .NET5 build targets and updated some test dependencies. I have all tests passing on .NET5 locally. Can you please confirm in your docker instance (I haven't a clue how to do that)?

@JimBobSquarePants lots of tests are failing in docker: Failed: 293, Passed: 5299, Skipped: 5, Total: 5597 And that is not even all tests executed. If you want the log files, let me know in slack, its to much to post here.

If you want to execute it in docker just use the following in the root directory of ImageSharp:

docker-compose build
docker-compose up

This will run the Issue1356 project and the output will be written to the host system in docker_output folder (in ImageSharp root directory)

If you want to run the tests just uncomment the last two lines in the dockerfile.

@tocsoft i will look into your reproduction code now.

brianpopow commented 4 years ago

@klogeaage Thanks for trying that.. sounds like I've not set up the test correctly I'll keep working see if I can get local repeatable reproduction. Hoping @brianpopow might end up having an idea on what I've done wrong seeing as he seemed to have managed to reproduce the issue locally/in docker before.

@tocsoft I have tested your reproduction and i can reproduce it with it. Both net5 results (low and high memory fail) and look like this:

garden

small

net31 results are looking fine.

tocsoft commented 4 years ago

ok some more stuff for everyone looks like it might have nothing to do with memory at this stage but in fact its looks like it more related to number of CPUs/core available

The difference between the 2 skus mentioned by @klogeaage is not just memory size but also CPU constraints. Based on the memory looks like we are talking about a 'B1' scale machine and a 'P2V2' the difference on those machines is not just memory by a move from approx. 1 cpu to something more like 4.

Only way i've managed to repo it at the moment is run docker using the hyper-v backend with the hyper-v vm itself being constrained (not just the running container as my test cases above where trying to do).

tocsoft commented 4 years ago

@tannergooding the guide toward the fact it seems to be a CPU count issue rather than a memory constraint issue might help in your verification/tests too.

tannergooding commented 4 years ago

Ah, yeah. I can repro instantly by setting Hyper-V to use 1-core, rather than 6 (even with multiple gigs of memory).

tocsoft commented 4 years ago

looking like its the decoder that triggering the regression, if I generate a hash of pixel data after decoding be before any other process is ran then the hashes differ on 1cpu and are the same on 2 cpus