Let's talk about Pixel

[johannesvollmer]

Hey! I've been trying to implement f32 support for a while now, but progress always stops when I try to modify Pixel in a backwards-compatible and sane way. While we could probably think of some way to add f32 support in a non-breaking way, I see no possibility that doesn't drive us further and further into technical debt. I think we can't continue adding yet another hack to the pixel trait forever. Adding new features will become impossible eventually, it's already hard. This is because the original Pixel trait design is not flexible enough.
I have been thinking about the Pixel type for a whole lot of time now. I've seen #1099, #956, #1464, #1212, #1013, #1499, #1516. That's why I want to call for action right now.

With the upcoming 0.24.0 release planned, which allows us to do breaking changes, we finally have the chance to actually improve the design of the trait in a sustainable manner. I propose we redesign the pixel trait right now! The goal of the redesign would be to maximise the flexibility such that it will last for a long time without major modifications.

Limitations of the current design

As far as I understand, these are currently the biggest problems with the pixel trait:

1. It is not generic enough:
   (F32 Variant of Dynamic Image #1511, Add alpha count associated constant to Pixel trait #956, Pixel::map methods should map to arbitrary subpixel type #1464, Why is the FromColor trait not public? #1212).
   The purpose of a trait is to enable generic code.
   Implementing the pixel trait is problematic for multiple reasons.

   • Color conversions to_rgb is not generic. Rust has traits for conversions, the From and Into traits. These should be used, if at all. Color conversion is more complex than that though, so it should probably be separated.

2. It has too many responsibilities:
   (F32 Variant of Dynamic Image #1511, Simplify Pixel trait #1099, implement std::convert::From<T: Pixel> for Pixel #1013, Why is the FromColor trait not public? #1212)
   Adding multiple responsibilities to a trait is problematic, as the flexibility is reduced further and further. In the std library, we can observe multiple traits that are split up atomically. Many times, traits contain only a single method definition.
   I identified the following responsibilities in the Pixel trait:

   • Field access. Methods like as_slice, from_slice and friends exist to access the samples in the pixel. Granting access to the sample data should be a separate responsibility, just like image operations and codecs should be separated.
   • Color Type hints. COLOR_TYPE: ColorType is passed to the image encoders.
   • Color conversion. This will be reworked anyways. It should be removed from the pixel trait. This allows us to add color conversion without having to modify the pixel trait, which adds stability to the design.
   • Contains some color operations, such as invert, blend, but no other color operations

   All of these could theoretically be separate traits, especially Blend, Invert, and to_xyz color conversions

Furthermore, there are other minor problems:

• Many algorithms use channels4 method of the pixel trait.
  • For example, see hue_rotate_in_place. In reality, the hue algorithm will only make sense for Rgb. These algorithms should probably not use the pixel trait in the first place.
  • horizontal_sample should probably use the generic slice methods, and work with every possible number of channels. Even more idiomatic would be to actually treat the pixel as a whole, by not adding up each sample of a pixel individually. Instead, the algorithm should use hypothetical operations such as blend or sum or average on the pixel.

How to start?

Of course, a redesign is a huge task. And even settling on a design will be challenging. Splitting up the pixel into atomic traits would probably help reduce bike shedding, as color conversion and similar tasks could be added independently. To speed up the design process, it could also help to list the new requirements of the pixel trait(s).

Maybe we can start with the following list of requirements:

• Generic over Sample Type (works with u8, u16, f32, or even f16 if the user wants to implement that)
• Single Responsibility: Field Access. No Color Conversion. Helper functions and color type declarations are done with additional traits and extension traits.
• Generic over Alpha Channel.
• Supports RGB and Luma. Maybe even user-defined colors such as YCbCr? Can we have add and blend for those`?
• Continuous Memory: Internal storage can be assumed to be a primitive array of fixed size. Therefore, all channels have the same type.
• Maybe support converting between sample types? pixel.map(|f32| (f32 255.0) as u8)?
• Supports different color spaces already? What about sRGB vs LinearRGB? From my point of view, this is the only open question.

I propose we remove the color conversion from the pixel trait into a separate module. We could keep the old conversion algorithms until real color spaces are introduced.

Example Design

This is by far not a full design. It should rather be understood as an example. It shows an extreme variant of the trait separation. I just typed this off of my head, so it probably doesn't factor in every requirement yet.

The design focuses on separating concerns and providing default implementations for most methods. The code does not make any assumptions about the sample type, except for it being Primitive. The pixel is never assumed to be a specific color space. The only real assumption made, is that there exists an underlying array of samples. Color space models can be introduced to the crate by adding more traits, probably even without changing the existing traits.

/// The most basic information about a Pixel.
/// Maybe the only information that `ImageBuffer` needs at all!
pub trait PrimitivePixel: Sized + Copy + Clone {

    /// The type of each channel in this pixel.
    type Sample: Primitive;

}

/// Access the samples of the pixel.
pub trait SlicePixel: PrimitivePixel {

    /// A slice of all the samples in this pixel, including alpha.
    fn as_slice(&self) -> &[Self::Sample];

    /// A slice of all the samples in this pixel, including alpha.
    fn as_slice_mut(&mut self) -> &mut [Self::Sample];
    
    fn from_iter(iter: impl Iterator<Item=Self::Sample>) -> Self {
        let mut result = Self::default();
        for (target, source) in result.as_slice_mut().iter_mut().zip(iterator) {
            *target = *source;
        }
        result
    }
    
    fn into_iter(self) -> SamplesIter<Self::Sample> { .. }

    fn apply(&mut self, mapper: impl FnMut(Self::Sample) -> Self::Sample) {
        for value in self.as_slice_mut() {
            *value = mapper(*value);
        }
    }

    fn apply2(&mut self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) {
        for (own, other) in self.as_slice_mut().iter().zip(other.as_slice()) {
            *own = mapper(*own, *other);
        }
    }

    fn map(&self, mapper: impl FnMut(Self::Sample) -> Self::Sample) -> Self {
        let mut cloned = self.clone();
        cloned.apply(mapper);
        cloned
    }

    fn map2(&self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) -> Self {
        let mut cloned = self.clone();
        self.apply2(other, mapper);
        cloned
    }
}


/// Only implemented for Rgba and LumaA
pub trait AlphaPixel: PrimitivePixel {
    fn alpha(&self) -> &Self::Sample;
    fn alpha_mut(&mut self) -> &mut Self::Sample;
}

/// Implemented for all colors manually.
/// This example design is based on the assumption that
/// all colors with alpha will have alpha as the last channel.
/// The assumption is only needed to provide default implementations for the first two methods.
pub trait MaybeAlphaPixel: SlicePixel {
    const HAS_ALPHA: bool;

    fn alpha_channel_count() -> u8 { if Self::HAS_ALPHA { 1 } else { 0 } }

    fn color_channel_count() -> u8 where Self: ArrayPixel {
        Self::CHANNEL_COUNT - Self::alpha_channel_count()
    }

    fn as_color_and_maybe_alpha(&self) -> (&[Self::Sample], Option<&Self::Sample>) {
        let slice = self.as_slice();
        debug_assert_ne!(slice.len(), 0);

        if Self::HAS_ALPHA { (&slice[..slice.len()-1], slice.last()) }
        else { (self.as_slice(), None) }
    }

    fn as_color_and_maybe_alpha_mut(&mut self) -> (&mut [Self::Sample], Option<&mut Self::Sample>) {
        let slice = self.as_slice_mut();

        if Self::HAS_ALPHA {
            let (last, color) = slice.split_last_mut().expect("zero samples found");
            (color, Some(last))
        }
        else { (self.as_slice_mut(), None) }
    }

    fn as_color_slice(&self) -> &[Self::Sample] {
        self.as_color_slice_and_alpha().0
    }

    fn as_color_slice_mut(&self) -> &mut [Self::Sample] {
        self.as_color_slice_and_alpha_mut().0
    }

    fn apply_without_alpha(&mut self, mapper: impl FnMut(Self::Sample) -> Self::Sample) {
        for value in self.as_slice_without_alpha_mut() {
            *value = mapper(*value);
        }
    }

    fn apply2_without_alpha(&mut self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) {
        for (own, other) in self.as_slice_without_alpha_mut().iter().zip(other.as_slice_without_alpha_mut()) {
            *own = mapper(*own, *other);
        }
    }

    fn map_without_alpha(&self, mapper: impl FnMut(Self::Sample) -> Self::Sample) -> Self {
        let mut cloned = self.clone();
        cloned.apply_without_alpha(mapper);
        cloned
    }

    fn map2_without_alpha(&self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) -> Self {
        let mut cloned = self.clone();
        self.apply2_without_alpha(other, mapper);
        cloned
    }
}

/// Add an alpha channel, for example go from RGB to RGBA, or L to LA. No color conversion is happening here.
/// Implemented by all colors.
pub trait IntoWithAlpha: PrimitivePixel {
    type WithAlpha: PrimitivePixel<Sample=Self::Sample>;
    fn with_alpha(self, alpha: Self::Sample) -> Self::WithAlpha;
}

/// Take away an alpha channel, for example go from RGBA to RGB, or LA to L. No color conversion is happening here.
/// Implemented by all colors.
pub trait IntoWithoutAlpha: PrimitivePixel {
    type WithoutAlpha: PrimitivePixel<Sample=Self::Sample>;
    fn without_alpha(self) -> Self::WithAlpha;
}

/// Used to detect the image type when encoding an image.
pub trait InherentColorType {
    const COLOR_TYPE: ColorType;
}

/// Implemented for all colors individually. 
/// Allows converting the sample to a different sample type.
pub trait MapSamples<NewSampleType>: PrimitivePixel {
    type NewPixel: PrimitivePixel<Sample = NewSampleType>;
    fn map_samples(self, mapper: impl FnMut(Self::Sample) -> NewSampleType) -> Self::NewPixel;
}

/// This alias might not be a good idea - it's very monolithic again, just like the old design.
// Most methods should only use the traits they actually need, instead of requiring all these traits.
pub trait Pixel:
    Default + 
    SlicePixel +
    MaybeAlphaPixel +
    IntoWithAlpha + 
    IntoWithoutAlpha
    // not InherentColorType, as it is only useful in a few select circumstances
{ }


// ### example color implementation ###

#[derive(Clone, Copy, Default)]
struct Rgb <T> ([T; 3]);

impl<T> PrimitivePixel for Rgb<T> { type Sample = T; }

impl<T> SlicePixel for Rgb<T> {
    fn as_slice(&self) -> &[Self::Sample] { &self.0 }
    fn as_slice_mut(&mut self) -> &mut [Self::Sample] { &mut self.0 }
}

impl<T> MaybeAlphaPixel for Rgb<T> {
    const HAS_ALPHA: bool = false;
}

// non-generic implementations for the color type, as not all sample types may be supported for a color
impl InherentColorType for Rgb<u8> { const COLOR_TYPE: ColorType = ColorType::Rgb8; }
impl InherentColorType for Rgb<u16> { const COLOR_TYPE: ColorType = ColorType::Rgb16; }
impl InherentColorType for Rgb<f32> { const COLOR_TYPE: ColorType = ColorType::Rgb32F; }

impl<T> IntoWithAlpha for Rgb<T> {
    type WithAlpha = Rgba<T>;
    fn with_alpha(self, alpha: Self::Sample) -> Self::WithAlpha {
        let [r,g,b] = self.0;
        Rgba([r,g,b,a])
    }
}

impl<T> IntoWithoutAlpha for Rgb<T> {
    type WithoutAlpha = Self;
    fn without_alpha(self) -> Self::WithoutAlpha { self }
}

impl<T, D> MapSamples<D> for Rgb<T> {
    type NewPixel = Rgb<D>;

    fn map_samples(self, mapper: impl FnMut(Self::Sample) -> D) -> Self::NewPixel {
        self.as_slice().iter().cloned().map(mapper).collect()
    }
}

// ### inside image buffer and dynamic image ###
struct ImageBuffer<Px, Container> { .. } // no trait bound on struct definition
impl<Px,Container> ImageBuffer<Px, Container> where Px: PrimitivePixel, Container: AsRef<[Px::Sample]> {
    fn pub fn from_raw(width: u32, height: u32, buf: Container) -> Option<ImageBuffer<Px, Container>>
        where Px: ArrayPixel // channel count required for length check
    { ... }

    fn save(&self, write: impl Write) -> ImageResult where Px: InherentColorType { ... }

    fn pixels(&self) -> Pixels<Px> where Px: ArrayPixel { ... } 
}

// ### inside rgb algorithms ###
fn hue_rotate(image: impl GenericImage<Pixel=RGB>){
   let [r,g,b] = color.to_array();
   // modify values
   let color = RGB::from_array([r,g,b]);
}

[johannesvollmer]

Going back a step and merging some of the unnecessarily split traits, this could be an alternative design:

/// A pixel containing a sample for each channel.
/// Grants access to the sample data, and does nothing more. No conversions.
/// Basically nothing but a trait for wrapped primitive arrays, which is aware of alpha channels.
pub trait Pixel: Sized + Copy + Clone + Default {

    /// The type of each channel in this pixel.
    type Sample: Primitive;
    
    const CHANNEL_COUNT: u8;
    
    const HAS_ALPHA: bool; // assumes no more than one alpha channel per pixel
        
    /// A slice of all the samples in this pixel, including alpha.
    fn as_slice(&self) -> &[Self::Sample];

    /// A slice of all the samples in this pixel, including alpha.
    fn as_slice_mut(&mut self) -> &mut [Self::Sample];
    

    // ### default implementations. these could also be put in a separate PixelExt trait ### 

    fn bytes_per_channel() -> usize { ... }
    fn bytes_per_pixel() -> usize { ... }
    fn alpha_channel_count() -> u8 { ... }
    fn color_channel_count() -> u8 { ... }
    fn from_iter(iter: impl IntoIterator<Item=Self::Sample>) -> Self { ... }
    fn apply(&mut self, mapper: impl FnMut(Self::Sample) -> Self::Sample) { ... }
    fn apply2(&mut self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) { ... }
    fn map(&self, mapper: impl FnMut(Self::Sample) -> Self::Sample) -> Self { ... }
    fn map2(&self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) -> Self { ... }
    fn as_color_slice_and_alpha(&self) -> (&[Self::Sample], Option<&Self::Sample>) { ... }
    fn as_color_slice_and_alpha_mut(&mut self) -> (&mut [Self::Sample], Option<&mut Self::Sample>) { ... }
    fn as_color_slice(&self) -> &[Self::Sample] { ... }
    fn as_color_slice_mut(&self) -> &mut [Self::Sample] { ... }
    fn apply_without_alpha(&mut self, mapper: impl FnMut(Self::Sample) -> Self::Sample) { ... }
    fn apply2_without_alpha(&mut self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) { ... }
    fn map_without_alpha(&self, mapper: impl FnMut(Self::Sample) -> Self::Sample) -> Self { ... }
    fn map2_without_alpha(&self, other: &Self, mapper: impl FnMut(Self::Sample, Self::Sample) -> Self::Sample) -> Self { ... }
}


/// Add an alpha channel, for example go from RGB to RGBA, or L to LA. No color conversion is happening here.
/// Implemented by all colors.
// This is a separate trait because it is not simply data access and has an associated type
pub trait WithAlpha: Pixel {
    type WithAlpha: Pixel<Sample=Self::Sample>;
    fn with_alpha(self, alpha: Self::Sample) -> Self::WithAlpha;
}

/// Take away an alpha channel, for example go from RGBA to RGB, or LA to L. No color conversion is happening here.
/// Implemented by all colors.
// This is a separate trait because it is not simply data access and has an associated type
pub trait WithoutAlpha: Pixel {
    type WithoutAlpha: Pixel<Sample=Self::Sample>;
    fn without_alpha(self) -> Self::WithoutAlpha;
}

/// Used to detect the image type when encoding an image.
pub trait InherentColorType {
    const COLOR_TYPE: ColorType;
}

/// Implemented for all colors individually. 
/// Allows converting the sample to a different sample type.
// This is a separate trait because it has an associated type
pub trait MapSamples<NewSampleType>: Pixel {
    type NewPixel: Pixel<Sample = NewSampleType>;
    fn map_samples(self, mapper: impl FnMut(Self::Sample) -> NewSampleType) -> Self::NewPixel;
}



// ### example color implementation ###

#[derive(Clone, Copy, Default)]
struct Rgb <T> ([T; 3]);

impl<T> Pixel for Rgb<T> where T: Primitive { 
    type Sample = T; 
    const HAS_ALPHA: bool = false;
    const CHANNEL_COUNT: u8 = 3;
    
    fn as_slice(&self) -> &[Self::Sample] { &self.0 }
    fn as_slice_mut(&mut self) -> &mut [Self::Sample] { &mut self.0 }
}


// non-generic implementations for the color type, as not all sample types may be supported for a color
impl InherentColorType for Rgb<u8> { const COLOR_TYPE: ColorType = ColorType::Rgb8; }
impl InherentColorType for Rgb<u16> { const COLOR_TYPE: ColorType = ColorType::Rgb16; }
impl InherentColorType for Rgb<f32> { const COLOR_TYPE: ColorType = ColorType::Rgb32F; }


impl<T> WithAlpha for Rgb<T> where T: Primitive {
    type WithAlpha = Rgba<T>;
    fn with_alpha(self, a: Self::Sample) -> Self::WithAlpha {
        let [r,g,b] = self.0;
        Rgba([r,g,b,a])
    }
}

impl<T> WithoutAlpha for Rgb<T> where T: Primitive {
    type WithoutAlpha = Self;
    fn without_alpha(self) -> Self::WithoutAlpha { self }
}

impl<T, D> MapSamples<D> for Rgb<T> where T: Primitive, D: Primitive {
    type NewPixel = Rgb<D>;

    fn map_samples(self, mapper: impl FnMut(Self::Sample) -> D) -> Self::NewPixel {
        Rgb::from_iter(self.as_slice().iter().cloned().map(mapper))
    }
}

[drewcassidy]

I wonder if it might make sense to not use any enumerations for color formats, and instead just have RGB8 et al be type aliases. Const generics could also allow for >4 channel images. Perhaps the need to avoid changing the API is unfounded and its time for a 1.0.0?

[fintelia]

@drewcassidy I'm not sure I follow. The reason that we use enums for color formats is because users can load files at runtime that can be in any format. The PngDecoder::color_type method returns ColorType to signal what format the PNG is. And that determines which variant of DynamicImage the higher level methods like image::open would return.