An improved ImageOutputFormat design

[HeroicKatora]

Another solution that comes to mind for the next version could be something like this:

pub enum ImageOutputFormat {
    Png(PngEncoderOptions),
    Jpeg(JpegEncoderOptions),
    Pnm(PngEncoderOptions),
    Gif(GifEncoderOptions,
    Ico(IcoEncoderOptions),
    Bmp(BmpEncoderOptions),
    Farbfeld(FarbfeldEncoderOptions),
    Tga(TgaEncoderOptions),
    Avif(AvifEncoderOptions),
}

// NOTE: This already exists in the current version, but using `From`
// and having a variant in `ImageOutputFormat` for unsupported formats.
impl TryFrom<ImageFormat> for ImageOutputFormat {
    type Error = UnsupportedImageOutputFormat;

    fn try_from(fmt: ImageFormat) -> Result<Self, Self::Error> {
        match fmt {
            ImageFormat::Png => Ok(Self::Png(Default::default())),
            ImageFormat::Jpeg => Ok(Self::Jpeg(Default::default())),
            ImageFormat::Pnm => Ok(Self::Pnm(Default::default())),
            ImageFormat::Gif => Ok(Self::Gif(Default::default())),
            ImageFormat::Ico => Ok(Self::Ico(Default::default())),
            ImageFormat::Bmp => Ok(Self::Bmp(Default::default())),
            ImageFormat::Farbfeld => Ok(Self::Farbfeld(Default::default())),
            ImageFormat::Tga => Ok(Self::Tga(Default::default())),
            ImageFormat::Avif => Ok(Self::Avif(Default::default())),
            f => Err(UnsupportedImageOutputFormat(f)),
        }
    }
}

Originally posted by @paolobarbolini in #1152 (comment)

[HeroicKatora]

Two points of feedback:

• The names for individual variants are rather long and unhandy. Would you find PngOptions, JpegOptions to be too ambiguous?
• Given that the fidelity (or quality) or speed setting is present in multiple formats and it is already an approximation stating intent rather than a guarantee, should there be a common constructor? For example, I could imagine png defaulting to using automatic row filter mode when a high compression ratio is requested.

[est31]

Would you find PngOptions, JpegOptions to be too ambiguous?

Depends where they are located. If they are in the codecs::png/codecs::jpeg etc. modules, it'd be confusing. If they were in a top level encoder module next to each other, and the first line of rustdoc explicitly said they exist for encoding, it'd be ok I think.

[est31]

As an alternative, one can also add #[non_exhaustive] to all enum variants:

pub enum ImageOutputFormat {
    #[non_exhaustive]
    Png,
    #[non_exhaustive]
    Jpeg(u8),
    #[non_exhaustive]
    Pnm(PNMSubtype),
    Gif,
    #[non_exhaustive]
    Ico,
    #[non_exhaustive]
    Bmp,
    #[non_exhaustive]
    Farbfeld,
    #[non_exhaustive]
    Tga,
    #[non_exhaustive]
    Unsupported(String),
    // some variants omitted
}

Given that the fidelity (or quality) or speed setting is present in multiple formats and it is already an approximation stating intent rather than a guarantee, should there be a common constructor?

The issue with this is that different libraries interpet the value differently. A quality setting of 5 can be sth different for avif from a quality setting of 5 jpeg. Same goes for speed. Not even sure the scales are linear..

[fintelia]

Given that the fidelity (or quality) or speed setting is present in multiple formats and it is already an approximation stating intent rather than a guarantee, should there be a common constructor?

The issue with this is that different libraries interpet the value differently. A quality setting of 5 can be sth different for avif from a quality setting of 5 jpeg. Same goes for speed. Not even sure the scales are linear.

I think the right approach is to have an enum with semantic options rather than trying to have a linear scale. We could cover most of the common cases just with:

enum EncodeQuality {
    VeryFast,
    Fast,
    Balanced,
    HighQuality,
    VeryHighQuality,
}

[HeroicKatora]

The #[non_exhaustive] on each variant makes construction, and thus usage downstream, far harder than both the current case, where only the enum itself is non-exhaustive, and the original proposal. Speaking of the current enum being non-exhaustive, it would be interesting to test some of the more configurable instances, jpeg, by adding a new variant in 0.23 already before completely deprecating the style.

The issue with this is that different libraries interpet the value differently. A quality setting of 5 can be sth different for avif from a quality setting of 5 jpeg. Same goes for speed. Not even sure the scales are linear..

That sounds a bit problematic in itself. A common interface for all types would be better. Prior art: the list of presets supported by ffmpeg/x264 which for some reason is not symmetrical. Over time I would like to see some quantifiable and queriable value for the difference between two settings in terms of speedup (also visual fidelity but that is hard to quantify) but that's for some far future and by no means necessary. (edit: The simple design of @fintelia would probably suffice).

[BezPowell]

Over time I would like to see some quantifiable and queriable value for the difference between two settings in terms of speedup (also visual fidelity but that is hard to quantify) but that's for some far future and by no means necessary. (edit: The simple design of @fintelia would probably suffice).

I'm very much a newbie to this, but would it be possible to eventually have a scale for visual fidelity loosely based upon SSIM values? I saw this file which tries to roughly map jpeg quality levels to ssim values, not sure what encoder they're using for it. Each codec could then have a hashmap for which quality setting loosely maps to a visual quality tier?

[fintelia]

I'm realizing that for many formats there are actually three dimensions we can trade off between: speed, compressed size, and image quality. Not sure the best way to encode the different options between those

[paolobarbolini]

I'm realizing that for many formats there are actually three dimensions we can trade off between: speed, compressed size, and image quality. Not sure the best way to encode the different options between those

Yeah that's why I was thinking about having a separate options struct for every format. Every format would then implement Default, probably choosing medium settings between compression, size and encode speed.

The names for individual variants are rather long and unhandy. Would you find PngOptions, JpegOptions to be too ambiguous?

I'm ok with just calling it [format]Options.

[kaj]

For what it's worth, I quite like the initial suggestion in this issue, the enum of Format(FormatEncoderOptions) alternatives. But maybe the enum itself should be called ImageEncoderFormat for consistency?

To be able to handle some kind of generic encoder options, why not just have a separate type GenericEncoderOptions and impl From<GenericEncoderOptions> for each of the FormatEncoderOptions ?

If the name [Format]EncoderOptions is too long, how about encoder::[Format]Options. That way the calling code can decide if it should use image::encoder::JpegOptions or type out encoder::JpegOptions to avoid some conflict. If so, the ImageEncoderFormat may also be named encoder::ImageFormat.