Imagesharp: Save method with IEncoderOptions behavior is misleading

[vpenades]

Prerequisites

• [x] I have written a descriptive issue title
• [x] I have verified that I am running the latest version of ImageSharp
• [x] I have verified if the problem exist in both DEBUG and RELEASE mode
• [x] I have searched open and closed issues to ensure it has not already been reported

Description

When using this method using an explicit PngEncoderOptions:
public Image<TColor> Save(Stream stream, IEncoderOptions options);

and probably this one too:
public Image<TColor> Save(string filePath, IEncoderOptions options);

I've found that sometimes they worked and sometimes not. The reason was because I was assuming that if you call these methods with PngEncoderOptions is because you want to write a PNG.

What actually happens is that it still uses the encoder from the file format used to load the image. If the image was originally a png, everything's fine. If not, then my encoding options are decimated and then it uses the source image original encoder.

Steps to Reproduce

Use public Image<TColor> Save(Stream stream, IEncoderOptions options); to save an image, loaded from a JPG file, with PngEncodingOptions

Proposed behavior

I think the API should behave by intent, if someone uses PngEncodingOptions is because he wants to save a PNG. Checking the architecture, I think there's some work required to infer, from the options object, which encoder/decoder to use. Maybe by letting EncodingOptions to have an abstract property that returns the file format associated with these options.

If that's not possible, I think the second best option would be to throw an exception in case there's a mismatch between the options and the encoder.

System Configuration

• ImageSharp version: 1.0.0.59
• Other ImageSharp packages and versions:
• Environment (Operating system, version and so on):
• .NET Framework version:
• Additional information:

[dlemstra]

I think you are correct and the behavior should be changed. I will pick this up next week.

[tocsoft]

maybe we should just drop the XxxxEncoderOptions/IXxxxEncoderOptions objects and just move their options directly onto the XxxEncoder objects, that would simplify the Image.Save(...) APIs, I feel it would be more explicit around which encoder we are going to be using and also reduces the surface area of our public API. as we can remove all the IEncoderOptions overloads.

Whatever we do we will also want to do the same things to the Image.Load(...)/XxxDecoder overloads so they mimic each other.

[vpenades]

@tocsoft I have mixed feelings about removing Encoder/DecoderOptions; removing them would certainly simplify the API, but at the same time, as ImageSharp begins supporting more exotic formats, the requirement for per-format options would be life saving.

For example, I've wrote an implementation for ICO file format, to be able to save icon files; it takes any image as source, and in the enconding options you can choose which size levels and color patters you want to store, and upon saving, it does all the resizes for all the sizes selected.

I also have code to support DDS and it would also require some special options, like supporting mipmap levels, cubemaps, etc. In fact, in order to save a cubemap DDS you would need 6 images as source... which is way beyond what can be achieved with "options" objects.

Maybe the solution is just to have a general "options" for all formats, that would cover basic usage, and then, for exotic options, to use load/save options specific to the file format, something like this:

DdsEncoder : IImageEncoder
{
// default encoder
public void Encode<TColor>(Image<TColor> image, Stream stream, IEncoderOptions options) where TColor : struct, IPixel<TColor>
}

// specific cubemap encoder for DDS format
public void EncodeCubeMap(Image<TColor> up,Image<TColor> down,Image<TColor> left,Image<TColor> right,Image<TColor> front,Image<TColor> back, Stream stream, IEncoderOptions options) where TColor : struct, IPixel<TColor>
}

[tocsoft]

This is now being closed as we no longer have EncoderOptions and instead set the configuration directly on the encoder and thus it will save it in the format specified.