A compatible option for decoding to various pixel types

[HeroicKatora]

The only point of these helper methods is to allocate a buffer, such that I don't have to repeat the boilerplate allocation code twice in every test.
The Rgb::SubPixel type will not resolve to an f32 color type. The Pixel trait is only really implemented for Rgb8 and Rgb16, not f32

This helper method as sketched would then be better suited for impl ImageBuffer. It is similar to DynamicImage::from_decoder. I'm afraid the Pixel issue is something we can't easily resolve in this major version of image. We could, however, already add an improved trait that is just used for these method. Something like:

#[repr(u8)]
enum SampleType {
    U8 = 0,
    U16 = 1,
…
    F32 = 2,
}

trait Sample: Primitive {
    const TYPE: SampleType;
}

mod hidden {
    pub trait PixelV2 { …  }
    impl<T: Sample> PixelV2 for Rgb<T> { … }
}

impl<Px> ImageBuffer<Px, Vec<Px::Subpixel>>
where
    Px: hidden::PixelV2,
{
    fn from_decoder(mut decoder: impl ImageDecoder) -> ImageResult<Self> {
        if Px::COLOR_TYPE != decoder.color_type() { … }
    }
}

In any case, I would much rather discuss these helpers in a separate PR after the core EXR functionality has been added together with the new ColorType variant :)

Originally posted by @HeroicKatora in #1475 (comment) (Read the responses from before, this is a condensed sketch derived from the oberservation of @johannesvollmer in supporting a large range of types in EXR decoder).

[fintelia]

The problem with the current Pixel trait is how COLOR_TYPE is implemented. After stripping out the macros, the code looks like:

impl<T: Primitive + 'static> Pixel for Rgba<T> {
    const COLOR_TYPE: ColorType =
        [ColorType::Rgba8, ColorType::Rgba16][(std::mem::size_of::<T>() > 1) as usize];
}

At one point I looked into using TypeId::of<T>() instead of just matching on the size, but sadly using that function from const contexts is still unstable (and there doesn't seem to be a ton of motion on changing that).

I guess that PixelV2 could use the same size_of trick but assume size=4 means f32 (which would of course break for i32/u32). Doesn't feel like a great solution though

[johannesvollmer]

Wouldn't it be possible to have one macro call for each color type? Like:

impl_pixel!{ RGB, f32, 3  }
impl_pixel!{ RGBA, f32, 4 }
impl_pixel!{ RGB, u8, 3  }
impl_pixel!{ RGBA, u8, 4 }
impl_pixel!{ Y, f32, 1  }
...

This might simplify the implementation of the macro

[HeroicKatora]

Not without getting rid of impl<T: Primitive> Pixel for Rgba<T> since this impl overlaps those individual ones, which is forbidden.

[johannesvollmer]

Yes, but I don't think this would be a problem. Each macro invocation implements the type directly instead of having one generic impl<T> definition. Also, I don't see the value of having one generic implementation, when it does not support all generic parameters correctly (f32 is not correctly implemented).

I'll try to find out whether implementing pixel this way would be an easy change

[johannesvollmer]

I think I have arrived at a possible solution. I changed the macro definition. It should be a backwards-compatible change, except for the generic parameter now being replaced with concrete primitive types.

It's not backwards compatible, because the user now can't put their own u128 into the Rgb struct. It would not have yielded the correct color type anyways, so I don't think anyone actually does that?

Calling the macro now looks like this:

impl_colors! {
    Rgb, #[doc="Red, green, and blue. No alpha. Can contain three `u8`, `u16` or `f32` values."],
    "RGB", 3, false,
    u8:Rgb8, u16:Rgb16, f32:Rgb32F
}
impl_colors! {
    Rgba, #[doc="Red, green, blue and alpha. Can contain four `u8`, `u16` or `f32` values."],
     "RGBA", 4, true,
     u8:Rgba8, u16:Rgba16, f32:Rgba32F
}
impl_colors! {
    Bgr, #[doc="Red, green, and blue. No alpha. Can contain three `u8` or `u16` values."],
     "BGR", 3, false, 
     u8:Bgr8
}
impl_colors! {
    Bgra, #[doc="Red, green, blue and alpha. Can contain four `u8` or `u16` values."],
     "BGRA", 4, true, 
     u8:Bgra8
}
impl_colors! {
    Luma, #[doc="Luminance. No alpha. Can contain one `u8` or `u16` value."],
     "Y", 1, false, 
     u8:L8, u16:L16
}
impl_colors! {
    LumaA, #[doc="Luminance and alpha. Can contain two `u8` or `u16` values."],
     "YA", 2, true, 
     u8:La8, u16:La16
}

[johannesvollmer]

Also, there seem to be some Luma<SubPixel<..>> problems going on, which seems to be caused by exactly it not being a generic implementation anymore

[johannesvollmer]

The pixel type could be drastically simplified, even using a generic implementation again, if we could split up the channels and the bit depth (RGB|RGBA|Luma|LumaA + u8|u16|f32). Most code could probably be generic over the primitive type, instead of having duplicate code for all those. Of course, that's something for a future release.

[HeroicKatora]

It should be a backwards-compatible change, except for the generic parameter now being replaced with concrete primitive types.

I'm not a fan of taking chances on this one. It either is compatible according to the Rust RFC rules or we add a second trait. In any case, the cost of a different trait doesn't seem too high unless your experimentation yields a different result.

[fintelia]

I do think a breaking change could be justified if a significant enough improvement was being made to the pixel structs. But we shouldn't release a minor version which could risk breaking downstream users

[HeroicKatora]

That's what I was trying to say: Definitely enough to justify going to the next major version, not enough to disregard SemVer and try to come up with justifications to represent it as compatible.

[johannesvollmer]

I'm currently trying to add f32 support for the dynamic image, where it is necessary to implement f32 support for the pixel type. The dynamic image will be a breaking change anyways, and I already refactored the pixel macro for that use case.

How about we target both the dynamic image f32 variant and the pixel f32 support for the next branch? These helper methods can then be built upon that. I suggest that, because I personally don't want to spend too much time on researching a backwards-compatible change that will be replaced with a clean implementation anyways haha