Imagesharp: Image.Load() on PNG sometimes results in NULL return, no exception

[justintubbs]

I have PNG files that when used with Image.Load() return a NULL with no indication of what the error or issue might be.

Example PNG images:
image1.png
image2.png

Using ImageSharp nightly dev NuGet (also tried stable/release which has same issue)

<PackageReference Include="SixLabors.ImageSharp" Version="1.0.0-dev000642" />

Code I'm using:

using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Formats;

public IActionResult UpdateReportBranding([FromForm] IFormFile file)
{
   var ms = new MemoryStream();
   file.OpenReadStream().CopyTo(ms);
   IImageDecoder decoder = new SixLabors.ImageSharp.Formats.Png.PngDecoder();
   var image = Image.Load(ms, decoder);
   if (image == null) << IS NULL HERE
   {
       ModelState.AddModelError("File", "Image was unable to load and may not be properly formatted.");
       return BadRequest();
   }

[JimBobSquarePants]

Hi @justintubbs

As with #440 I cannot access the images to investigate them. Could you please allow access.

Cheers

James

[dlemstra]

This will happen when there is no data chunk in the file. I think this PR will fix the issue: #442. We can add a unit test once we can access those images.

[justintubbs]

Is anyone else having issues getting to these photos? I can view them in multiple browsers, even in Incognito/Private browsing mode

[tocsoft]

Getting 403 Forbidden here. My guess is you have some form of IP address filtering on them.

[tocsoft]

I'm even getting it when just navigating to http://justintubbs.com/

[justintubbs]

[justintubbs]

[justintubbs]

I'm just using Windows "Snipping Tool" and saving as PNG or JPG format here...you should be able to replicate yourself.

[JimBobSquarePants]

I'm unable to replicate this using the current repository codebase with any of your posted images nor with any new images generated using the Window Snipping Tool in either format. Would you be able to test against that code locally?

I'm using:

• Windows 10 Version 1709 (OS Build 16299.192)
• Snipping Tool 10.0.16299

[justintubbs]

I'll try to put an example together to see what is going on. Since I'm using .NET Core 2.0 to upload a file, it might be different than loading a file from disk.

[saucecontrol]

Oh hey, just butting in again with some [potentially helpful] info. I can report the following:

1. I was able to download the files linked in the original issue
2. The files attached later appear to be re-encoded versions of those images
3. Both the original and the re-encoded files load fine in 1.0.0-beta0002.

@justintubbs Looking at your example code, it appears you simply forgot to reset the stream position to 0 before attempting to load the image. I reckon an exception should be thrown by the decoder there, but the images themselves are fine, and the decoder handles them fine if it can find them

[JimBobSquarePants]

@saucecontrol Thanks for that! I couldn't see the wood for the trees there.

@justintubbs The way you are loading the images is unconventional. Normally you would use Image<TPixel>.Load(Stream). That method attempts to identify the correct codec to use and will throw an exception if it cannot do so.

We've already opened #442 that will add throwing an exception if there is not data contained within the png and we can ensure all other decoders do similar with appropriate messaging.

[justintubbs]

@JimBobSquarePants I tried changing my code implementation to not specify a decoder when using Image.Load():

![image](https://user-images.githubusercontent.com/12716768/35387347-9bce9a60-018c-11e8-9500-9ed59c72c210.png)

[justintubbs]

[justintubbs]

@saucecontrol Your post was helpful, as I checked the ms MemoryStream in the above error and saw that it's Position == Length.

In all the .NET Core examples of uploading files, nobody is telling people to rewind the MemoryStream using .Seek():
https://docs.microsoft.com/en-us/aspnet/core/mvc/models/file-uploads
https://docs.microsoft.com/en-us/aspnet/core/tutorials/razor-pages/uploading-files#add-a-helper-method-to-upload-files

Problem is solved though. Perhaps ImageSharp could warn if the Stream being Loaded has Position == Length?

[dlemstra]

Checking if Position == Length sounds like a good plan to aid developers. But we will need to check if we can use those properties. Not sure what we need to check for that though.

[JimBobSquarePants]

No one in those examples is copying one stream to another. If you copy to a memory stream it's gonna be at the length position in copy completion. I'm not sure why you are not using the filestream directly.

We can check if the position is equal to the length and throw there but nothing more.

[antonfirsov]

We can add hints and remarks in docs, and maybe in exception messages when we fail to recognize the header.

[justintubbs]

Thanks all, appreciate the quick feedback and responses. (Apologies for creating the issue...)

[JimBobSquarePants]

Och, no need for an apology at all! It's taught us that we need to make our error messages more developer friendly and add that check. 😄